| Message ID | 20210603234526.2503590-3-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/surface: aggregator: Extend user-space interface for events | expand |
Hi Maximilian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.13-rc4 next-20210603]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Maximilian-Luz/platform-surface-aggregator-Extend-user-space-interface-for-events/20210604-074904
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f88cd3fb9df228e5ce4e13ec3dbad671ddb2146e
config: x86_64-randconfig-r016-20210604 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5c0d1b2f902aa6a9cf47cc7e42c5b83bb2217cf9)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/7b6b5a24e7c8e389e231ee547972e871145f5972
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Maximilian-Luz/platform-surface-aggregator-Extend-user-space-interface-for-events/20210604-074904
git checkout 7b6b5a24e7c8e389e231ee547972e871145f5972
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/platform/surface/aggregator/controller.c:2317:23: warning: variable 'nf_head' set but not used [-Wunused-but-set-variable]
struct ssam_nf_head *nf_head;
^
drivers/platform/surface/aggregator/controller.c:2386:23: warning: variable 'nf_head' set but not used [-Wunused-but-set-variable]
struct ssam_nf_head *nf_head;
^
2 warnings generated.
vim +/nf_head +2317 drivers/platform/surface/aggregator/controller.c
2289
2290 /**
2291 * ssam_controller_event_enable() - Enable the specified event.
2292 * @ctrl: The controller to enable the event for.
2293 * @reg: The event registry to use for enabling the event.
2294 * @id: The event ID specifying the event to be enabled.
2295 * @flags: The SAM event flags used for enabling the event.
2296 *
2297 * Increment the event reference count of the specified event. If the event has
2298 * not been enabled previously, it will be enabled by this call.
2299 *
2300 * Note: In general, ssam_notifier_register() with a non-observer notifier
2301 * should be preferred for enabling/disabling events, as this will guarantee
2302 * proper ordering and event forwarding in case of errors during event
2303 * enabling/disabling.
2304 *
2305 * Return: Returns zero on success, %-ENOSPC if the reference count for the
2306 * specified event has reached its maximum, %-ENOMEM if the corresponding event
2307 * entry could not be allocated. If this is the first time that this event has
2308 * been enabled (i.e. the reference count was incremented from zero to one by
2309 * this call), returns the status of the event-enable EC-command.
2310 */
2311 int ssam_controller_event_enable(struct ssam_controller *ctrl,
2312 struct ssam_event_registry reg,
2313 struct ssam_event_id id, u8 flags)
2314 {
2315 u16 rqid = ssh_tc_to_rqid(id.target_category);
2316 struct ssam_nf_refcount_entry *entry;
> 2317 struct ssam_nf_head *nf_head;
2318 struct ssam_nf *nf;
2319 int status;
2320
2321 if (!ssh_rqid_is_event(rqid))
2322 return -EINVAL;
2323
2324 nf = &ctrl->cplt.event.notif;
2325 nf_head = &nf->head[ssh_rqid_to_event(rqid)];
2326
2327 mutex_lock(&nf->lock);
2328
2329 entry = ssam_nf_refcount_inc(nf, reg, id);
2330 if (IS_ERR(entry)) {
2331 mutex_unlock(&nf->lock);
2332 return PTR_ERR(entry);
2333 }
2334
2335 ssam_dbg(ctrl, "enabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n",
2336 reg.target_category, id.target_category, id.instance,
2337 entry->refcount);
2338
2339 if (entry->refcount == 1) {
2340 status = ssam_ssh_event_enable(ctrl, reg, id, flags);
2341 if (status) {
2342 kfree(ssam_nf_refcount_dec(nf, reg, id));
2343 mutex_unlock(&nf->lock);
2344 return status;
2345 }
2346
2347 entry->flags = flags;
2348
2349 } else if (entry->flags != flags) {
2350 ssam_warn(ctrl,
2351 "inconsistent flags when enabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n",
2352 flags, entry->flags, reg.target_category,
2353 id.target_category, id.instance);
2354 }
2355
2356 mutex_unlock(&nf->lock);
2357 return 0;
2358 }
2359 EXPORT_SYMBOL_GPL(ssam_controller_event_enable);
2360
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index cd3a6b77f48d..49edddea417e 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -2287,6 +2287,141 @@ int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_not } EXPORT_SYMBOL_GPL(ssam_notifier_unregister); +/** + * ssam_controller_event_enable() - Enable the specified event. + * @ctrl: The controller to enable the event for. + * @reg: The event registry to use for enabling the event. + * @id: The event ID specifying the event to be enabled. + * @flags: The SAM event flags used for enabling the event. + * + * Increment the event reference count of the specified event. If the event has + * not been enabled previously, it will be enabled by this call. + * + * Note: In general, ssam_notifier_register() with a non-observer notifier + * should be preferred for enabling/disabling events, as this will guarantee + * proper ordering and event forwarding in case of errors during event + * enabling/disabling. + * + * Return: Returns zero on success, %-ENOSPC if the reference count for the + * specified event has reached its maximum, %-ENOMEM if the corresponding event + * entry could not be allocated. If this is the first time that this event has + * been enabled (i.e. the reference count was incremented from zero to one by + * this call), returns the status of the event-enable EC-command. + */ +int ssam_controller_event_enable(struct ssam_controller *ctrl, + struct ssam_event_registry reg, + struct ssam_event_id id, u8 flags) +{ + u16 rqid = ssh_tc_to_rqid(id.target_category); + struct ssam_nf_refcount_entry *entry; + struct ssam_nf_head *nf_head; + struct ssam_nf *nf; + int status; + + if (!ssh_rqid_is_event(rqid)) + return -EINVAL; + + nf = &ctrl->cplt.event.notif; + nf_head = &nf->head[ssh_rqid_to_event(rqid)]; + + mutex_lock(&nf->lock); + + entry = ssam_nf_refcount_inc(nf, reg, id); + if (IS_ERR(entry)) { + mutex_unlock(&nf->lock); + return PTR_ERR(entry); + } + + ssam_dbg(ctrl, "enabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", + reg.target_category, id.target_category, id.instance, + entry->refcount); + + if (entry->refcount == 1) { + status = ssam_ssh_event_enable(ctrl, reg, id, flags); + if (status) { + kfree(ssam_nf_refcount_dec(nf, reg, id)); + mutex_unlock(&nf->lock); + return status; + } + + entry->flags = flags; + + } else if (entry->flags != flags) { + ssam_warn(ctrl, + "inconsistent flags when enabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", + flags, entry->flags, reg.target_category, + id.target_category, id.instance); + } + + mutex_unlock(&nf->lock); + return 0; +} +EXPORT_SYMBOL_GPL(ssam_controller_event_enable); + +/** + * ssam_controller_event_disable() - Disable the specified event. + * @ctrl: The controller to disable the event for. + * @reg: The event registry to use for disabling the event. + * @id: The event ID specifying the event to be disabled. + * @flags: The flags used when enabling the event. + * + * Decrement the reference count of the specified event. If the reference count + * reaches zero, the event will be disabled. + * + * Note: In general, ssam_notifier_register()/ssam_notifier_unregister() with a + * non-observer notifier should be preferred for enabling/disabling events, as + * this will guarantee proper ordering and event forwarding in case of errors + * during event enabling/disabling. + * + * Return: Returns zero on success, %-ENOENT if the given event has not been + * enabled on the controller. If the reference count of the event reaches zero + * during this call, returns the status of the event-disable EC-command. + */ +int ssam_controller_event_disable(struct ssam_controller *ctrl, + struct ssam_event_registry reg, + struct ssam_event_id id, u8 flags) +{ + u16 rqid = ssh_tc_to_rqid(id.target_category); + struct ssam_nf_refcount_entry *entry; + struct ssam_nf_head *nf_head; + struct ssam_nf *nf; + int status = 0; + + if (!ssh_rqid_is_event(rqid)) + return -EINVAL; + + nf = &ctrl->cplt.event.notif; + nf_head = &nf->head[ssh_rqid_to_event(rqid)]; + + mutex_lock(&nf->lock); + + entry = ssam_nf_refcount_dec(nf, reg, id); + if (WARN_ON(!entry)) { + mutex_unlock(&nf->lock); + return -ENOENT; + } + + ssam_dbg(ctrl, "disabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", + reg.target_category, id.target_category, id.instance, + entry->refcount); + + if (entry->flags != flags) { + ssam_warn(ctrl, + "inconsistent flags when disabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", + flags, entry->flags, reg.target_category, + id.target_category, id.instance); + } + + if (entry->refcount == 0) { + status = ssam_ssh_event_disable(ctrl, reg, id, flags); + kfree(entry); + } + + mutex_unlock(&nf->lock); + return status; +} +EXPORT_SYMBOL_GPL(ssam_controller_event_disable); + /** * ssam_notifier_disable_registered() - Disable events for all registered * notifiers. diff --git a/include/linux/surface_aggregator/controller.h b/include/linux/surface_aggregator/controller.h index cf4bb48a850e..7965bdc669c5 100644 --- a/include/linux/surface_aggregator/controller.h +++ b/include/linux/surface_aggregator/controller.h @@ -838,4 +838,12 @@ int ssam_notifier_register(struct ssam_controller *ctrl, int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_notifier *n); +int ssam_controller_event_enable(struct ssam_controller *ctrl, + struct ssam_event_registry reg, + struct ssam_event_id id, u8 flags); + +int ssam_controller_event_disable(struct ssam_controller *ctrl, + struct ssam_event_registry reg, + struct ssam_event_id id, u8 flags); + #endif /* _LINUX_SURFACE_AGGREGATOR_CONTROLLER_H */
We can already enable and disable SAM events via one of two ways: either via a (non-observer) notifier tied to a specific event group, or a generic event enable/disable request. In some instances, however, neither method may be desirable. The first method will tie the event enable request to a specific notifier, however, when we want to receive notifications for multiple event groups of the same target category and forward this to the same notifier callback, we may receive duplicate events, i.e. one event per registered notifier. The second method will bypass the internal reference counting mechanism, meaning that a disable request will disable the event regardless of any other client driver using it, which may break the functionality of that driver. To address this problem, add new functions that allow enabling and disabling of events via the event reference counting mechanism built into the controller, without needing to register a notifier. This can then be used in combination with observer notifiers to process multiple events of the same target category without duplication in the same callback function. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- .../platform/surface/aggregator/controller.c | 135 ++++++++++++++++++ include/linux/surface_aggregator/controller.h | 8 ++ 2 files changed, 143 insertions(+)