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

Zijun Hu
2024-08-23T21:45:49Z

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@quicinc.com>
>>>>>>
>>>>>> 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@quicinc.com
>>>> 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@quicinc.com>
>>>>>> ---
>>>>>> 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
>>
>
>