| Message ID | 20210604134755.535590-3-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/surface: aggregator: Extend user-space interface for events | expand |
Hi, On 6/4/21 3:47 PM, Maximilian Luz wrote: > 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> > --- > > Changes in v2: > - Remove unused variable > - Avoid code duplication > > --- > .../platform/surface/aggregator/controller.c | 293 +++++++++++++++--- > include/linux/surface_aggregator/controller.h | 8 + > 2 files changed, 253 insertions(+), 48 deletions(-) > > diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c > index cd3a6b77f48d..c673aa8309c8 100644 > --- a/drivers/platform/surface/aggregator/controller.c > +++ b/drivers/platform/surface/aggregator/controller.c > @@ -407,6 +407,31 @@ ssam_nf_refcount_dec(struct ssam_nf *nf, struct ssam_event_registry reg, > return NULL; > } > > +/** > + * ssam_nf_refcount_dec_free() - Decrement reference-/activation-count of the > + * given event and free its entry if the reference count reaches zero. > + * @nf: The notifier system reference. > + * @reg: The registry used to enable/disable the event. > + * @id: The event ID. > + * > + * Decrements the reference-/activation-count of the specified event, freeing > + * its entry if it reaches zero. > + * > + * Note: ``nf->lock`` must be held when calling this function. > + */ > +static void ssam_nf_refcount_dec_free(struct ssam_nf *nf, > + struct ssam_event_registry reg, > + struct ssam_event_id id) > +{ > + struct ssam_nf_refcount_entry *entry; > + > + lockdep_assert_held(&nf->lock); > + > + entry = ssam_nf_refcount_dec(nf, reg, id); > + if (entry && entry->refcount == 0) > + kfree(entry); > +} > + > /** > * ssam_nf_refcount_empty() - Test if the notification system has any > * enabled/active events. > @@ -2122,6 +2147,109 @@ int ssam_ctrl_notif_d0_entry(struct ssam_controller *ctrl) > > /* -- Top-level event registry interface. ----------------------------------- */ > > +/** > + * ssam_nf_refcount_enable() - Enable event for reference count entry if it has > + * not already been enabled. > + * @ctrl: The controller to enable the event on. > + * @entry: The reference count entry for the event to be enabled. > + * @flags: The flags used for enabling the event on the EC. > + * > + * Enable the event associated with the given reference count entry if the > + * reference count equals one, i.e. the event has not previously been enabled. > + * If the event has already been enabled (i.e. reference count not equal to > + * one), check that the flags used for enabling match and warn about this if > + * they do not. > + * > + * This does not modify the reference count itself, which is done with > + * ssam_nf_refcount_inc() / ssam_nf_refcount_dec(). > + * > + * Note: ``nf->lock`` must be held when calling this function. > + * > + * Return: Returns zero on success. If the event is enabled by this call, > + * returns the status of the event-enable EC command. > + */ > +static int ssam_nf_refcount_enable(struct ssam_controller *ctrl, > + struct ssam_nf_refcount_entry *entry, u8 flags) > +{ > + const struct ssam_event_registry reg = entry->key.reg; > + const struct ssam_event_id id = entry->key.id; > + struct ssam_nf *nf = &ctrl->cplt.event.notif; > + int status; > + > + lockdep_assert_held(&nf->lock); > + > + 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) > + 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); > + } > + > + return 0; > +} > + > +/** > + * ssam_nf_refcount_enable() - Disable event for reference count entry if it is s/ssam_nf_refcount_enable/ssam_nf_refcount_disable_free/ No need to resend, I'll fix this up when merging this series. Regards, Hans > + * no longer in use and free the corresponding entry. > + * @ctrl: The controller to disable the event on. > + * @entry: The reference count entry for the event to be disabled. > + * @flags: The flags used for enabling the event on the EC. > + * > + * If the reference count equals zero, i.e. the event is no longer requested by > + * any client, the event will be disabled and the corresponding reference count > + * entry freed. The reference count entry must not be used any more after a > + * call to this function. > + * > + * Also checks if the flags used for disabling the event match the flags used > + * for enabling the event and warns if they do not (regardless of reference > + * count). > + * > + * This does not modify the reference count itself, which is done with > + * ssam_nf_refcount_inc() / ssam_nf_refcount_dec(). > + * > + * Note: ``nf->lock`` must be held when calling this function. > + * > + * Return: Returns zero on success. If the event is disabled by this call, > + * returns the status of the event-enable EC command. > + */ > +static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl, > + struct ssam_nf_refcount_entry *entry, u8 flags) > +{ > + const struct ssam_event_registry reg = entry->key.reg; > + const struct ssam_event_id id = entry->key.id; > + struct ssam_nf *nf = &ctrl->cplt.event.notif; > + int status; > + > + lockdep_assert_held(&nf->lock); > + > + 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); > + } > + > + return status; > +} > + > /** > * ssam_notifier_register() - Register an event notifier. > * @ctrl: The controller to register the notifier on. > @@ -2166,41 +2294,26 @@ int ssam_notifier_register(struct ssam_controller *ctrl, struct ssam_event_notif > mutex_unlock(&nf->lock); > return PTR_ERR(entry); > } > - > - ssam_dbg(ctrl, "enabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", > - n->event.reg.target_category, n->event.id.target_category, > - n->event.id.instance, entry->refcount); > } > > status = ssam_nfblk_insert(nf_head, &n->base); > if (status) { > - if (entry) { > - entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); > - if (entry->refcount == 0) > - kfree(entry); > - } > + if (entry) > + ssam_nf_refcount_dec_free(nf, n->event.reg, n->event.id); > > mutex_unlock(&nf->lock); > return status; > } > > - if (entry && entry->refcount == 1) { > - status = ssam_ssh_event_enable(ctrl, n->event.reg, n->event.id, n->event.flags); > + if (entry) { > + status = ssam_nf_refcount_enable(ctrl, entry, n->event.flags); > if (status) { > ssam_nfblk_remove(&n->base); > - kfree(ssam_nf_refcount_dec(nf, n->event.reg, n->event.id)); > + ssam_nf_refcount_dec_free(nf, n->event.reg, n->event.id); > mutex_unlock(&nf->lock); > synchronize_srcu(&nf_head->srcu); > return status; > } > - > - entry->flags = n->event.flags; > - > - } else if (entry && entry->flags != n->event.flags) { > - ssam_warn(ctrl, > - "inconsistent flags when enabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", > - n->event.flags, entry->flags, n->event.reg.target_category, > - n->event.id.target_category, n->event.id.instance); > } > > mutex_unlock(&nf->lock); > @@ -2247,35 +2360,20 @@ int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_not > * If this is an observer notifier, do not attempt to disable the > * event, just remove it. > */ > - if (n->flags & SSAM_EVENT_NOTIFIER_OBSERVER) > - goto remove; > - > - entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); > - if (WARN_ON(!entry)) { > - /* > - * If this does not return an entry, there's a logic error > - * somewhere: The notifier block is registered, but the event > - * refcount entry is not there. Remove the notifier block > - * anyways. > - */ > - status = -ENOENT; > - goto remove; > - } > - > - ssam_dbg(ctrl, "disabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", > - n->event.reg.target_category, n->event.id.target_category, > - n->event.id.instance, entry->refcount); > - > - if (entry->flags != n->event.flags) { > - ssam_warn(ctrl, > - "inconsistent flags when disabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", > - n->event.flags, entry->flags, n->event.reg.target_category, > - n->event.id.target_category, n->event.id.instance); > - } > + if (!(n->flags & SSAM_EVENT_NOTIFIER_OBSERVER)) { > + entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); > + if (WARN_ON(!entry)) { > + /* > + * If this does not return an entry, there's a logic > + * error somewhere: The notifier block is registered, > + * but the event refcount entry is not there. Remove > + * the notifier block anyways. > + */ > + status = -ENOENT; > + goto remove; > + } > > - if (entry->refcount == 0) { > - status = ssam_ssh_event_disable(ctrl, n->event.reg, n->event.id, n->event.flags); > - kfree(entry); > + status = ssam_nf_refcount_disable_free(ctrl, entry, n->event.flags); > } > > remove: > @@ -2287,6 +2385,105 @@ 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 *nf = &ctrl->cplt.event.notif; > + struct ssam_nf_refcount_entry *entry; > + int status; > + > + if (!ssh_rqid_is_event(rqid)) > + return -EINVAL; > + > + mutex_lock(&nf->lock); > + > + entry = ssam_nf_refcount_inc(nf, reg, id); > + if (IS_ERR(entry)) { > + mutex_unlock(&nf->lock); > + return PTR_ERR(entry); > + } > + > + status = ssam_nf_refcount_enable(ctrl, entry, flags); > + if (status) { > + ssam_nf_refcount_dec_free(nf, reg, id); > + mutex_unlock(&nf->lock); > + return status; > + } > + > + 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 *nf = &ctrl->cplt.event.notif; > + struct ssam_nf_refcount_entry *entry; > + int status = 0; > + > + if (!ssh_rqid_is_event(rqid)) > + return -EINVAL; > + > + mutex_lock(&nf->lock); > + > + entry = ssam_nf_refcount_dec(nf, reg, id); > + if (!entry) { > + mutex_unlock(&nf->lock); > + return -ENOENT; > + } > + > + status = ssam_nf_refcount_disable_free(ctrl, entry, flags); > + > + 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 */ >
On 6/4/21 10:13 PM, Hans de Goede wrote: > Hi, > > On 6/4/21 3:47 PM, Maximilian Luz wrote: [...] >> +static int ssam_nf_refcount_enable(struct ssam_controller *ctrl, >> + struct ssam_nf_refcount_entry *entry, u8 flags) >> +{ >> + const struct ssam_event_registry reg = entry->key.reg; >> + const struct ssam_event_id id = entry->key.id; >> + struct ssam_nf *nf = &ctrl->cplt.event.notif; >> + int status; >> + >> + lockdep_assert_held(&nf->lock); >> + >> + 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) >> + 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); >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * ssam_nf_refcount_enable() - Disable event for reference count entry if it is > > s/ssam_nf_refcount_enable/ssam_nf_refcount_disable_free/ > > No need to resend, I'll fix this up when merging this series. Oh right, thanks! Regards, Max
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-20210604]
[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-215134
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/1d43dd8c1ca610c171da9a73c4122752f7cfd81d
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-215134
git checkout 1d43dd8c1ca610c171da9a73c4122752f7cfd81d
# 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:2245:6: warning: variable 'status' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (entry->refcount == 0) {
^~~~~~~~~~~~~~~~~~~~
drivers/platform/surface/aggregator/controller.c:2250:9: note: uninitialized use occurs here
return status;
^~~~~~
drivers/platform/surface/aggregator/controller.c:2245:2: note: remove the 'if' if its condition is always true
if (entry->refcount == 0) {
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/platform/surface/aggregator/controller.c:2231:12: note: initialize the variable 'status' to silence this warning
int status;
^
= 0
1 warning generated.
--
>> drivers/platform/surface/aggregator/controller.c:2227: warning: expecting prototype for ssam_nf_refcount_enable(). Prototype was for ssam_nf_refcount_disable_free() instead
vim +2245 drivers/platform/surface/aggregator/controller.c
2200
2201 /**
2202 * ssam_nf_refcount_enable() - Disable event for reference count entry if it is
2203 * no longer in use and free the corresponding entry.
2204 * @ctrl: The controller to disable the event on.
2205 * @entry: The reference count entry for the event to be disabled.
2206 * @flags: The flags used for enabling the event on the EC.
2207 *
2208 * If the reference count equals zero, i.e. the event is no longer requested by
2209 * any client, the event will be disabled and the corresponding reference count
2210 * entry freed. The reference count entry must not be used any more after a
2211 * call to this function.
2212 *
2213 * Also checks if the flags used for disabling the event match the flags used
2214 * for enabling the event and warns if they do not (regardless of reference
2215 * count).
2216 *
2217 * This does not modify the reference count itself, which is done with
2218 * ssam_nf_refcount_inc() / ssam_nf_refcount_dec().
2219 *
2220 * Note: ``nf->lock`` must be held when calling this function.
2221 *
2222 * Return: Returns zero on success. If the event is disabled by this call,
2223 * returns the status of the event-enable EC command.
2224 */
2225 static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl,
2226 struct ssam_nf_refcount_entry *entry, u8 flags)
> 2227 {
2228 const struct ssam_event_registry reg = entry->key.reg;
2229 const struct ssam_event_id id = entry->key.id;
2230 struct ssam_nf *nf = &ctrl->cplt.event.notif;
2231 int status;
2232
2233 lockdep_assert_held(&nf->lock);
2234
2235 ssam_dbg(ctrl, "disabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n",
2236 reg.target_category, id.target_category, id.instance, entry->refcount);
2237
2238 if (entry->flags != flags) {
2239 ssam_warn(ctrl,
2240 "inconsistent flags when disabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n",
2241 flags, entry->flags, reg.target_category, id.target_category,
2242 id.instance);
2243 }
2244
> 2245 if (entry->refcount == 0) {
2246 status = ssam_ssh_event_disable(ctrl, reg, id, flags);
2247 kfree(entry);
2248 }
2249
2250 return status;
2251 }
2252
---
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..c673aa8309c8 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -407,6 +407,31 @@ ssam_nf_refcount_dec(struct ssam_nf *nf, struct ssam_event_registry reg, return NULL; } +/** + * ssam_nf_refcount_dec_free() - Decrement reference-/activation-count of the + * given event and free its entry if the reference count reaches zero. + * @nf: The notifier system reference. + * @reg: The registry used to enable/disable the event. + * @id: The event ID. + * + * Decrements the reference-/activation-count of the specified event, freeing + * its entry if it reaches zero. + * + * Note: ``nf->lock`` must be held when calling this function. + */ +static void ssam_nf_refcount_dec_free(struct ssam_nf *nf, + struct ssam_event_registry reg, + struct ssam_event_id id) +{ + struct ssam_nf_refcount_entry *entry; + + lockdep_assert_held(&nf->lock); + + entry = ssam_nf_refcount_dec(nf, reg, id); + if (entry && entry->refcount == 0) + kfree(entry); +} + /** * ssam_nf_refcount_empty() - Test if the notification system has any * enabled/active events. @@ -2122,6 +2147,109 @@ int ssam_ctrl_notif_d0_entry(struct ssam_controller *ctrl) /* -- Top-level event registry interface. ----------------------------------- */ +/** + * ssam_nf_refcount_enable() - Enable event for reference count entry if it has + * not already been enabled. + * @ctrl: The controller to enable the event on. + * @entry: The reference count entry for the event to be enabled. + * @flags: The flags used for enabling the event on the EC. + * + * Enable the event associated with the given reference count entry if the + * reference count equals one, i.e. the event has not previously been enabled. + * If the event has already been enabled (i.e. reference count not equal to + * one), check that the flags used for enabling match and warn about this if + * they do not. + * + * This does not modify the reference count itself, which is done with + * ssam_nf_refcount_inc() / ssam_nf_refcount_dec(). + * + * Note: ``nf->lock`` must be held when calling this function. + * + * Return: Returns zero on success. If the event is enabled by this call, + * returns the status of the event-enable EC command. + */ +static int ssam_nf_refcount_enable(struct ssam_controller *ctrl, + struct ssam_nf_refcount_entry *entry, u8 flags) +{ + const struct ssam_event_registry reg = entry->key.reg; + const struct ssam_event_id id = entry->key.id; + struct ssam_nf *nf = &ctrl->cplt.event.notif; + int status; + + lockdep_assert_held(&nf->lock); + + 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) + 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); + } + + return 0; +} + +/** + * ssam_nf_refcount_enable() - Disable event for reference count entry if it is + * no longer in use and free the corresponding entry. + * @ctrl: The controller to disable the event on. + * @entry: The reference count entry for the event to be disabled. + * @flags: The flags used for enabling the event on the EC. + * + * If the reference count equals zero, i.e. the event is no longer requested by + * any client, the event will be disabled and the corresponding reference count + * entry freed. The reference count entry must not be used any more after a + * call to this function. + * + * Also checks if the flags used for disabling the event match the flags used + * for enabling the event and warns if they do not (regardless of reference + * count). + * + * This does not modify the reference count itself, which is done with + * ssam_nf_refcount_inc() / ssam_nf_refcount_dec(). + * + * Note: ``nf->lock`` must be held when calling this function. + * + * Return: Returns zero on success. If the event is disabled by this call, + * returns the status of the event-enable EC command. + */ +static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl, + struct ssam_nf_refcount_entry *entry, u8 flags) +{ + const struct ssam_event_registry reg = entry->key.reg; + const struct ssam_event_id id = entry->key.id; + struct ssam_nf *nf = &ctrl->cplt.event.notif; + int status; + + lockdep_assert_held(&nf->lock); + + 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); + } + + return status; +} + /** * ssam_notifier_register() - Register an event notifier. * @ctrl: The controller to register the notifier on. @@ -2166,41 +2294,26 @@ int ssam_notifier_register(struct ssam_controller *ctrl, struct ssam_event_notif mutex_unlock(&nf->lock); return PTR_ERR(entry); } - - ssam_dbg(ctrl, "enabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", - n->event.reg.target_category, n->event.id.target_category, - n->event.id.instance, entry->refcount); } status = ssam_nfblk_insert(nf_head, &n->base); if (status) { - if (entry) { - entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); - if (entry->refcount == 0) - kfree(entry); - } + if (entry) + ssam_nf_refcount_dec_free(nf, n->event.reg, n->event.id); mutex_unlock(&nf->lock); return status; } - if (entry && entry->refcount == 1) { - status = ssam_ssh_event_enable(ctrl, n->event.reg, n->event.id, n->event.flags); + if (entry) { + status = ssam_nf_refcount_enable(ctrl, entry, n->event.flags); if (status) { ssam_nfblk_remove(&n->base); - kfree(ssam_nf_refcount_dec(nf, n->event.reg, n->event.id)); + ssam_nf_refcount_dec_free(nf, n->event.reg, n->event.id); mutex_unlock(&nf->lock); synchronize_srcu(&nf_head->srcu); return status; } - - entry->flags = n->event.flags; - - } else if (entry && entry->flags != n->event.flags) { - ssam_warn(ctrl, - "inconsistent flags when enabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", - n->event.flags, entry->flags, n->event.reg.target_category, - n->event.id.target_category, n->event.id.instance); } mutex_unlock(&nf->lock); @@ -2247,35 +2360,20 @@ int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_not * If this is an observer notifier, do not attempt to disable the * event, just remove it. */ - if (n->flags & SSAM_EVENT_NOTIFIER_OBSERVER) - goto remove; - - entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); - if (WARN_ON(!entry)) { - /* - * If this does not return an entry, there's a logic error - * somewhere: The notifier block is registered, but the event - * refcount entry is not there. Remove the notifier block - * anyways. - */ - status = -ENOENT; - goto remove; - } - - ssam_dbg(ctrl, "disabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n", - n->event.reg.target_category, n->event.id.target_category, - n->event.id.instance, entry->refcount); - - if (entry->flags != n->event.flags) { - ssam_warn(ctrl, - "inconsistent flags when disabling event: got %#04x, expected %#04x (reg: %#04x, tc: %#04x, iid: %#04x)\n", - n->event.flags, entry->flags, n->event.reg.target_category, - n->event.id.target_category, n->event.id.instance); - } + if (!(n->flags & SSAM_EVENT_NOTIFIER_OBSERVER)) { + entry = ssam_nf_refcount_dec(nf, n->event.reg, n->event.id); + if (WARN_ON(!entry)) { + /* + * If this does not return an entry, there's a logic + * error somewhere: The notifier block is registered, + * but the event refcount entry is not there. Remove + * the notifier block anyways. + */ + status = -ENOENT; + goto remove; + } - if (entry->refcount == 0) { - status = ssam_ssh_event_disable(ctrl, n->event.reg, n->event.id, n->event.flags); - kfree(entry); + status = ssam_nf_refcount_disable_free(ctrl, entry, n->event.flags); } remove: @@ -2287,6 +2385,105 @@ 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 *nf = &ctrl->cplt.event.notif; + struct ssam_nf_refcount_entry *entry; + int status; + + if (!ssh_rqid_is_event(rqid)) + return -EINVAL; + + mutex_lock(&nf->lock); + + entry = ssam_nf_refcount_inc(nf, reg, id); + if (IS_ERR(entry)) { + mutex_unlock(&nf->lock); + return PTR_ERR(entry); + } + + status = ssam_nf_refcount_enable(ctrl, entry, flags); + if (status) { + ssam_nf_refcount_dec_free(nf, reg, id); + mutex_unlock(&nf->lock); + return status; + } + + 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 *nf = &ctrl->cplt.event.notif; + struct ssam_nf_refcount_entry *entry; + int status = 0; + + if (!ssh_rqid_is_event(rqid)) + return -EINVAL; + + mutex_lock(&nf->lock); + + entry = ssam_nf_refcount_dec(nf, reg, id); + if (!entry) { + mutex_unlock(&nf->lock); + return -ENOENT; + } + + status = ssam_nf_refcount_disable_free(ctrl, entry, flags); + + 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> --- Changes in v2: - Remove unused variable - Avoid code duplication --- .../platform/surface/aggregator/controller.c | 293 +++++++++++++++--- include/linux/surface_aggregator/controller.h | 8 + 2 files changed, 253 insertions(+), 48 deletions(-)