Re: [PATCH] driver core: bus: Mark an impossible error path with WARN_ON() in bus_add_driver()

[Zijun Hu]

On 2024/9/15 21:55, Greg Kroah-Hartman wrote:
> On Sun, Sep 15, 2024 at 09:38:15PM +0800, Zijun Hu wrote:
>> On 2024/9/15 21:00, Greg Kroah-Hartman wrote:
>>> On Sun, Sep 15, 2024 at 06:22:05PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>>
>>>> driver_attach() called by bus_add_driver() always returns 0, so its
>>>> corresponding error path will never happen, hence mark the impossible
>>>> error path with WARN_ON() to remind readers to disregard it.
>>>
>>> So you just caused the machine to crash and reboot if that happens
>>> (remember, panic-on-warn is enabled in a few billion Linux systems...)
>>>
>> are there good way to mark a if condition which is always or mostly
>> evaluated to false currently without any side effect?
> 
> If always, then remove the code involved.  If mostly, just do it
> normally.
> 
>> i think this is a generic requirement since readers may not want to
>> care about things which will never or rarely happen, below link
>> involves such discussion:
>> https://lore.kernel.org/all/2024090444-earmark-showpiece-b3dc@gregkh/
> 
> Yes, but likely/unlikely is for performance, not for documentation.

if you git grep unlikely in current kernel tree, you will find
that there are too many unlikely usages which should be irrelevant
performance. you maybe look at drivers/base/devres.c.

so i think one of purpose of unlikely may be for the requirement i
mentioned.

> 
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/base/bus.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>>> index 657c93c38b0d..59a48edda267 100644
>>>> --- a/drivers/base/bus.c
>>>> +++ b/drivers/base/bus.c
>>>> @@ -673,7 +673,7 @@ int bus_add_driver(struct device_driver *drv)
>>>>  	klist_add_tail(&priv->knode_bus, &sp->klist_drivers);
>>>>  	if (sp->drivers_autoprobe) {
>>>>  		error = driver_attach(drv);
>>>> -		if (error)
>>>> +		if (WARN_ON(error))
>>>
>>> What exactly are you trying to show here?  If this really can never
>>> fail, then let's just remove the check entirely.
>>>
>> what i want to show is that this error patch will never happen here
>> currently, so readers can disregard it.
> 
> Then just remove it, and document in the changelog text why it can never
> happen.  But if it can never happen, then why is the function returning
> anything at all?
> 
> thanks,
> 
> greg k-h