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

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

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

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.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 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))
 			goto out_del_list;
 	}
 	error = module_add_driver(drv->owner, drv);

---
base-commit: 6a36d828bdef0e02b1e6c12e2160f5b83be6aab5
change-id: 20240915-bus_add_driver_fix-f54841e6a69a

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

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

[Greg Kroah-Hartman]

On Sun, Sep 15, 2024 at 06:22:05PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> 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...)

> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  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.

thanks,

greg k-h

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

[Zijun Hu]

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

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/

>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  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.

let me try to do it after discussion done.

> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

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

> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  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

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@quicinc.com>
>>>>
>>>> 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...)
>>>
[cut]
>> 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?
> 

the only condition for driver_attach() returning error will be
impossible within bus_add_driver()'s context as shown below:

int bus_add_driver(struct device_driver *drv)
{
	struct subsys_private *sp = bus_to_subsys(drv->bus);
	struct driver_private *priv;
	int error = 0;

	if (!sp)
		return -EINVAL;
		....
		
	if (sp->drivers_autoprobe) {
		error = driver_attach(drv);
		if (error)
			goto out_del_list;
	}
...
}

int driver_attach(const struct device_driver *drv)
{
return bus_for_each_dev(drv->bus, NULL, (void *)drv, __driver_attach);
}

int bus_for_each_dev(const struct bus_type *bus, struct device *start,
		     void *data, int (*fn)(struct device *, void *))
{
	struct subsys_private *sp = bus_to_subsys(bus);
	struct klist_iter i;
	struct device *dev;
	int error = 0;

	if (!sp)
		return -EINVAL;   // this is the only condition for
                                  // driver_attach() to return error.

	klist_iter_init_node(&sp->klist_devices, &i,
			     (start ? &start->p->knode_bus : NULL));
	while (!error && (dev = next_device(&i)))
		error = fn(dev, data);
	klist_iter_exit(&i);
	subsys_put(sp);
	return error;
}

int __driver_attach(struct device *dev, void *data)() will always
return 0 even if it has int as return value type.

> thanks,
> 
> greg k-h

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

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

[Greg Kroah-Hartman]

On Sun, Sep 15, 2024 at 10:36:44PM +0800, Zijun Hu wrote:
> 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@quicinc.com>
> >>>>
> >>>> 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.

Yes, and they should be removed, we have someone that normally sweeps
the tree every few years and runs a tool that can compare if those
actually are correct or not, and usually not.

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

Nope, sorry, performance only.

thanks,

greg k-h