[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa08e89d-40d0-41b9-8542-f37f1f716b56@icloud.com>
Date: Mon, 14 Oct 2024 00:13:30 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v2] driver core: bus: Remove an impossible error handling
path in bus_add_driver()
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@...cinc.com>
>>>>>
>>>>> 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@...cinc.com>
>>>>> ---
>>>>> 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@quicinc.com
>>>>> ---
>>>>> 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. but driver_attach() has
__must_check attribute, it will has build error if ignore the failure,
so this solution is worked out: if (error) pr_warn().
does this solution meet __must_check attribute ?
> thanks,
>
> greg k-h
Powered by blists - more mailing lists