[PATCH V1] firmware: arm_scmi: Optimize the iteration of scmi_requested_devices

[PATCH V1] firmware: arm_scmi: Optimize the iteration of scmi_requested_devices

[Peng Fan (OSS)]

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.

Back to search the next id with 'continue' to save cpu cycles, if
protocol_id does not match.

Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V1:
 Follow Dan's suggestion

 drivers/firmware/arm_scmi/bus.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

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;
 	}
 
 	/*
-- 
2.37.1

Re: [PATCH V1] firmware: arm_scmi: Optimize the iteration of scmi_requested_devices

[Cristian Marussi]

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

Re: [PATCH V1] firmware: arm_scmi: Optimize the iteration of scmi_requested_devices

[Sudeep Holla]

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