Re: [PATCH v15 06/35] firmware: arm_scmi: Introduce protocol instance notifiers

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon,  9 Nov 2026 09:11:52 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> SCMI Protocol notifications are typically used by SCMI drivers to detect
> and react to particular conditions: this was the assumption and the classic
> usage scenario upon which the SCMI notification framework was built.
> 
> Some protocols like Telemetry, though, could be interested in enabling and
> registering for such notifications: in such a scenario each initialized
> protocol instance could want to register a common protocol notifier.
> 
> In order to reuse the existing SCMI Notifications machinery, allow
> protocols themselves to register for their own notifications and provide
> their own notifier callbacks.
> 
> Each protocol, at the end of its initialization phase, can now provide one
> unique per-protocol-instance notifier block, which will be registered, by
> the core SCMI stack, on the related notification chain: such notifier will
> be automatically removed during the protocol de-initialization phase.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Hi Cristian,

Perhaps it is more a reflection of Monday afternoon than anything
else, but this took a lot more work to understand than I'd expect
for something so conceptually simple.

The recursive nature of the unwind code seems overly complex
given it only ever recurses once (so the gain is minor).  To my eyes
that just makes it harder to reason about for minimal advantage
in amount of code.

A suggestion inline that perhaps a bit of refactoring will allow
a simpler linear flow.

> ---
> v13 --> v14
>  - reworked the helpers used to register protocol notifiers so as to
>    possibly worked during the protocol initialization phase when locks
>    are enabled
> v12 --> v13
>  - refactored a few IFs
> v10 -->v11
>  - added a few dev_dbg
> v8 --> v9
>  - Added membarrier comment
> v7 --> v8
>  - Reworked logic around protocol notifier registration to address
>    possible races as pointed out by Sashiko
>  - Reviewed commit message
> v5 -->v6
>  - move protocol notifier registration out of proto_init
> v2-->v3
>  - split out unrelated changes on event sizing
> v1-->v2
>  - Fixed multiline comment format
> ---
>  drivers/firmware/arm_scmi/common.h    |  24 ++++
>  drivers/firmware/arm_scmi/driver.c    | 115 +++++++++++++++----
>  drivers/firmware/arm_scmi/notify.c    | 154 ++++++++++++++++++++++++++
>  drivers/firmware/arm_scmi/protocols.h |  10 ++
>  4 files changed, 282 insertions(+), 21 deletions(-)



