Re: [PATCH v6 03/22] firmware: arm_scmi: Introduce protocol instance notifiers

On Fri, 24 Jul 2026 15:44:11 +0100
Cristian Marussi <[email protected]> wrote:

> Allow protocols themselves to register for their own notifications and
> provide their own notifier callbacks. Each protocol can now register one
> unique per-protocol instance notifier block whose callback will be
> registered on the proper notification chain as usual: such notifier will
> be automatically removed during the protocol de-initialiazation phase.
> 

Would be nice to say why they might do this. I'm sure it becomes
apparent later in the series, but anyone looking just this patch
is missing that useful information.

I'd also like something here to talk briefly about why it is fine to
drop the lock briefly on the unregister side.

> Signed-off-by: Cristian Marussi <[email protected]>


> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 8e06e40d1a11..0f9e8dfc6137 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
...


> @@ -2367,13 +2402,29 @@ int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
>  void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
>  {
>  	struct scmi_info *info = handle_to_scmi_info(handle);
> +	struct notifier_block *proto_notifier_nb = NULL;
>  	struct scmi_protocol_instance *pi;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> -	if (WARN_ON(!pi))
> -		goto out;
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (WARN_ON(!pi))
> +			return;
> +
> +		proto_notifier_nb = pi->pno.nb;
> +		/* Ensure NULL is visible */
> +		smp_store_mb(pi->pno.nb, NULL);
> +	}
>  

I'd like to see a little commentary on safety of not holding
the mutex over the whole sequence.  Can anything mess around
with pi between the code getting hold of it and the release below?
I assume the refcount is enough.

> +	if (proto_notifier_nb) {
> +		int ret;
> +
> +		ret = scmi_protocol_notifier_unregister(pi->handle, &pi->pno);
> +		if (ret)

Maybe this changes later but if not get rid of ret. It kind of suggests
the value itself is useful in a way that isn't true.

		if (scm_protocol_notifier_unregister(pi->handle, &pi->pno))
			dev_err();

> +			dev_err(handle->dev,
> +				"Failed to release protocol notifier\n");
> +	}
> +
> +	guard(mutex)(&info->protocols_mtx);
>  	if (refcount_dec_and_test(&pi->users)) {
>  		void *gid = pi->gid;
>  
> @@ -2391,9 +2442,6 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
>  		dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n",
>  			protocol_id);
>  	}
> -
> -out:
> -	mutex_unlock(&info->protocols_mtx);
>  }
>