[PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Simplify device_find_child_by_name() implementation by using present
driver APIs device_find_child() and device_match_name().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c    | 15 +++------------
 include/linux/device.h |  4 ++++
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 730cae66607c..22ab4b8a2bcd 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
 struct device *device_find_child_by_name(struct device *parent,
 					 const char *name)
 {
-	struct klist_iter i;
-	struct device *child;
-
-	if (!parent)
-		return NULL;
-
-	klist_iter_init(&parent->p->klist_children, &i);
-	while ((child = next_device(&i)))
-		if (sysfs_streq(dev_name(child), name) && get_device(child))
-			break;
-	klist_iter_exit(&i);
-	return child;
+	/* TODO: remove type cast after const device_find_child() prototype */
+	return device_find_child(parent, (void *)name,
+				 (int (*)(struct device *, void *))device_match_name);
 }
 EXPORT_SYMBOL_GPL(device_find_child_by_name);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 34eb20f5966f..685ffd2dc867 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -47,6 +47,9 @@ struct dev_pin_info;
 struct dev_iommu;
 struct msi_device_data;
 
+/* TODO: unify device match() parameter of driver APIs to this signature */
+typedef int (*device_match_t)(struct device *dev, const void *data);
+
 /**
  * struct subsys_interface - interfaces to device functions
  * @name:       name of the device function
@@ -1073,6 +1076,7 @@ int device_for_each_child(struct device *dev, void *data,
 			  int (*fn)(struct device *dev, void *data));
 int device_for_each_child_reverse(struct device *dev, void *data,
 				  int (*fn)(struct device *dev, void *data));
+/* TODO: change type of @data to const void * and @match to device_match_t */
 struct device *device_find_child(struct device *dev, void *data,
 				 int (*match)(struct device *dev, void *data));
 struct device *device_find_child_by_name(struct device *parent,

---
base-commit: b57d5ffc3ab507d0e19fc8b90b19c76af43fb790
change-id: 20240720-simplify_drv_api-058f9f3584c2

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

Re: [PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Greg Kroah-Hartman]

On Sat, Jul 20, 2024 at 09:21:50AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Simplify device_find_child_by_name() implementation by using present
> driver APIs device_find_child() and device_match_name().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/base/core.c    | 15 +++------------
>  include/linux/device.h |  4 ++++
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 730cae66607c..22ab4b8a2bcd 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
>  struct device *device_find_child_by_name(struct device *parent,
>  					 const char *name)
>  {
> -	struct klist_iter i;
> -	struct device *child;
> -
> -	if (!parent)
> -		return NULL;
> -
> -	klist_iter_init(&parent->p->klist_children, &i);
> -	while ((child = next_device(&i)))
> -		if (sysfs_streq(dev_name(child), name) && get_device(child))
> -			break;
> -	klist_iter_exit(&i);
> -	return child;
> +	/* TODO: remove type cast after const device_find_child() prototype */

I do not understand the TODO here.  Why is it needed?  Why not fix it up
now?

> +	return device_find_child(parent, (void *)name,
> +				 (int (*)(struct device *, void *))device_match_name);
>  }
>  EXPORT_SYMBOL_GPL(device_find_child_by_name);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 34eb20f5966f..685ffd2dc867 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -47,6 +47,9 @@ struct dev_pin_info;
>  struct dev_iommu;
>  struct msi_device_data;
>  
> +/* TODO: unify device match() parameter of driver APIs to this signature */

Same here, why have this?  Why not unify them and then fix this up?

> +typedef int (*device_match_t)(struct device *dev, const void *data);
> +
>  /**
>   * struct subsys_interface - interfaces to device functions
>   * @name:       name of the device function
> @@ -1073,6 +1076,7 @@ int device_for_each_child(struct device *dev, void *data,
>  			  int (*fn)(struct device *dev, void *data));
>  int device_for_each_child_reverse(struct device *dev, void *data,
>  				  int (*fn)(struct device *dev, void *data));
> +/* TODO: change type of @data to const void * and @match to device_match_t */

Again, why leave this here?

thanks,

greg k-h

Re: [PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Zijun Hu]

On 2024/7/31 20:53, Greg Kroah-Hartman wrote:
> On Sat, Jul 20, 2024 at 09:21:50AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Simplify device_find_child_by_name() implementation by using present
>> driver APIs device_find_child() and device_match_name().
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/base/core.c    | 15 +++------------
>>  include/linux/device.h |  4 ++++
>>  2 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 730cae66607c..22ab4b8a2bcd 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
>>  struct device *device_find_child_by_name(struct device *parent,
>>  					 const char *name)
>>  {
>> -	struct klist_iter i;
>> -	struct device *child;
>> -
>> -	if (!parent)
>> -		return NULL;
>> -
>> -	klist_iter_init(&parent->p->klist_children, &i);
>> -	while ((child = next_device(&i)))
>> -		if (sysfs_streq(dev_name(child), name) && get_device(child))
>> -			break;
>> -	klist_iter_exit(&i);
>> -	return child;
>> +	/* TODO: remove type cast after const device_find_child() prototype */
> 
> I do not understand the TODO here.  Why is it needed?  Why not fix it up
> now?
> 

i have below findings during trying to simplify this API.

there are a type of driver APIs for finding device, for example,
(bus|driver|class)_find_device() which all take below type for
parameter @match:
int (*match)(struct device *, const void *match_data)
but device_find_child() take below type with void * for @match_data:
int (*match)(struct device *, void *match_data).

@match's type of device_find_child() is not good as other finding APIs
since nothing will be touched for finding operations normally.

i want to introduce a dedicate type for device match.
typedef int (*device_match_t)(struct device *dev, const void *data);

advantages:
1) device_match_t is simpler for finding APIs declarations and definitions
2) maybe stop further driver APIs from using bad match type as
device_find_child()

TODO:
1) introduce device_match_t and use it for current present APIs
(bus|driver|class)_find_device()
2) change API device_find_child() to take device_match_t, more jobs to
do since need to touch many drivers
3) correct this change by by remove all TODO inline comments and force cast.

