Re: [PATCH v3 02/24] firmware: arm_scmi: Reduce the scope of protocols mutex

[Usama Arif <usama.arif@linux.dev>]

On Sun, 29 Mar 2026 17:33:13 +0100 Cristian Marussi <cristian.marussi@arm.com> wrote:

> Currently the mutex dedicated to the protection of the list of registered
> protocols is held during all the protocol initialization phase.
> 
> Such a wide locking region is not needed and causes problem when trying to
> initialize notifications from within a protocol initialization routine.
> 
> Reduce the scope of the protocol mutex.

I think this changes more than the mutex scope. scmi_get_protocol_instance()
can now drop protocols_mtx after idr_find() while scmi_protocol_release()
can concurrently drop the final reference, remove the IDR entry, and release
the devres group. Does that leaves a use-after-free window around the returned
pi?


> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v1-->v2
>  - Fixed improper mixed usage of cleanup and goto constructs
> ---
>  drivers/firmware/arm_scmi/driver.c | 50 ++++++++++++++----------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 3e76a3204ba4..26f192b8d7a9 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -17,6 +17,7 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/bitmap.h>
> +#include <linux/cleanup.h>
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> @@ -2190,7 +2191,6 @@ static void scmi_protocol_version_initialize(struct device *dev,
>   * all resources management is handled via a dedicated per-protocol devres
>   * group.
>   *
> - * Context: Assumes to be called with @protocols_mtx already acquired.
>   * Return: A reference to a freshly allocated and initialized protocol instance
>   *	   or ERR_PTR on failure. On failure the @proto reference is at first
>   *	   put using @scmi_protocol_put() before releasing all the devres group.
> @@ -2236,8 +2236,10 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  	if (ret)
>  		goto clean;
>  
> -	ret = idr_alloc(&info->protocols, pi, proto->id, proto->id + 1,
> -			GFP_KERNEL);
> +	/* Finally register the initialized protocol */
> +	mutex_lock(&info->protocols_mtx);
> +	ret = idr_alloc(&info->protocols, pi, proto->id, proto->id + 1, GFP_KERNEL);
> +	mutex_unlock(&info->protocols_mtx);
>  	if (ret != proto->id)
>  		goto clean;
>  
> @@ -2284,27 +2286,25 @@ 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);
> +	const struct scmi_protocol *proto;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> -
> -	if (pi) {
> -		refcount_inc(&pi->users);
> -	} else {
> -		const struct scmi_protocol *proto;
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		struct scmi_protocol_instance *pi;
>  
> -		/* 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);
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (pi) {
> +			refcount_inc(&pi->users);
> +			return pi;
> +		}
>  	}
> -	mutex_unlock(&info->protocols_mtx);
>  
> -	return pi;
> +	/* Fails if protocol not registered on bus */
> +	proto = scmi_protocol_get(protocol_id, &info->version);
> +	if (!proto)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return scmi_alloc_init_protocol_instance(info, proto);
>  }
>  
>  /**
> @@ -2335,10 +2335,11 @@ 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;
>  
> -	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;
> +	}
>  
>  	if (refcount_dec_and_test(&pi->users)) {
>  		void *gid = pi->gid;
> @@ -2357,9 +2358,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);
>  }
>  
>  void scmi_setup_protocol_implemented(const struct scmi_protocol_handle *ph,
> -- 
> 2.53.0
> 
>