>  static const struct scmi_xfer_ops xfer_ops = {
>  	.xfer_get_init = xfer_get_init,
>  	.reset_rx_to_maxsz = reset_rx_to_maxsz,
> @@ -2297,6 +2318,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  	pi->ph.hops = &helpers_ops;
>  	pi->ph.set_priv = scmi_set_protocol_priv;
>  	pi->ph.get_priv = scmi_get_protocol_priv;
> +	pi->ph.instance_notifier_register = scmi_register_instance_notifier;
>  	refcount_set(&pi->users, 1);
>  
>  	/*
> @@ -2358,12 +2380,12 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  static struct scmi_protocol_instance * __must_check
>  scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
>  {
> -	struct scmi_protocol_instance *pi;
>  	struct scmi_info *info = handle_to_scmi_info(handle);
> +	struct scmi_protocol_instance *pi;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> +	guard(mutex)(&info->protocols_mtx);
>  
> +	pi = idr_find(&info->protocols, protocol_id);
>  	if (pi) {
>  		refcount_inc(&pi->users);
		return pi;
	}

Then you can drop the else.

Maybe this gets more complex in later patches but looking
just at this, it seems worth doing that to reduce the indent
of what follows.


>  	} else {
> @@ -2371,16 +2393,44 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
>  
>  		/* Fails if protocol not registered on bus */
>  		proto = scmi_protocol_get(protocol_id, &info->version);
> -		if (proto)
> -			pi = scmi_alloc_init_protocol_instance(info, proto);
> -		else
> -			pi = ERR_PTR(-EPROBE_DEFER);
> +		if (!proto)
> +			return ERR_PTR(-EPROBE_DEFER);
> +
> +		pi = scmi_alloc_init_protocol_instance(info, proto);
> +		if (IS_ERR(pi))
> +			return pi;
> +
> +		/*
> +		 * A unique protocol notifier, when present, is registered
> +		 * after the protocol itself and all of its supported events
> +		 * have been successfully initialized.
> +		 */
> +		if (smp_load_acquire(&pi->pno.nb) &&
> +		    scmi_protocol_notifier_register(pi->handle, &pi->pno))
> +			dev_warn(handle->dev,
> +				 "Failed to register protocol [0x%X] notifier\n",
> +				 proto->id);
>  	}
> -	mutex_unlock(&info->protocols_mtx);
>  
>  	return pi;
>  }
>  
> +int scmi_protocol_acquire_unlocked(const struct scmi_handle *handle, u8 protocol_id)
> +{
> +	struct scmi_info *info = handle_to_scmi_info(handle);
> +	struct scmi_protocol_instance *pi;
> +
> +	lockdep_assert_held(&info->protocols_mtx);
> +
> +	pi = idr_find(&info->protocols, protocol_id);
> +	if (WARN_ON(!pi))
> +		return -ENODEV;
> +
> +	refcount_inc(&pi->users);
> +
> +	return 0;
> +}
> +
>  /**
>   * scmi_protocol_acquire  - Protocol acquire
>   * @handle: A reference to the SCMI platform instance.
> @@ -2396,23 +2446,32 @@ int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
>  	return PTR_ERR_OR_ZERO(scmi_get_protocol_instance(handle, protocol_id));
>  }
>  
> -/**
> - * scmi_protocol_release  - Protocol de-initialization helper.
> - * @handle: A reference to the SCMI platform instance.
> - * @protocol_id: The protocol being requested.
> - *
> - * Remove one user for the specified protocol and triggers de-initialization
> - * and resources de-allocation once the last user has gone.
> - */
> -void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
> +void scmi_protocol_release_unlocked(const struct scmi_handle *handle,
> +				    u8 protocol_id)
>  {
>  	struct scmi_info *info = handle_to_scmi_info(handle);
>  	struct scmi_protocol_instance *pi;
>  
> -	mutex_lock(&info->protocols_mtx);
> +	lockdep_assert_held(&info->protocols_mtx);
> +
>  	pi = idr_find(&info->protocols, protocol_id);
>  	if (WARN_ON(!pi))
> -		goto out;
> +		return;
> +
> +	/*
> +	 * If a protocol notifier was registered and this is the last instance
> +	 * releasing the protocol, un-register the notifier: note that the
> +	 * notifier itself counts as one user, as for any other regular notifier,
> +	 * so if a protocol notifier was registered and there are only 2 users
> +	 * active we can derive that this is the last protocol instance
> +	 * de-registering.
> +	 */
> +	if (scmi_protocol_notifier_registered(&pi->pno) &&
> +	    refcount_read(&pi->users) == 2 &&
> +	    scmi_protocol_notifier_unregister(pi->handle, &pi->pno))
> +		dev_err(handle->dev,
> +			"Failed to release protocol [0x%X] notifier\n",
> +			protocol_id);
>  
>  	if (refcount_dec_and_test(&pi->users)) {
>  		void *gid = pi->gid;
> @@ -2431,9 +2490,23 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
>  		dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n",
>  			protocol_id);
>  	}
> +}

> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index fa2339209a69..6fe95a79465d 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -1061,6 +1061,44 @@ static int scmi_register_event_handler(struct scmi_notify_instance *ni,
>  	return ret;

> +
>  /**
>   * __scmi_event_handler_get_ops()  - Utility to get or create an event handler
>   * @ni: A reference to the notification instance to use
> @@ -1438,6 +1476,67 @@ static int scmi_notifier_register(const struct scmi_handle *handle,
>  	return ret;
>  }
>  
> +/**
> + * scmi_protocol_notifier_register  - Register the protocol notifier
> + * @handle: The handle identifying the platform instance against which the
> + *	    callback is registered
> + * @pno: The protocol notifier descriptor
> + *
> + * A protocol can optionally register a single protocol notifier callback
> + * during its initialization phase: the notifier_block will be the registered
> + * using the usual common SCMI notification framework.
> + *
> + * This custom wrapper function is used to register such a notifier since, by
> + * construction, when a protocol notifier is registered:
> + *  - it is the first notifier to be registered
> + *  - the related protocol and its events have been already initialized so this
> + *    notifier will never need to transition from the pending events list
> + *
> + * Return: 0 on Success
> + */
> +int scmi_protocol_notifier_register(const struct scmi_handle *handle,
> +				    struct scmi_protocol_notifier *pno)
> +{
> +	struct scmi_event_handler *hndl;
> +	struct scmi_notify_instance *ni;
> +	u32 evt_key;
> +	int ret;
> +
> +	ni = scmi_notification_instance_data_get(handle);
> +	if (!ni)
> +		return -ENODEV;
> +
> +	/* Ensure registered was visible */
> +	if (smp_load_acquire(&pno->registered))
> +		return -EBUSY;
> +
> +	evt_key = MAKE_HASH_KEY(pno->proto_id, pno->evt_id,
> +				pno->src_id ? *pno->src_id : SRC_ID_MASK);
> +
> +	/* This protocol handler notifier cannot exist already at this point */
> +	hndl = scmi_create_registered_handler(ni, evt_key);
> +	if (IS_ERR(hndl))
> +		return PTR_ERR(hndl);
> +
> +	blocking_notifier_chain_register(&hndl->chain, pno->nb);
> +	ret = scmi_event_handler_enable_events(hndl);
> +	if (ret) {
> +		/* When enable fails throw the handler away */
> +		scmi_put_handler_unlocked(ni, hndl);
> +		/* Drop protocol refcount acquired after creation */
> +		scmi_protocol_release_unlocked(handle, pno->proto_id);
> +		return ret;
> +	}
> +
> +	/* Make sure registration is visible */
> +	smp_store_release(&pno->registered, true);
> +
> +	dev_dbg(handle->dev,
> +		"Registered SCMI protocol [0x%X] notifier\n", pno->proto_id);
> +
> +	return 0;
> +}
> +
>  /**
>   * scmi_notifier_unregister()  - Unregister a notifier_block for an event
>   * @handle: The handle identifying the platform instance against which the
> @@ -1495,6 +1594,61 @@ static int scmi_notifier_unregister(const struct scmi_handle *handle,
>  	return 0;
>  }
>  
> +int scmi_protocol_notifier_unregister(const struct scmi_handle *handle,
> +				      struct scmi_protocol_notifier *pno)
> +{
> +	struct scmi_registered_event *r_evt;
> +	struct scmi_event_handler *hndl;

It is a little confusing to have handle and hndl.
Perhaps name this one ev_handler or something like that the
difference is more obvious?

> +	struct scmi_notify_instance *ni;
> +	u32 evt_key;
> +	bool freed;
> +
> +	ni = scmi_notification_instance_data_get(handle);
> +	if (!ni)
> +		return -ENODEV;
> +
> +	evt_key = MAKE_HASH_KEY(pno->proto_id, pno->evt_id,
> +				pno->src_id ? *pno->src_id : SRC_ID_MASK);
> +
> +	r_evt = SCMI_GET_REVT(ni, KEY_XTRACT_PROTO_ID(evt_key),
> +			      KEY_XTRACT_EVT_ID(evt_key));
> +
> +	if (WARN_ON(!r_evt))
> +		return -ENODEV;
> +
> +	/* Make sure de-registration is visible BEFORE calling unregister */
> +	smp_store_release(&pno->registered, false);
> +
> +	scoped_guard(mutex, &r_evt->proto->registered_mtx) {
> +		hndl = KEY_FIND(r_evt->proto->registered_events_handlers,
> +				hndl, evt_key);
> +		if (!hndl) {
> +			/* Visibly revert registered state on failure */
> +			smp_store_release(&pno->registered, true);

I assume any race in the window where it doesn't look registered doesn't
matter because there should be no path in which the key isn't found?
Perhaps add something to say that.

> +			return -ENOMEM;
> +		}
> +
> +		blocking_notifier_chain_unregister(&hndl->chain, pno->nb);
> +
> +		freed = scmi_put_handler_unlocked(ni, hndl);
> +		/* This must cause the handler to be freed at this point */
> +		WARN_ON(!freed);
> +	}
> +
> +	/*
> +	 * Releasing the protocol for this notifier only here AFTER unlocking
> +	 * registered_mtx, since releasing the protocol itself can trigger its
> +	 * de-initialization (ie. including r_evt and registered_mtx)
> +	 */
> +	if (freed)
> +		scmi_protocol_release_unlocked(handle, pno->proto_id);

This recursion makes this code hard to follow.  

I'm not 100% sure, but feels like you could add a tiny bit more code
into scm_protocol_release() to first check if there is a self registered
notifier and drop the recursive bit. 

Something like the following.

void scmi_protocol_do_release(struct scmi_info *info, struct scmi_protocol_instance *pi) 
{
	void *gid = pi->gid;

	if (pi->proto->events)
		scmi_deregister_protocol_events(handle, protocol_id);

	if (pi->proto->instance_deinit)
		pi->proto->instance_deinit(&pi->ph);

	idr_remove(&info->protocols, protocol_id);

	scmi_protocol_put(pi->proto);

	devres_release_group(handle->dev, gid);
	dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n",	protocol_id);
}

int scmi_protocol_notifier_unregister(const struct scmi_handle *handle,
				      struct scmi_protocol_notifier *pno)
{
	struct scmi_registered_event *r_evt;
	struct scmi_event_handler *hndl;
	struct scmi_notify_instance *ni;
	u32 evt_key;
	bool freed;

	ni = scmi_notification_instance_data_get(handle);
	if (!ni)
		return -ENODEV;

	evt_key = MAKE_HASH_KEY(pno->proto_id, pno->evt_id,
				pno->src_id ? *pno->src_id : SRC_ID_MASK);

	r_evt = SCMI_GET_REVT(ni, KEY_XTRACT_PROTO_ID(evt_key),
			      KEY_XTRACT_EVT_ID(evt_key));

	if (WARN_ON(!r_evt))
		return -ENODEV;

	/* Make sure de-registration is visible BEFORE calling unregister */
	smp_store_release(&pno->registered, false);

	scoped_guard(mutex, &r_evt->proto->registered_mtx) {
		hndl = KEY_FIND(r_evt->proto->registered_events_handlers,
				hndl, evt_key);
		if (!hndl) {
			/* Visibly revert registered state on failure */
			smp_store_release(&pno->registered, true);
			return -ENOMEM;
		}

		blocking_notifier_chain_unregister(&hndl->chain, pno->nb);

		freed = scmi_put_handler_unlocked(ni, hndl);
		/* This must cause the handler to be freed at this point */
		WARN_ON(!freed);
	}

	/*
	 * Releasing the protocol for this notifier only here AFTER unlocking
	 * registered_mtx, since releasing the protocol itself can trigger its
	 * de-initialization (ie. including r_evt and registered_mtx)
	 */
//	if (freed)
//		scmi_protocol_release_unlocked(handle, pno->proto_id);

	dev_dbg(handle->dev,
		"Unregistered SCMI protocol [0x%X] notifier\n", pno->proto_id);

	return 0;
}

void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
{
	struct scmi_info *info = handle_to_scmi_info(handle);
	struct scmi_protocol_instance *pi;

	guard(mutex)(&info->protocols_mtx);
	pi = idr_find(&info->protocols, protocol_id);
 	if (WARN_ON(!pi))
		return;

	// Simple case, it was the last one - which inherently means no
	// self notifier bit.
 	if (refcount_dec_and_test(&pi->users)) {
		scmi_protocol_do_release(info, pi);
		return;
	}

	// The self notifier bit.
	if (scmi_protocol_notifier_registered(&pi->pno) &&
	    refcount_read(&pi->users) == 1 &&
	    refcount_dec_and_test(&pi->users)) {
	//last bit can be an unconditional dec.
		int ret = scmi_protocol_notifier_unregister(pi->handle,  &pi->pn);
		if (ret)
			dev_err();
		scmi_protocol_do_release(info, pi);
	}
}
		
> +
> +	dev_dbg(handle->dev,
> +		"Unregistered SCMI protocol [0x%X] notifier\n", pno->proto_id);
> +
> +	return 0;
> +}