From: Zijun Hu <zijun.hu@oss.qualcomm.com>

auxiliary_match_id() repeatedly calculates variable @match_size in the
for loop, however, the variable is fixed actually, so it is enough to
only calculate the variable once.

Besides, the function should return directly if name of the @auxdev
does not include '.', but it still iterates over the ID table.

Additionally, statement 'dev_name(&auxdev->dev)' is fixed, but may be
evaluated more than 3 times.

Optimize logic of the function by:
- Move the logic calculating the variable out of the for loop
- Return NULL directly if @p == NULL
- Give the statement an dedicated local variable @auxdev_name

Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
---
Changes in v2:
- Give statement 'dev_name(&auxdev->dev)' a dedicated local variable
- Correct tile and commit message
- Link to v1: https://lore.kernel.org/r/20250902-fix_auxbus-v1-1-9ba6d8aff027@oss.qualcomm.com
---
 drivers/base/auxiliary.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
index 12ffdd8437567f282374bbf3775d9de7ca0dc4c7..ed2537cf3b048149e784e5a582631db549050734 100644
--- a/drivers/base/auxiliary.c
+++ b/drivers/base/auxiliary.c
@@ -171,17 +171,18 @@
 static const struct auxiliary_device_id *auxiliary_match_id(const struct auxiliary_device_id *id,
 							    const struct auxiliary_device *auxdev)
 {
-	for (; id->name[0]; id++) {
-		const char *p = strrchr(dev_name(&auxdev->dev), '.');
-		int match_size;
+	const char *auxdev_name = dev_name(&auxdev->dev);
+	const char *p = strrchr(auxdev_name, '.');
+	int match_size;
 
-		if (!p)
-			continue;
-		match_size = p - dev_name(&auxdev->dev);
+	if (!p)
+		return NULL;
+	match_size = p - auxdev_name;
 
+	for (; id->name[0]; id++) {
 		/* use dev_name(&auxdev->dev) prefix before last '.' char to match to */
 		if (strlen(id->name) == match_size &&
-		    !strncmp(dev_name(&auxdev->dev), id->name, match_size))
+		    !strncmp(auxdev_name, id->name, match_size))
 			return id;
 	}
 	return NULL;

---
base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
change-id: 20250902-fix_auxbus-76bec91376db

Best regards,
-- 
Zijun Hu <zijun.hu@oss.qualcomm.com>

On Wed Sep 3, 2025 at 1:37 PM CEST, Zijun Hu wrote:
> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> index 12ffdd8437567f282374bbf3775d9de7ca0dc4c7..ed2537cf3b048149e784e5a582631db549050734 100644
> --- a/drivers/base/auxiliary.c
> +++ b/drivers/base/auxiliary.c
> @@ -171,17 +171,18 @@
>  static const struct auxiliary_device_id *auxiliary_match_id(const struct auxiliary_device_id *id,
>  							    const struct auxiliary_device *auxdev)
>  {
> -	for (; id->name[0]; id++) {
> -		const char *p = strrchr(dev_name(&auxdev->dev), '.');
> -		int match_size;
> +	const char *auxdev_name = dev_name(&auxdev->dev);

I feel like this is a bit too ambitious. Using dev_name() directly for the calls
below seems more obvious to me.

Yes, dev_name() contains a branch, but I'm pretty sure the compiler optimizes
them away for subsequent calls anyways.

> +	const char *p = strrchr(auxdev_name, '.');
> +	int match_size;
>  
> -		if (!p)
> -			continue;
> -		match_size = p - dev_name(&auxdev->dev);
> +	if (!p)
> +		return NULL;
> +	match_size = p - auxdev_name;
>  
> +	for (; id->name[0]; id++) {
>  		/* use dev_name(&auxdev->dev) prefix before last '.' char to match to */
>  		if (strlen(id->name) == match_size &&
> -		    !strncmp(dev_name(&auxdev->dev), id->name, match_size))
> +		    !strncmp(auxdev_name, id->name, match_size))
>  			return id;
>  	}
>  	return NULL;
>
> ---
> base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
> change-id: 20250902-fix_auxbus-76bec91376db
>
> Best regards,
> -- 
> Zijun Hu <zijun.hu@oss.qualcomm.com>

On 2025/9/3 19:46, Danilo Krummrich wrote:
>>  static const struct auxiliary_device_id *auxiliary_match_id(const struct auxiliary_device_id *id,
>>  							    const struct auxiliary_device *auxdev)
>>  {
>> -	for (; id->name[0]; id++) {
>> -		const char *p = strrchr(dev_name(&auxdev->dev), '.');
>> -		int match_size;
>> +	const char *auxdev_name = dev_name(&auxdev->dev);
> I feel like this is a bit too ambitious. Using dev_name() directly for the calls
> below seems more obvious to me.
> 

dev_name(&auxdev->dev) which is a inline function appears 3 times in the
for loop. may be worthy of a new variable


> Yes, dev_name() contains a branch, but I'm pretty sure the compiler optimizes
> them away for subsequent calls anyways.

From debugging perspective:
more difficult to debug if more the compiler optimizes.

On Thu, Sep 04, 2025 at 07:00:37AM +0800, Zijun Hu wrote:
> On 2025/9/3 19:46, Danilo Krummrich wrote:
> >>  static const struct auxiliary_device_id *auxiliary_match_id(const struct auxiliary_device_id *id,
> >>  							    const struct auxiliary_device *auxdev)
> >>  {
> >> -	for (; id->name[0]; id++) {
> >> -		const char *p = strrchr(dev_name(&auxdev->dev), '.');
> >> -		int match_size;
> >> +	const char *auxdev_name = dev_name(&auxdev->dev);
> > I feel like this is a bit too ambitious. Using dev_name() directly for the calls
> > below seems more obvious to me.
> > 
> 
> dev_name(&auxdev->dev) which is a inline function appears 3 times in the
> for loop. may be worthy of a new variable
> 
> 
> > Yes, dev_name() contains a branch, but I'm pretty sure the compiler optimizes
> > them away for subsequent calls anyways.
> 
> >From debugging perspective:
> more difficult to debug if more the compiler optimizes.

You aren't debugging stuff like this :)

Anyway, I've taken this, as it is a nice tiny cleanup, thanks.

greg k-h