not sure if my ideas is good, what is your opinions?

>> +	return device_find_child(parent, (void *)name,
>> +				 (int (*)(struct device *, void *))device_match_name);
>>  }
>>  EXPORT_SYMBOL_GPL(device_find_child_by_name);
>>  
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 34eb20f5966f..685ffd2dc867 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -47,6 +47,9 @@ struct dev_pin_info;
>>  struct dev_iommu;
>>  struct msi_device_data;
>>  
>> +/* TODO: unify device match() parameter of driver APIs to this signature */
> 
> Same here, why have this?  Why not unify them and then fix this up?
> 
>> +typedef int (*device_match_t)(struct device *dev, const void *data);
>> +
>>  /**
>>   * struct subsys_interface - interfaces to device functions
>>   * @name:       name of the device function
>> @@ -1073,6 +1076,7 @@ int device_for_each_child(struct device *dev, void *data,
>>  			  int (*fn)(struct device *dev, void *data));
>>  int device_for_each_child_reverse(struct device *dev, void *data,
>>  				  int (*fn)(struct device *dev, void *data));
>> +/* TODO: change type of @data to const void * and @match to device_match_t */
> 
> Again, why leave this here?
> 
> thanks,
> 
> greg k-h

Re: [PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Greg Kroah-Hartman]

On Thu, Aug 01, 2024 at 12:04:40AM +0800, Zijun Hu wrote:
> On 2024/7/31 20:53, Greg Kroah-Hartman wrote:
> > On Sat, Jul 20, 2024 at 09:21:50AM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> Simplify device_find_child_by_name() implementation by using present
> >> driver APIs device_find_child() and device_match_name().
> >>
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  drivers/base/core.c    | 15 +++------------
> >>  include/linux/device.h |  4 ++++
> >>  2 files changed, 7 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index 730cae66607c..22ab4b8a2bcd 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
> >>  struct device *device_find_child_by_name(struct device *parent,
> >>  					 const char *name)
> >>  {
> >> -	struct klist_iter i;
> >> -	struct device *child;
> >> -
> >> -	if (!parent)
> >> -		return NULL;
> >> -
> >> -	klist_iter_init(&parent->p->klist_children, &i);
> >> -	while ((child = next_device(&i)))
> >> -		if (sysfs_streq(dev_name(child), name) && get_device(child))
> >> -			break;
> >> -	klist_iter_exit(&i);
> >> -	return child;
> >> +	/* TODO: remove type cast after const device_find_child() prototype */
> > 
> > I do not understand the TODO here.  Why is it needed?  Why not fix it up
> > now?
> > 
> 
> i have below findings during trying to simplify this API.
> 
> there are a type of driver APIs for finding device, for example,
> (bus|driver|class)_find_device() which all take below type for
> parameter @match:
> int (*match)(struct device *, const void *match_data)
> but device_find_child() take below type with void * for @match_data:
> int (*match)(struct device *, void *match_data).
> 
> @match's type of device_find_child() is not good as other finding APIs
> since nothing will be touched for finding operations normally.
> 
> i want to introduce a dedicate type for device match.
> typedef int (*device_match_t)(struct device *dev, const void *data);

Yes, that would be good.

> advantages:
> 1) device_match_t is simpler for finding APIs declarations and definitions
> 2) maybe stop further driver APIs from using bad match type as
> device_find_child()
> 
> TODO:
> 1) introduce device_match_t and use it for current present APIs
> (bus|driver|class)_find_device()
> 2) change API device_find_child() to take device_match_t, more jobs to
> do since need to touch many drivers
> 3) correct this change by by remove all TODO inline comments and force cast.
> 
> not sure if my ideas is good, what is your opinions?

