[PATCH] firmware: arm_scmi: Delete the meaningless scmi_bus_id.

[PATCH] firmware: arm_scmi: Delete the meaningless scmi_bus_id.

[guomin_chen]

From: guomin chen <guomin_chen@sina.com>

Currently, scmi_bus_id is only used to set scmi_dev.id,
which in turn sets the SCMI device name. After removing
scmi_bus_id, it is clearer and more meaningful to directly
use the input parameters name and protocol to set the SCMI
device name.

Signed-off-by: guomin chen <guomin_chen@sina.com>
---
 drivers/firmware/arm_scmi/bus.c    | 16 ++--------------
 drivers/firmware/arm_scmi/driver.c |  4 ++--
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 157172a5f2b5..c4e013a77e33 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -20,7 +20,6 @@
 BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
 EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
 
-static DEFINE_IDA(scmi_bus_id);
 
 static DEFINE_IDR(scmi_requested_devices);
 /* Protect access to scmi_requested_devices */
@@ -341,7 +340,6 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
 	if (scmi_dev->protocol_id == SCMI_PROTOCOL_SYSTEM)
 		atomic_set(&scmi_syspower_registered, 0);
 
-	ida_free(&scmi_bus_id, scmi_dev->id);
 	device_unregister(&scmi_dev->dev);
 }
 
@@ -349,7 +347,7 @@ static struct scmi_device *
 __scmi_device_create(struct device_node *np, struct device *parent,
 		     int protocol, const char *name)
 {
-	int id, retval;
+	int retval;
 	struct scmi_device *scmi_dev;
 
 	/*
@@ -387,20 +385,12 @@ __scmi_device_create(struct device_node *np, struct device *parent,
 		return NULL;
 	}
 
-	id = ida_alloc_min(&scmi_bus_id, 1, GFP_KERNEL);
-	if (id < 0) {
-		kfree_const(scmi_dev->name);
-		kfree(scmi_dev);
-		return NULL;
-	}
-
-	scmi_dev->id = id;
 	scmi_dev->protocol_id = protocol;
 	scmi_dev->dev.parent = parent;
 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
 	scmi_dev->dev.bus = &scmi_bus_type;
 	scmi_dev->dev.release = scmi_device_release;
-	dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);
+	dev_set_name(&scmi_dev->dev, "scmi_dev.%s.%d", scmi_dev->name, protocol);
 
 	retval = device_register(&scmi_dev->dev);
 	if (retval)
@@ -413,7 +403,6 @@ __scmi_device_create(struct device_node *np, struct device *parent,
 	return scmi_dev;
 put_dev:
 	put_device(&scmi_dev->dev);
-	ida_free(&scmi_bus_id, id);
 	return NULL;
 }
 
@@ -526,7 +515,6 @@ static void __exit scmi_bus_exit(void)
 	 */
 	scmi_devices_unregister();
 	bus_unregister(&scmi_bus_type);
-	ida_destroy(&scmi_bus_id);
 }
 module_exit(scmi_bus_exit);
 
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 1b5fb2c4ce86..bbf1f05f2be3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2641,8 +2641,8 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
 	cinfo->max_msg_size = info->desc->max_msg_size;
 
 	/* Create a unique name for this transport device */
-	snprintf(name, 32, "__scmi_transport_device_%s_%02X",
-		 idx ? "rx" : "tx", prot_id);
+	snprintf(name, 32, "__scmi_transport_device_%s",
+		 idx ? "rx" : "tx");
 	/* Create a uniquely named, dedicated transport device for this chan */
 	tdev = scmi_device_create(of_node, info->dev, prot_id, name);
 	if (!tdev) {
-- 
2.47.1

Re: [PATCH] firmware: arm_scmi: Delete the meaningless scmi_bus_id.

[Cristian Marussi]

On Wed, Dec 11, 2024 at 09:45:05PM +0800, guomin_chen@sina.com wrote:
> From: guomin chen <guomin_chen@sina.com>
> 
> Currently, scmi_bus_id is only used to set scmi_dev.id,
> which in turn sets the SCMI device name. After removing
> scmi_bus_id, it is clearer and more meaningful to directly
> use the input parameters name and protocol to set the SCMI
> device name.

Hi,

even though using progressive IDs in devices is less readable, I agree,
we need ID in the name to keep the device unique.

It is true that we can have only one protocol/name unique pair amongst
the *requested* devices BUT it is also true that the SCMI stack as it
stands can be instantiated multiple times if you define multiple DT SCMI
top-nodes: this is already used in the wild to sort of represent
multiple virtual/physical SCMI Server backend (all anyway identified by
ID-0 as from the spec...)

So if you have defind in the DT 2 SCMI instance with both a protocol@15/HWMON,
as an example, the arm_scmi/driver.c will probe twice and it will try to
create 2 instances of 'requested' devices for 15/hwmon and both of these
will be attached to the same *unique* SCMI bus...so we need the uniqe ID
to differentiate them....same for the transport devices.

Sorry but NACK for me: regarding the readability, I could agree, but you
can anyway easily understand which device is which by looking at the
drivers/ links generated in the scmi_protocol bus directory.

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Delete the meaningless scmi_bus_id.

[gchen chen]

Cristian Marussi <cristian.marussi@arm.com> 于2024年12月12日周四 02:28写道：
>
> On Wed, Dec 11, 2024 at 09:45:05PM +0800, guomin_chen@sina.com wrote:
> > From: guomin chen <guomin_chen@sina.com>
> >
> > Currently, scmi_bus_id is only used to set scmi_dev.id,
> > which in turn sets the SCMI device name. After removing
> > scmi_bus_id, it is clearer and more meaningful to directly
> > use the input parameters name and protocol to set the SCMI
> > device name.
>
> Hi,
>
> even though using progressive IDs in devices is less readable, I agree,
> we need ID in the name to keep the device unique.
>
> It is true that we can have only one protocol/name unique pair amongst
> the *requested* devices BUT it is also true that the SCMI stack as it
> stands can be instantiated multiple times if you define multiple DT SCMI
> top-nodes: this is already used in the wild to sort of represent
> multiple virtual/physical SCMI Server backend (all anyway identified by
> ID-0 as from the spec...)
>
> So if you have defind in the DT 2 SCMI instance with both a protocol@15/HWMON,
> as an example, the arm_scmi/driver.c will probe twice and it will try to
> create 2 instances of 'requested' devices for 15/hwmon and both of these
> will be attached to the same *unique* SCMI bus...so we need the uniqe ID
> to differentiate them....same for the transport devices.

Okay, it is true that if two SCMI instances are defined in DT using
the same protocol,
it will result in devices with the same name. Thank you for pointing this out!

Thanks

> Sorry but NACK for me: regarding the readability, I could agree, but you
> can anyway easily understand which device is which by looking at the
> drivers/ links generated in the scmi_protocol bus directory.
>
> Thanks,
> Cristian