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>
---
 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) {
+		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) {
+			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)
+		pi = scmi_alloc_init_protocol_instance(info, proto);
 
 	return pi;
 }
@@ -2294,10 +2295,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;
@@ -2316,9 +2318,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.51.0

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;
>  }

On Fri, Oct 17, 2025 at 04:07:02PM +0100, Jonathan Cameron wrote:
> 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!)

Hi Jonathan,

thanks for having a look at this series.

> 
> > ---
> >  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.
> 

Thanks for the heads-up I will dig better into cleanup.h which obviously
I did not enough...my bad.

> 
> > +		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.

You are right...also the Dox comment is obsoleted..I will review this
patch as a whole, since even if probably right in its essence is badly
implemented because I rushed it in in this series to fix a probblem that
popped up on KASAN.

> 
> > +			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, protoo);

Yes this seems a better rework...

Thanks,
Cristian