[PATCH 1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids

[PATCH 1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids

[Sudeep Holla]

Currently in scmi_protocol_device_request(), no duplicate scmi device
name is allowed across any protocol. However scmi_dev_match_id() first
matches the protocol id and then the name. So, there is no strict
requirement to keep this scmi device name unique across all the protocols.

Relax the constraint on the duplicate name across the protocols and
inhibit only within the same protocol id.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index a3386bf36de5..8acf33ccfd21 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -42,7 +42,7 @@ static atomic_t scmi_syspower_registered = ATOMIC_INIT(0);
  * This helper let an SCMI driver request specific devices identified by the
  * @id_table to be created for each active SCMI instance.
  *
- * The requested device name MUST NOT be already existent for any protocol;
+ * The requested device name MUST NOT be already existent for this protocol;
  * at first the freshly requested @id_table is annotated in the IDR table
  * @scmi_requested_devices and then the requested device is advertised to any
  * registered party via the @scmi_requested_devices_nh notification chain.
@@ -52,7 +52,6 @@ static atomic_t scmi_syspower_registered = ATOMIC_INIT(0);
 static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
 {
 	int ret = 0;
-	unsigned int id = 0;
 	struct list_head *head, *phead = NULL;
 	struct scmi_requested_dev *rdev;
 
@@ -67,19 +66,13 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
 	}
 
 	/*
-	 * Search for the matching protocol rdev list and then search
-	 * of any existent equally named device...fails if any duplicate found.
+	 * Find the matching protocol rdev list and then search of any
+	 * existent equally named device...fails if any duplicate found.
 	 */
 	mutex_lock(&scmi_requested_devices_mtx);
-	idr_for_each_entry(&scmi_requested_devices, head, id) {
-		if (!phead) {
-			/* A list found registered in the IDR is never empty */
-			rdev = list_first_entry(head, struct scmi_requested_dev,
-						node);
-			if (rdev->id_table->protocol_id ==
-			    id_table->protocol_id)
-				phead = head;
-		}
+	phead = idr_find(&scmi_requested_devices, id_table->protocol_id);
+	if (phead) {
+		head = phead;
 		list_for_each_entry(rdev, head, node) {
 			if (!strcmp(rdev->id_table->name, id_table->name)) {
 				pr_err("Ignoring duplicate request [%d] %s\n",
-- 
2.34.1

[PATCH 2/3] firmware: arm_scmi: Add name and protocol id attributes

[Sudeep Holla]

Add the name and the protocol id attributes to the SCMI devices on the
bus so that they are exposed to the user-space via the sysfs.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 38 +++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 8acf33ccfd21..862dd34eca62 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -17,6 +17,8 @@
 
 #include "common.h"
 
+#define SCMI_UEVENT_MODALIAS_FMT	"%s:%02x:%s"
+
 BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
 EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
 
@@ -276,11 +278,47 @@ static void scmi_dev_remove(struct device *dev)
 		scmi_drv->remove(scmi_dev);
 }
 
+static int scmi_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	const struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return add_uevent_var(env, "MODALIAS=" SCMI_UEVENT_MODALIAS_FMT,
+			      dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
+			      scmi_dev->name);
+}
+
+static ssize_t protocol_id_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return sprintf(buf, "0x%02x\n", scmi_dev->protocol_id);
+}
+static DEVICE_ATTR_RO(protocol_id);
+
+static ssize_t name_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return sprintf(buf, "%s\n", scmi_dev->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static struct attribute *scmi_device_attributes_attrs[] = {
+	&dev_attr_protocol_id.attr,
+	&dev_attr_name.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(scmi_device_attributes);
+
 const struct bus_type scmi_bus_type = {
 	.name =	"scmi_protocol",
 	.match = scmi_dev_match,
 	.probe = scmi_dev_probe,
 	.remove = scmi_dev_remove,
+	.uevent	= scmi_device_uevent,
+	.dev_groups = scmi_device_attributes_groups,
 };
 EXPORT_SYMBOL_GPL(scmi_bus_type);
 
-- 
2.34.1

Re: [PATCH 2/3] firmware: arm_scmi: Add name and protocol id attributes

[Dhruva Gole]

On Jan 31, 2025 at 14:18:21 +0000, Sudeep Holla wrote:
> Add the name and the protocol id attributes to the SCMI devices on the
> bus so that they are exposed to the user-space via the sysfs.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 38 +++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 

Reviewed-by: Dhruva Gole <d-gole@ti.com>

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

[PATCH 3/3] firmware: arm_scmi: Emit modalias for SCMI devices

[Sudeep Holla]

In order to enable libkmod lookups for SCMI device objects to their
corresponding module, add 'modalias' to the base attribute of SCMI
devices.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 862dd34eca62..7af01664ce7e 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -287,6 +287,17 @@ static int scmi_device_uevent(const struct device *dev, struct kobj_uevent_env *
 			      scmi_dev->name);
 }
 
+static ssize_t modalias_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return sysfs_emit(buf, SCMI_UEVENT_MODALIAS_FMT,
+			  dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
+			  scmi_dev->name);
+}
+static DEVICE_ATTR_RO(modalias);
+
 static ssize_t protocol_id_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
@@ -308,6 +319,7 @@ static DEVICE_ATTR_RO(name);
 static struct attribute *scmi_device_attributes_attrs[] = {
 	&dev_attr_protocol_id.attr,
 	&dev_attr_name.attr,
+	&dev_attr_modalias.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(scmi_device_attributes);
-- 
2.34.1

Re: [PATCH 3/3] firmware: arm_scmi: Emit modalias for SCMI devices

[Cristian Marussi]

On Fri, Jan 31, 2025 at 02:18:22PM +0000, Sudeep Holla wrote:
> In order to enable libkmod lookups for SCMI device objects to their
> corresponding module, add 'modalias' to the base attribute of SCMI
> devices.
> 

Hi Sudeep,

Indeed, as of today if you build the SCMI stack completely as modules, only the
core stack (scmi-core/scmi-module) and the needed SCMI transports are loaded
automatically, so this patch, I understand, is meant to add the missing bits
to enable autoloading also for the SCMI drivers that sits on top like
scmi-cpufreq etc..

In fact, I tried experimenting with something similar to your solution
recently to enable MODALIAS emission and full stack autoloading, BUT then
I realized it could have worked ONLY with an additional hack...

...the problem lays in the fact that since we wanted to allow Vendor
drivers and protocols to be easily added without changing the core (as
asked by vendors themslves AFAICR), we dont have a central static
devices-table hold by the SCMI bus core (as usual) but instead we let
SCMI drivers dynamically request protocol/devices that are needed...

...and this in turn means that the devices are created only when a driver
requires a specifc protocol/name pair (and a matching DT entry is found)
during its initalization at load time...so it's a chicken-egg problem
since the MODALIAS that will cause, say, scmi-cpufreq to be loaded will
be emitted only when the device for scmi-cpufeq will be created BUT that
currently happens only if the device was requested during the init/loading
of scmi-cpufreq itself...

...the dynamic request-device mechanism added to give vendors flexibility
does break the usual module autoloading capabilities AFAIU...

...indeed I tested this series with a full modularized SCMI stack and it
does NOT cause the full stack to be loaded...

Having said that, since this dynamic-request device mechanism was meant
to ease vendors additions, my hack was simply to pre-request all the
standard protocol devices at bus init...in that way the full stack does
finally autoload...I will post that 2 patches of mine as and RFC in
response to this for the sake of clarity...

Note that such a hack, or a better polished version of it, would NOT work
anyway for Vendor modules: if we want to keep such flexibility they still
have to be loaded manually...

Alternatively we could get rid of dynamic device creation as a whole and
revert to a more standard static central devicetable for all...not sure
really if the flexibility of device creation at runtime without keeping
a common central table is really needed/appreciated by the Vendors that
asked for it in first place :D

Thanks,
Cristian

[RFC PATCH 1/2] firmware: arm_scmi: Generate aliases for SCMI modules

[Cristian Marussi]

Generate aliases for SCMI modules to allow automatic module probing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/mod_devicetable.h   | 6 ++++++
 include/linux/scmi_protocol.h     | 6 +-----
 scripts/mod/devicetable-offsets.c | 3 +++
 scripts/mod/file2alias.c          | 9 +++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4338b1b4ac44..90c1f7bd4d1e 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -972,4 +972,10 @@ struct coreboot_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/* SCMI Devices */
+struct scmi_device_id {
+	__u8 protocol_id;
+	const char *name;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5437275d3ac7..3d5c4b5a7dd8 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/hashtable.h>
 #include <linux/log2.h>
+#include <linux/mod_devicetable.h>
 #include <linux/notifier.h>
 #include <linux/types.h>
 #include <linux/xarray.h>
@@ -1196,11 +1197,6 @@ struct scmi_device {
 
 #define to_scmi_dev(d) container_of_const(d, struct scmi_device, dev)
 
-struct scmi_device_id {
-	u8 protocol_id;
-	const char *name;
-};
-
 struct scmi_driver {
 	const char *name;
 	int (*probe)(struct scmi_device *sdev);
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 9c7b404defbd..58d614109fae 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -281,5 +281,8 @@ int main(void)
 	DEVID(coreboot_device_id);
 	DEVID_FIELD(coreboot_device_id, tag);
 
+	DEVID(scmi_device_id);
+	DEVID_FIELD(scmi_device_id, protocol_id);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 19ec72a69e90..f86f194a1711 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -510,6 +510,14 @@ static void do_css_entry(struct module *mod, void *symval)
 	module_alias_printf(mod, false, "css:t%01X", type);
 }
 
+/* looks like: "scmi:prN" */
+static void do_scmi_entry(struct module *mod, void *symval)
+{
+	DEF_FIELD(symval, scmi_device_id, protocol_id);
+
+	module_alias_printf(mod, true, "scmi:pr%02X", protocol_id);
+}
+
 /* Looks like: "serio:tyNprNidNexN" */
 static void do_serio_entry(struct module *mod, void *symval)
 {
@@ -1471,6 +1479,7 @@ static const struct devtable devtable[] = {
 	{"usb", SIZE_usb_device_id, do_usb_entry_multi},
 	{"pnp", SIZE_pnp_device_id, do_pnp_device_entry},
 	{"pnp_card", SIZE_pnp_card_device_id, do_pnp_card_entry},
+	{"scmi", SIZE_scmi_device_id, do_scmi_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.47.0

[RFC PATCH 2/2] firmware: arm_scmi: Add bus support for autoloading

[Cristian Marussi]

Emit proper MODALIAS uevents when SCMI devices are created and make sure
all the standard protocol devices are requested when the bus is
initialized.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 52 +++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index def41fe3b161..dfa4fde2a81c 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -92,12 +92,17 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
 				phead = head;
 		}
 		list_for_each_entry(rdev, head, node) {
+			/*
+			 * HACK
+			 * We ignore requested duplicates since they could have
+			 * been pre-requested...
+			 */
 			if (!strcmp(rdev->id_table->name, id_table->name)) {
-				pr_err("Ignoring duplicate request [%d] %s\n",
-				       rdev->id_table->protocol_id,
-				       rdev->id_table->name);
-				ret = -EINVAL;
-				goto out;
+				pr_debug("Device already requested [%d] %s\n",
+				        rdev->id_table->protocol_id,
+				        rdev->id_table->name);
+				mutex_unlock(&scmi_requested_devices_mtx);
+				return 0;
 			}
 		}
 	}
@@ -274,6 +279,16 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
 	return to_scmi_dev(dev);
 }
 
+static int scmi_dev_uevent(const struct device *dev,
+			   struct kobj_uevent_env *env)
+{
+	const struct scmi_device *sdev = to_scmi_dev(dev);
+
+	add_uevent_var(env, "MODALIAS=scmi:pr%02X", sdev->protocol_id);
+
+	return 0;
+}
+
 static int scmi_dev_probe(struct device *dev)
 {
 	struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver);
@@ -297,6 +312,7 @@ static void scmi_dev_remove(struct device *dev)
 const struct bus_type scmi_bus_type = {
 	.name =	"scmi_protocol",
 	.match = scmi_dev_match,
+	.uevent	= scmi_dev_uevent,
 	.probe = scmi_dev_probe,
 	.remove = scmi_dev_remove,
 };
@@ -516,6 +532,23 @@ static void scmi_devices_unregister(void)
 	bus_for_each_dev(&scmi_bus_type, NULL, NULL, __scmi_devices_unregister);
 }
 
+/* Standard protocols table */
+static const struct scmi_device_id scmi_std_id_table[] = {
+	{ SCMI_PROTOCOL_POWER, "genpd" },
+	{ SCMI_PROTOCOL_SYSTEM, "syspower" },
+	{ SCMI_PROTOCOL_PERF, "perf" },
+	{ SCMI_PROTOCOL_PERF, "cpufreq" },
+	{ SCMI_PROTOCOL_CLOCK, "clocks" },
+	{ SCMI_PROTOCOL_SENSOR, "hwmon" },
+	{ SCMI_PROTOCOL_SENSOR, "iiodev" },
+	{ SCMI_PROTOCOL_RESET, "reset" },
+	{ SCMI_PROTOCOL_VOLTAGE,  "regulator" },
+	{ SCMI_PROTOCOL_POWERCAP, "powercap" },
+	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+	{ SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" },
+	{ },
+};
+
 static int __init scmi_bus_init(void)
 {
 	int retval;
@@ -526,6 +559,15 @@ static int __init scmi_bus_init(void)
 
 	pr_info("SCMI protocol bus registered\n");
 
+	/* HACK pre-request ALL standard protocol devices */
+	retval = scmi_protocol_table_register(scmi_std_id_table);
+	if (retval) {
+		bus_unregister(&scmi_bus_type);
+		return retval;
+	}
+
+	pr_info("SCMI standard protocols devices requested\n");
+
 	return retval;
 }
 subsys_initcall(scmi_bus_init);
-- 
2.47.0

RE: [PATCH 1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids

[Peng Fan]

> Subject: [PATCH 1/3] firmware: arm_scmi: Relax duplicate name
> constraint across protocol ids
> 
> Currently in scmi_protocol_device_request(), no duplicate scmi device
> name is allowed across any protocol. However scmi_dev_match_id()
> first matches the protocol id and then the name. So, there is no strict
> requirement to keep this scmi device name unique across all the
> protocols.
> 
> Relax the constraint on the duplicate name across the protocols and
> inhibit only within the same protocol id.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids

[Dhruva Gole]

Hi Sudeep,

On Jan 31, 2025 at 14:18:20 +0000, Sudeep Holla wrote:
> Currently in scmi_protocol_device_request(), no duplicate scmi device
> name is allowed across any protocol. However scmi_dev_match_id() first

Yeah this was indeed a bit too stringent.

> matches the protocol id and then the name. So, there is no strict
> requirement to keep this scmi device name unique across all the protocols.

Right.

> 
> Relax the constraint on the duplicate name across the protocols and
> inhibit only within the same protocol id.

Makes sense to me, it will indeed make things simpler.
Reviewed-by: Dhruva Gole <d-gole@ti.com>

> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index a3386bf36de5..8acf33ccfd21 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -42,7 +42,7 @@ static atomic_t scmi_syspower_registered = ATOMIC_INIT(0);
>   * This helper let an SCMI driver request specific devices identified by the
>   * @id_table to be created for each active SCMI instance.
>   *
> - * The requested device name MUST NOT be already existent for any protocol;
> + * The requested device name MUST NOT be already existent for this protocol;
>   * at first the freshly requested @id_table is annotated in the IDR table
>   * @scmi_requested_devices and then the requested device is advertised to any
>   * registered party via the @scmi_requested_devices_nh notification chain.
> @@ -52,7 +52,6 @@ static atomic_t scmi_syspower_registered = ATOMIC_INIT(0);
>  static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
>  {
>  	int ret = 0;
> -	unsigned int id = 0;
>  	struct list_head *head, *phead = NULL;
>  	struct scmi_requested_dev *rdev;
>  
> @@ -67,19 +66,13 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
>  	}
>  
>  	/*
> -	 * Search for the matching protocol rdev list and then search
> -	 * of any existent equally named device...fails if any duplicate found.
> +	 * Find the matching protocol rdev list and then search of any
> +	 * existent equally named device...fails if any duplicate found.
>  	 */
>  	mutex_lock(&scmi_requested_devices_mtx);
> -	idr_for_each_entry(&scmi_requested_devices, head, id) {
> -		if (!phead) {
> -			/* A list found registered in the IDR is never empty */
> -			rdev = list_first_entry(head, struct scmi_requested_dev,
> -						node);
> -			if (rdev->id_table->protocol_id ==
> -			    id_table->protocol_id)
> -				phead = head;
> -		}
> +	phead = idr_find(&scmi_requested_devices, id_table->protocol_id);
> +	if (phead) {
> +		head = phead;
>  		list_for_each_entry(rdev, head, node) {
>  			if (!strcmp(rdev->id_table->name, id_table->name)) {
>  				pr_err("Ignoring duplicate request [%d] %s\n",
> -- 
> 2.34.1
> 
> 

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

Re: [PATCH 1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids

[Sudeep Holla]

On Fri, 31 Jan 2025 14:18:20 +0000, Sudeep Holla wrote:
> Currently in scmi_protocol_device_request(), no duplicate scmi device
> name is allowed across any protocol. However scmi_dev_match_id() first
> matches the protocol id and then the name. So, there is no strict
> requirement to keep this scmi device name unique across all the protocols.
>
> Relax the constraint on the duplicate name across the protocols and
> inhibit only within the same protocol id.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/3] firmware: arm_scmi: Relax duplicate name constraint across protocol ids
      https://git.kernel.org/sudeep.holla/c/21ee965267bc
[2/3] firmware: arm_scmi: Add name and protocol id attributes
      https://git.kernel.org/sudeep.holla/c/feaea74fd697
[3/3] firmware: arm_scmi: Emit modalias for SCMI devices
      https://git.kernel.org/sudeep.holla/c/d069c33f5ce2
--
Regards,
Sudeep