| Message ID | 20250107052004.762817-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V1] firmware: arm_scmi: Optimize the iteration of scmi_requested_devices | expand |
On Tue, Jan 07, 2025 at 01:20:04PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > scmi_requested_devices is organized in IDR based link lists, so only > need to search the link lists when there is a match protocol_id. Hi, while the original implementation of this loop was awful (my bad), I think this optiomization indeed NOW changes the logic of the checks.... ...you can have number of proto_id/name pairs describing requested devices and the idea of the logic in this loop was to catch ANY duplicate name across ANY of the protocol list as stated in the function Dox: --- * 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; * 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. --- The 'awfulness' of the original loop was that it was trying to do 2 things at once: - loop all the lists in the IDR for ALL protocols looking for a duplicate name anywhere, and FAIL if ANY found ... WHILE at the same time: - save the head of the protocol list_head matching the requested id_table->protocol, if any already existing, so as not to have to scan So, indeed the original loop was meant to be an optimization, even though probably unreadable.... ... having said that, I cannot really recall WHY I decided to avoid ANY kind of duplicate naming given that teh match happen by proto_id/name... ....maybe just for clarity ? not sure...even what the effects could be of having such duplicated names across protocols... Thanks, Cristian
On Thu, Jan 30, 2025 at 01:07:35PM +0000, Cristian Marussi wrote: > > ... having said that, I cannot really recall WHY I decided to avoid ANY > kind of duplicate naming given that teh match happen by proto_id/name... > > ....maybe just for clarity ? not sure...even what the effects could be > of having such duplicated names across protocols... > Looking at scmi_dev_match_id(), it should be fine even if the names are duplicated across protocols. We must just not allow duplicates within a protocol IMO. I will push a patch to handle it soon. -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 157172a5f2b5..c26f470fc24f 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -72,14 +72,11 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table) */ 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; - } + /* 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) + continue; + list_for_each_entry(rdev, head, node) { if (!strcmp(rdev->id_table->name, id_table->name)) { pr_err("Ignoring duplicate request [%d] %s\n", @@ -89,6 +86,8 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table) goto out; } } + phead = head; + break; } /*