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@xxxxxxxxx>
> ---
>
> 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 */
>
