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

Zijun Hu
2024-09-15T14:15:57Z

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