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@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>>  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