Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yijing Wang <wangyijing@huawei.com>]

>> I found some kernel code still check this return value, and I also think return the
>> real retval make sense.
> 
> No, that is not right.
> 
> If you look closely,
> 
> device_attach() returns
> 1: success
> 0: not attached
> <0: fail.

Hi Yinghai,
   Thanks for your explanation.
I found all the kernel code to check its return value, only for print a warning for users.


So I think we can drop all return checking from calling path, or rework pci_bus_add_device(),
return 0 if device_attach success, otherwise return -1, indicates the device not bound to a driver.

int pci_bus_add_device(struct pci_dev *dev)
{
	int retval;

	/*
	 * Can not put in pci_device_add yet because resources
	 * are not assigned yet for some devices.
	 */
	pci_fixup_device(pci_fixup_final, dev);
	pci_create_sysfs_dev_files(dev);
	pci_proc_attach_device(dev);

	dev->match_driver = true;
	retval = device_attach(&dev->dev);
	WARN_ON(retval < 0);

	dev->is_added = 1;

	return retval >=0 ? 0:-1;
}

Yinghai, which solution would you prefer?

Thanks!
Yijing.

> 
> so if you return ret directly, you have false warning from
> 
> drivers/edac/i82875p_edac.c:            err = pci_bus_add_device(dev);
> drivers/edac/i82875p_edac.c:                            "%s():
> pci_bus_add_device() Failed\n",
> drivers/platform/x86/asus-wmi.c:                                if
> (pci_bus_add_device(dev))
> drivers/platform/x86/eeepc-laptop.c:                            if
> (pci_bus_add_device(dev))
> 
> We already have
>     WARN_ON(retval < 0);
> 
> so please just remove all return checking from calling path.
> compiler already drop them...
> 
>>
>> eg.
>>
>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>                 /* Skip already-added devices */
>>                 if (dev->is_added)
>>                         continue;
>>                 retval = pci_bus_add_device(dev);
>>                 if (retval)
>>                         dev_err(&dev->dev, "Error adding device (%d)\n",
>>                                 retval);
> 
> should be dropped.
> 
>>         }
>>
>>
>>
>>         if (!blocked) {
>>                 dev = pci_get_slot(bus, 0);
>>                 if (dev) {
>>                         /* Device already present */
>>                         pci_dev_put(dev);
>>                         goto out_put_dev;
>>                 }
>>                 dev = pci_scan_single_device(bus, 0);
>>                 if (dev) {
>>                         pci_bus_assign_resources(bus);
>>                         if (pci_bus_add_device(dev))
>>                                 pr_err("Unable to hotplug wifi\n");
> oh, no, no one have that report. instead if should have trace printed out.
>>                 }
> 
> Thanks
> 
> Yinghai
> 
> .
> 


-- 
Thanks!
Yijing