| Message ID | 1400729544-7940-1-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
[+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 -- 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
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 > >
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 -- 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
>> 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 > > . >
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 -- 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
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. > >
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; } /**
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(-)