Re: [PATCH v2 1/4] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()

[Zijun Hu]

On 2024/8/24 01:19, Ira Weiny wrote:
> Zijun Hu wrote:
>> On 2024/8/20 22:14, Ira Weiny wrote:
>>> Zijun Hu wrote:
>>>> On 2024/8/20 20:53, Ira Weiny wrote:
>>>>> Zijun Hu wrote:
>>>>>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>>
>>>>>> The following API cluster takes the same type parameter list, but do not
>>>>>> have consistent parameter check as shown below.
>>>>>>
>>>>>> device_for_each_child(struct device *parent, ...)  // check (!parent->p)
>>>>>> device_for_each_child_reverse(struct device *parent, ...) // same as above
>>>>>> device_find_child(struct device *parent, ...)      // check (!parent)
>>>>>>
>>>>>
>>>>> Seems reasonable.
>>>>>
>>>>> What about device_find_child_by_name()?
>>>>>
>>>>
>>>> Plan to simplify this API implementation by * atomic * API
>>>> device_find_child() as following:
>>>>
>>>> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@xxxxxxxxxxx
>>>> struct device *device_find_child_by_name(struct device *parent,
>>>>  					 const char *name)
>>>> {
>>>> 	return device_find_child(parent, name, device_match_name);
>>>> }
>>>
>>> Ok.  Thanks.
>>>
>>>>
>>>>>> Fixed by using consistent check (!parent || !parent->p) for the cluster.
>>>>>>
>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>> ---
>>>>>>  drivers/base/core.c | 6 +++---
>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>>>> index 1688e76cb64b..b1dd8c5590dc 100644
>>>>>> --- a/drivers/base/core.c
>>>>>> +++ b/drivers/base/core.c
>>>>>> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data,
>>>>>>  	struct device *child;
>>>>>>  	int error = 0;
>>>>>>  
>>>>>> -	if (!parent->p)
>>>>>> +	if (!parent || !parent->p)
>>>>>>  		return 0;
>>>>>>  
>>>>>>  	klist_iter_init(&parent->p->klist_children, &i);
>>>>>> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data,
>>>>>>  	struct device *child;
>>>>>>  	int error = 0;
>>>>>>  
>>>>>> -	if (!parent->p)
>>>>>> +	if (!parent || !parent->p)
>>>>>>  		return 0;
>>>>>>  
>>>>>>  	klist_iter_init(&parent->p->klist_children, &i);
>>>>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data,
>>>>>>  	struct klist_iter i;
>>>>>>  	struct device *child;
>>>>>>  
>>>>>> -	if (!parent)
>>>>>> +	if (!parent || !parent->p)
>>>>>
>>>>> Perhaps this was just a typo which should have been.
>>>>>
>>>>> 	if (!parent->p)
>>>>> ?
>>>>>
>>>> maybe, but the following device_find_child_by_name() also use (!parent).
>>>>
>>>>> I think there is an expectation that none of these are called with a NULL
>>>>> parent.
>>>>>
>>>>
>>>> this patch aim is to make these atomic APIs have consistent checks as
>>>> far as possible, that will make other patches within this series more
>>>> acceptable.
>>>>
>>>> i combine two checks to (!parent || !parent->p) since i did not know
>>>> which is better.
>>>
>>> I'm not entirely clear either.  But checking the member p makes more sense
>>> to me than the parent parameter.  I would expect that iterating the
>>> children of a device must be done only when the parent device is not NULL.
>>>
>>> parent->p is more subtle.  I'm unclear why the API would need to allow
>>> that to run without error.
>>>
>> i prefer (!parent || !parent->p) with below reasons:
>>
>> 1)
>> original API authors have such concern that either (!parent) or
>> (!parent->p) maybe happen since they are checked, all their concerns
>> can be covered by (!parent || !parent->p).
>>
>> 2)
>> It is the more robust than either (!parent) or (!parent->p)
>>
>> 3)
>> it also does not have any negative effect.
> 
> It adds code and instructions to all paths calling these functions.
> 
such slight impacts can be ignored if a machine run linux OS.

right?

> What is the reason to allow?
> 
1)
it allow to use device_for_each_child() without misgiving.

2)
there are many many existing APIs which have similar checks such as
get_device(), kfree()...

> void foo() {
> ...
> 	device_for_each_child(NULL, ...);
> ...
> }
> 
> What are we finding the child of in that case?
>
similar usage as device_find_child(NULL, ...) which have check (!parent).

both device_for_each_child() and device_find_child() iterates over its
child.

original author's concern (!parent->p) for device_for_each_child() is
applicable for the other.

original author's concern (!parent) for device_find_child() is
applicable for the other as well.

so i use (!parent || !parent->p).

> Ira
> 
>>
>>> Ira
>>
> 
>