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

[Zijun Hu <zijun_hu@icloud.com>]

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