Re: [PATCH v2] driver core: bus: Remove an impossible error handling path in bus_add_driver()

[Zijun Hu]

On 2024/10/14 00:24, Greg Kroah-Hartman wrote:
> On Mon, Oct 14, 2024 at 12:13:30AM +0800, Zijun Hu wrote:
>> On 2024/10/14 00:00, Greg Kroah-Hartman wrote:
>>> On Sun, Oct 13, 2024 at 11:46:46PM +0800, Zijun Hu wrote:
>>>> On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
>>>>> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
>>>>>> On 2024/9/17 14:49, Zijun Hu wrote:
>>>>>>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>>>
>>>>>>> For the following function call chain:
>>>>>>> API driver_register() -> bus_add_driver() -> driver_attach()
>>>>>>>
>>>>>>> There is an error handling path for driver_attach() returning non-zero
>>>>>>> or failure in bus_add_driver(), remove it with below reasons:
>>>>>>>
>>>>>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
>>>>>>>   For int driver_attach(const struct device_driver *drv), the only factor
>>>>>>>   which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
>>>>>>>   the factor has been excluded by bus_add_driver() before calling it.
>>>>>>>
>>>>>>> - driver_attach() is irrelevant with driver_register(), so the former's
>>>>>>>   result should not also have an impact on the later.
>>>>>>>
>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> - Remove the error handling path instead of WARN_ON() it.
>>>>>>> - Correct title and commit message
>>>>>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@xxxxxxxxxxx
>>>>>>> ---
>>>>>>>  drivers/base/bus.c | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>>>>>> index 657c93c38b0d..54ff92aece92 100644
>>>>>>> --- a/drivers/base/bus.c
>>>>>>> +++ b/drivers/base/bus.c
>>>>>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
>>>>>>>  	if (sp->drivers_autoprobe) {
>>>>>>>  		error = driver_attach(drv);
>>>>>>>  		if (error)
>>>>>>> -			goto out_del_list;
>>>>>>> +			pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
>>>>>>> +				__func__, drv->name, sp->bus->name);
>>>>>>
>>>>>> driver_attach() has __must_check attribute and this error may be
>>>>>> inconsequential for driver_register(), so give pr_warn() here
>>>>>
>>>>> Yes, but you  now ignore the error, so someone will come back and add
>>>>> that error handling in.  I'd just leave it as-is.
>>>>>
>>>>
>>>> driver API driver_attach() may ONLY have below error -EINVAL.
>>>> is it worthy of a __must_check attribute ?
>>>
>>> Yes.
>>>
>>>> i agree with you to leave it as-is if your answer is "YES".
>>>> otherwise, i would like to also simply remove __must_check attribute.
>>>
>>> Please don't.  If you do that, then callers will end up not checking the
>>> results, and we do not want that.
>>>
>> okay.
>>
>> but as 2nd reason of commit message explained:
>> driver_attach() failure should NOT cause driver_register() failure, so
>> should ignore driver_attach() failure here.
> 
> How could driver_attach() fail and it still be ok to continue?
> 
sorry, i am wrong, agree with you to leave it as-is.

driver_attach() failure means driver itself error not attaching it with
devices error. so should cause driver_register() failure even if the
error is impossible here.

thanks.

> thanks,
> 
> greg k-h