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

On Fri, Oct 17, 2025 at 04:07:02PM +0100, Jonathan Cameron wrote:

On Thu, 25 Sep 2025 21:35:46 +0100
Cristian Marussi [off-list ref] wrote:

quoted

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.

quoted

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

quoted

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

quoted

+			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