That's great, but evolve it properly, don't add TODO lines here, there's
no real need for that.  Should end up with a lot of good cleanup
changes, and might not be all that bad overall.

thanks,

greg k-h

Re: [PATCH] driver core: Simplify driver API device_find_child_by_name() implementation

[Zijun Hu]

On 2024/8/1 00:21, Greg Kroah-Hartman wrote:
> On Thu, Aug 01, 2024 at 12:04:40AM +0800, Zijun Hu wrote:
>> On 2024/7/31 20:53, Greg Kroah-Hartman wrote:
>>> On Sat, Jul 20, 2024 at 09:21:50AM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> Simplify device_find_child_by_name() implementation by using present
>>>> driver APIs device_find_child() and device_match_name().
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>>  drivers/base/core.c    | 15 +++------------
>>>>  include/linux/device.h |  4 ++++
>>>>  2 files changed, 7 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>> index 730cae66607c..22ab4b8a2bcd 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -4089,18 +4089,9 @@ EXPORT_SYMBOL_GPL(device_find_child);
>>>>  struct device *device_find_child_by_name(struct device *parent,
>>>>  					 const char *name)
>>>>  {
>>>> -	struct klist_iter i;
>>>> -	struct device *child;
>>>> -
>>>> -	if (!parent)
>>>> -		return NULL;
>>>> -
>>>> -	klist_iter_init(&parent->p->klist_children, &i);
>>>> -	while ((child = next_device(&i)))
>>>> -		if (sysfs_streq(dev_name(child), name) && get_device(child))
>>>> -			break;
>>>> -	klist_iter_exit(&i);
>>>> -	return child;
>>>> +	/* TODO: remove type cast after const device_find_child() prototype */
>>>
>>> I do not understand the TODO here.  Why is it needed?  Why not fix it up
>>> now?
>>>
>>
>> i have below findings during trying to simplify this API.
>>
>> there are a type of driver APIs for finding device, for example,
>> (bus|driver|class)_find_device() which all take below type for
>> parameter @match:
>> int (*match)(struct device *, const void *match_data)
>> but device_find_child() take below type with void * for @match_data:
>> int (*match)(struct device *, void *match_data).
>>
>> @match's type of device_find_child() is not good as other finding APIs
>> since nothing will be touched for finding operations normally.
>>
>> i want to introduce a dedicate type for device match.
>> typedef int (*device_match_t)(struct device *dev, const void *data);
> 
> Yes, that would be good.
> 
>> advantages:
>> 1) device_match_t is simpler for finding APIs declarations and definitions
>> 2) maybe stop further driver APIs from using bad match type as
>> device_find_child()
>>
>> TODO:
>> 1) introduce device_match_t and use it for current present APIs
>> (bus|driver|class)_find_device()
>> 2) change API device_find_child() to take device_match_t, more jobs to
>> do since need to touch many drivers
>> 3) correct this change by by remove all TODO inline comments and force cast.
>>
>> not sure if my ideas is good, what is your opinions?
> 
> That's great, but evolve it properly, don't add TODO lines here, there's
> no real need for that.  Should end up with a lot of good cleanup
> changes, and might not be all that bad overall.
> 

okay, thanks for these good suggestions.

let me mark this patch as abandoned now since i would like to do it by
another better way.

> thanks,
> 
> greg k-h