[PATCH] PCI: fix return in pci_bus_add_device

[PATCH] PCI: fix return in pci_bus_add_device

[Yijing Wang]

Fix device_attach() return vaule in pci_bus_add_device
instead of meaningless 0.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index ba2bf55..42b42b7 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -253,7 +253,7 @@ int pci_bus_add_device(struct pci_dev *dev)
 
 	dev->is_added = 1;
 
-	return 0;
+	return retval;
 }
 
 /**
-- 
1.7.1

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Bjorn Helgaas]

[+cc Yinghai]

On Thu, May 22, 2014 at 11:32:24AM +0800, Yijing Wang wrote:
> Fix device_attach() return vaule in pci_bus_add_device
> instead of meaningless 0.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/bus.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index ba2bf55..42b42b7 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -253,7 +253,7 @@ int pci_bus_add_device(struct pci_dev *dev)
>  
>  	dev->is_added = 1;
>  
> -	return 0;
> +	return retval;

I'd like to see a Reviewed-by: from Yinghai, since he recently changed
this area, e.g.,

  4f535093cf8f PCI: Put pci_dev in device tree as early as possible
  58d9a38f6fac PCI: Skip attaching driver in device_add()

Bjorn

>  }
>  
>  /**
> -- 
> 1.7.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yijing Wang]

On 2014/5/28 11:22, Bjorn Helgaas wrote:
> [+cc Yinghai]
> 
> On Thu, May 22, 2014 at 11:32:24AM +0800, Yijing Wang wrote:
>> Fix device_attach() return vaule in pci_bus_add_device
>> instead of meaningless 0.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/bus.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index ba2bf55..42b42b7 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -253,7 +253,7 @@ int pci_bus_add_device(struct pci_dev *dev)
>>  
>>  	dev->is_added = 1;
>>  
>> -	return 0;
>> +	return retval;
> 
> I'd like to see a Reviewed-by: from Yinghai, since he recently changed
> this area, e.g.,
> 
>   4f535093cf8f PCI: Put pci_dev in device tree as early as possible
>   58d9a38f6fac PCI: Skip attaching driver in device_add()

OK, Yinghai, can you look at this change ?

I found some kernel code still check this return value, and I also think return the
real retval make sense.

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);
	}



	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");
		}

Thanks!
Yijing.



>>  }
>>  
>>  /**
>> -- 
>> 1.7.1
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
Thanks!
Yijing

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yinghai Lu]

On Tue, May 27, 2014 at 8:32 PM, Yijing Wang <wangyijing@huawei.com> wrote:
> On 2014/5/28 11:22, Bjorn Helgaas wrote:
>> [+cc Yinghai]
>>
>> On Thu, May 22, 2014 at 11:32:24AM +0800, Yijing Wang wrote:
>>> Fix device_attach() return vaule in pci_bus_add_device
>>> instead of meaningless 0.
>>>
>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>> ---
>>>  drivers/pci/bus.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>>> index ba2bf55..42b42b7 100644
>>> --- a/drivers/pci/bus.c
>>> +++ b/drivers/pci/bus.c
>>> @@ -253,7 +253,7 @@ int pci_bus_add_device(struct pci_dev *dev)
>>>
>>>      dev->is_added = 1;
>>>
>>> -    return 0;
>>> +    return retval;
>>
>> I'd like to see a Reviewed-by: from Yinghai, since he recently changed
>> this area, e.g.,
>>
>>   4f535093cf8f PCI: Put pci_dev in device tree as early as possible
>>   58d9a38f6fac PCI: Skip attaching driver in device_add()
>
> OK, Yinghai, can you look at this change ?
>
> 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.

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

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yijing Wang]

>> 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

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yinghai Lu]

On Wed, May 28, 2014 at 6:37 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>> 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

I prefer this one. as pci_bus_add_device already have

>         WARN_ON(retval < 0);

There is no need to print warning message later again.

Thanks

Yinghai

Re: [PATCH] PCI: fix return in pci_bus_add_device

[Yijing Wang]

On 2014/5/29 11:36, Yinghai Lu wrote:
> On Wed, May 28, 2014 at 6:37 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>> 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
> 
> I prefer this one. as pci_bus_add_device already have
> 
>>         WARN_ON(retval < 0);
> 
> There is no need to print warning message later again.

OK, thanks for your suggestion.
I will send a patchset to clean the unnecessary check in kernel.

Hi Bjorn, please drop this patch.

Thanks!
Yijing.

> 
> 


-- 
Thanks!
Yijing