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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 25 Sep 2025 21:35:46 +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.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Hi Cristian.  A few things inline and this runs into one of the things
that is dangerous to do with guard() or the other cleanup.h magic
(and documented there!)

> ---
>  drivers/firmware/arm_scmi/driver.c | 53 +++++++++++++++---------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index bd56a877fdfc..71ee25b78624 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>
> @@ -2179,10 +2180,13 @@ 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);
> -	if (ret != proto->id)
> -		goto clean;
> +	/* Finally register the initialized protocol */
> +	scoped_guard(mutex, &info->protocols_mtx) {

See the guidance in cleanup.h on mixing goto and anything defined in that file.

In some compilers, if you hit the goto above and hence jump over this
the cleanup variable will still be instantiated, but not initialized leading to
a potential attempt to unlock random memory.

Either this needs more substantial rework, or just handling the mutex with
out using guards.


> +		ret = idr_alloc(&info->protocols, pi, proto->id, proto->id + 1,
> +				GFP_KERNEL);
> +		if (ret != proto->id)
> +			goto clean;
> +	}
>  
>  	/*
>  	 * Warn but ignore events registration errors since we do not want
> @@ -2243,25 +2247,22 @@ 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_protocol_instance *pi = ERR_PTR(-EPROBE_DEFER);
>  	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;
> -
> -		/* 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);
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (pi) {

if !pi we carry on with it NULL, which is a behavior change from
before where it would be ERR_PTR(-EPROBE_DEFER);

That might not matter, but it's not 'obviously' a safe change.

> +			refcount_inc(&pi->users);
> +			return pi;
> +		}
>  	}
> -	mutex_unlock(&info->protocols_mtx);
> +
> +	/* Fails if protocol not registered on bus */
> +	proto = scmi_protocol_get(protocol_id, &info->version);
> +	if (proto)
Trivial but I'd flip the logic to
	if (!proto)
		return ERR_PTR(-EPROBE_DEFER);
assuming a NULL return as mentioned above isn't the intent.
Then
	return scmi_alloc_init_protocol_instance(info, proto);

> +		pi = scmi_alloc_init_protocol_instance(info, proto);
>  
>  	return pi;
>  }