[PATCH v2] firmware: arm_scmi: Delete the meaningless scmi_bus_id.
gchen chen
gchen.guomin at gmail.com
Mon Dec 16 06:10:40 PST 2024
Sudeep Holla <sudeep.holla at arm.com> 于2024年12月16日周一 18:45写道：
>
> On Mon, Dec 16, 2024 at 06:37:01PM +0800, gchen chen wrote:
> > Cristian Marussi <cristian.marussi at arm.com> 于2024年12月16日周一 17:45写道：
> > >
> > > On Mon, Dec 16, 2024 at 08:50:26AM +0000, Cristian Marussi wrote:
> > > > On Mon, Dec 16, 2024 at 03:37:45PM +0800, guomin_chen at sina.com wrote:
> > > > > From: guomin chen <guomin_chen at 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 unique tuple [Parent name,device name, protocol] to
> > > > > set the SCMI device name.
> > > > >
> > > >
> > > > Hi Guomin,
> > > >
> > > > this same pTCH was NACKed(Rejected) a few days ago:
> > > >
> > > > https://lore.kernel.org/arm-scmi/20241211134505.2218386-1-guomin_chen@sina.com/T/#u
> > > >
> > > > ...and you agreed that is not a simplification we can do (not to break
> > > > multuple instances...)...
> > > >
> > > > ..so why you are posting V2 now ?
> > >
> > > Wait...you changed slightly this indeed....you are using dev_parent...my
> > > bad...but please when you post new version of a patch add a summary of
> > > changes between versions...
> > >
> > > >
> > > > Thanks,
> > > > Cristian
> > > >
> > > > > Signed-off-by: guomin chen <guomin_chen at sina.com>
> > > > > ---
> > > > > drivers/firmware/arm_scmi/bus.c | 17 +++--------------
> > > > > drivers/firmware/arm_scmi/driver.c | 4 ++--
> > > > > 2 files changed, 5 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > > > > index 157172a5f2b5..800e8ec9357c 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,13 @@ __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.%s.%d", dev_name(parent),
> > > > > + scmi_dev->name, protocol);
> > > > >
> > >
> > > So now you are using the parent top node SCMI device to create a unique
> > > device...and it probably endup in a name like:
> > >
> > > scmi-dev.arm-scmi.0.auto.genpd.13
> > >
> > > scmi-dev.arm-scmi.1.auto.genpd.13
> > >
> > > ...that certainly is more readable but is bulky and anyway giving unique
> > > IDs to devices is pretty common and you lookup which device is which by
> > > simply looking at the drivers/ link....not suure what's the benefit of
> > > all of this...just to avoid an IDA table ?
> > >
> > > > > retval = device_register(&scmi_dev->dev);
> > > > > if (retval)
> > > > > @@ -413,7 +404,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 +516,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");
> > >
> > > I agree on what Dan said AND also this results in having the same name for different
> > > devices across 2 instances if you have a per-protocol channel because you havent anymore
> > > the protocol_id bit.
> > hi Cristian，and dan
> > Because I used a 3-tuple [parent name, name, protocol] when
> > creating the SCMI device name, I removed the prot_id from the
> > parameter ‘name’ when creating transport devices. This way, it avoids
> > the repetition of protocol ID in the SCMI device name.
> >
> > > All in all, I would drop this patch and keep naming as it is because I dont see a real benefit
> > > here....up to Sudeep decide.
> > Yes, it does not have real benefits, but from the perspective of
> > the device name, this change will be clearer and more meaningful. And
> > the code is more concise.
> >
>
> I would like to understand the motivation behind this change. What is the
> goal ? Do you prefer to fetch the name and protocol id from the device
> name itself ? Is that your requirement.
>
hi Sudeep
Okay, the reason I want to change names like 'scmi_dev.3' to
'scmi_dev.firmware:scmi.perf.19' is because when I was migrating the
SOC's kernel from v6.1 to v6.6, I found that the context for creating
SCMI devices had changed (commit d3cd7c525fd2: firmware: arm_scmi:
Refactor protocol device creation). This change meant that device
creation shifted from being directly created through
scmi_protocol_device_request during SCMI driver registration to being
created via scmi_device_request_notifier. This shift results in
changes to the order in which devices are created, causing the ID in
scmi_dev.id to drift.
Additionally, I encountered some cpufreq errors here—because both
scmi_cpufreq_drv and scmi_perf_domain_driver use the same
SCMI_PROTOCOL_PERF, this results in two SCMI devices corresponding to
the same device node. However, device_node.fwnode.dev only points to
the first registered scmi_device, causing other consumer devices to
find the wrong scmi device as the supplier. So I would find a
multitude of other consumer devices waiting for meaningless device
names like "scmi_dev.4" instead of meaningful names such as
"scmi_dev.firmware:scmi.perf.19" or
"scmi_dev.firmware:scmi.cpufreq.19".
Although I could further determine which specific driver it was by
looking at the driver links under the scmi_protocol bus directory, I
thought that if the logs directly displayed device names like
'scmi_dev.firmware:scmi.perf.19' instead of meaningless progressive
IDs, it would be more convenient and logical, and thus more
meaningful.
> From the commit log, I get a sense that you looked at the code and thought
> of possible improvement but when we mentioned the limitation you just
> improvised by adding parent name. Do you expect any userspace to parse
> the name as that will end up being ABI and we can't break it. I need
> real motive to be explained here in detail.
>
I did not use userspace tools to parse this SCMI device name; I simply
wanted the name to reflect the possible logic of the device.
I did not remove scmi_dev.id (This scmi_device structure has not
changed.）; I just no longer assign values to it using scmi_bus_id, so
it should not affect the kernel ABI (kABI).
Thanks
guomin.chen
> --
> Regards,
> Sudeep
More information about the linux-arm-kernel
mailing list