| Message ID | 1333726296-3817-1-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Apr 6, 2012 at 9:31 AM, Jiang Liu <liuj97@gmail.com> wrote: > Function pci_get_dev_by_id() will hold a reference count on the pci device > returned, so pci_dev_present() should release the corresponding reference > count to avoid memory leakage. > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > --- > drivers/pci/search.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index 9d75dc8..b572730 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids) > WARN_ON(in_interrupt()); > while (ids->vendor || ids->subvendor || ids->class_mask) { > found = pci_get_dev_by_id(ids, NULL); > - if (found) > - goto exit; > + if (found) { > + pci_dev_put(found); > + return 1; > + } > ids++; > } > -exit: > - if (found) > - return 1; > + > return 0; > } > EXPORT_SYMBOL(pci_dev_present); This might be the right thing to do, but I'd like to understand what's going on, so let's talk about it a bit first. I agree, there appears to be a leak here. Or at least, the fact that we keep a reference when a device is found doesn't match the comment. What problem are you seeing from this leak? There are not many callers, and most appear to be one-time things done at boot, looking for built-in devices known to be defective. These devices won't be removable, so the leak shouldn't be causing hot-remove issues. IMO, this is a bogus interface that leads to poor code, and I don't want to encourage its use. For device defect workarounds, I think it'd be better to use PCI quirks to catch the defective device. Some chipset defects affect all downstream devices, and a quirk could make the defect visible to all the drivers, not just the ones that use pci_dev_present(). For example, look at tg3_write_reorder_chipsets and tg3_dma_wait_state_chipsets. Those aren't for tg3 bugs, they're for chipset bugs that might affect other devices, too. But right now, that knowledge is buried in the tg3 driver. Bjorn -- 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 04/07/2012 07:23 AM, Bjorn Helgaas wrote: > On Fri, Apr 6, 2012 at 9:31 AM, Jiang Liu <liuj97@gmail.com> wrote: >> Function pci_get_dev_by_id() will hold a reference count on the pci device >> returned, so pci_dev_present() should release the corresponding reference >> count to avoid memory leakage. >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> --- >> drivers/pci/search.c | 10 +++++----- >> 1 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pci/search.c b/drivers/pci/search.c >> index 9d75dc8..b572730 100644 >> --- a/drivers/pci/search.c >> +++ b/drivers/pci/search.c >> @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids) >> WARN_ON(in_interrupt()); >> while (ids->vendor || ids->subvendor || ids->class_mask) { >> found = pci_get_dev_by_id(ids, NULL); >> - if (found) >> - goto exit; >> + if (found) { >> + pci_dev_put(found); >> + return 1; >> + } >> ids++; >> } >> -exit: >> - if (found) >> - return 1; >> + >> return 0; >> } >> EXPORT_SYMBOL(pci_dev_present); > > This might be the right thing to do, but I'd like to understand what's > going on, so let's talk about it a bit first. > > I agree, there appears to be a leak here. Or at least, the fact that > we keep a reference when a device is found doesn't match the comment. > What problem are you seeing from this leak? > > There are not many callers, and most appear to be one-time things done > at boot, looking for built-in devices known to be defective. These > devices won't be removable, so the leak shouldn't be causing > hot-remove issues. I noticed this issue when reading code, no real issue disclosed yet. As you have pointed out, this interface is used for built-in devices only, there should be no real issue currently and the patch is just for purity. > > IMO, this is a bogus interface that leads to poor code, and I don't > want to encourage its use. For device defect workarounds, I think > it'd be better to use PCI quirks to catch the defective device. Some > chipset defects affect all downstream devices, and a quirk could make > the defect visible to all the drivers, not just the ones that use > pci_dev_present(). For example, look at tg3_write_reorder_chipsets > and tg3_dma_wait_state_chipsets. Those aren't for tg3 bugs, they're > for chipset bugs that might affect other devices, too. But right now, > that knowledge is buried in the tg3 driver. Thanks for pointing out the real issue behind this bogus interface. It would be better to solve this type of chip bugs in PCI core instead of in individual drivers. > > Bjorn -- 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 Sat, Apr 7, 2012 at 8:51 AM, Jiang Liu <liuj97@gmail.com> wrote: > On 04/07/2012 07:23 AM, Bjorn Helgaas wrote: >> On Fri, Apr 6, 2012 at 9:31 AM, Jiang Liu <liuj97@gmail.com> wrote: >>> Function pci_get_dev_by_id() will hold a reference count on the pci device >>> returned, so pci_dev_present() should release the corresponding reference >>> count to avoid memory leakage. >>> >>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >>> --- >>> drivers/pci/search.c | 10 +++++----- >>> 1 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c >>> index 9d75dc8..b572730 100644 >>> --- a/drivers/pci/search.c >>> +++ b/drivers/pci/search.c >>> @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids) >>> WARN_ON(in_interrupt()); >>> while (ids->vendor || ids->subvendor || ids->class_mask) { >>> found = pci_get_dev_by_id(ids, NULL); >>> - if (found) >>> - goto exit; >>> + if (found) { >>> + pci_dev_put(found); >>> + return 1; >>> + } >>> ids++; >>> } >>> -exit: >>> - if (found) >>> - return 1; >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL(pci_dev_present); >> >> This might be the right thing to do, but I'd like to understand what's >> going on, so let's talk about it a bit first. >> >> I agree, there appears to be a leak here. Or at least, the fact that >> we keep a reference when a device is found doesn't match the comment. >> What problem are you seeing from this leak? >> >> There are not many callers, and most appear to be one-time things done >> at boot, looking for built-in devices known to be defective. These >> devices won't be removable, so the leak shouldn't be causing >> hot-remove issues. > I noticed this issue when reading code, no real issue disclosed yet. > As you have pointed out, this interface is used for built-in devices only, > there should be no real issue currently and the patch is just for purity. > >> >> IMO, this is a bogus interface that leads to poor code, and I don't >> want to encourage its use. For device defect workarounds, I think >> it'd be better to use PCI quirks to catch the defective device. Some >> chipset defects affect all downstream devices, and a quirk could make >> the defect visible to all the drivers, not just the ones that use >> pci_dev_present(). For example, look at tg3_write_reorder_chipsets >> and tg3_dma_wait_state_chipsets. Those aren't for tg3 bugs, they're >> for chipset bugs that might affect other devices, too. But right now, >> that knowledge is buried in the tg3 driver. > Thanks for pointing out the real issue behind this bogus interface. > It would be better to solve this type of chip bugs in PCI core instead > of in individual drivers. I applied this to the -next branch, thanks. Bjorn -- 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
diff --git a/drivers/pci/search.c b/drivers/pci/search.c index 9d75dc8..b572730 100644 --- a/drivers/pci/search.c +++ b/drivers/pci/search.c @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids) WARN_ON(in_interrupt()); while (ids->vendor || ids->subvendor || ids->class_mask) { found = pci_get_dev_by_id(ids, NULL); - if (found) - goto exit; + if (found) { + pci_dev_put(found); + return 1; + } ids++; } -exit: - if (found) - return 1; + return 0; } EXPORT_SYMBOL(pci_dev_present);
Function pci_get_dev_by_id() will hold a reference count on the pci device returned, so pci_dev_present() should release the corresponding reference count to avoid memory leakage. Signed-off-by: Jiang Liu <jiang.liu@huawei.com> --- drivers/pci/search.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)