* Greg KH <gregkh@xxxxxxx>: > On Thu, Aug 21, 2008 at 02:25:04PM -0600, Matthew Wilcox wrote: > > On Thu, Aug 21, 2008 at 02:19:18PM -0600, Alex Chiang wrote: > > > #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL) > > > > > > That eventually calls pci_get_dev_by_id(), which increases the > > > refcount on the device, but never decrements it. > > > > > > Looks like that change in behavior happened here: > > > > > > PCI: clean up search.c a lot > > > 95247b57ed844511a212265b45cf9a919753aea1 > > > > > > pci_get_device() used to decrement the refcount, but no longer > > > does. > > > > > > Thanks to Matthew Wilcox for helping me get this far... > > > > > > Like I said, I'm still trying to track down my particular issue, > > > but I'd like to get your opinion on this. > > > > In particular, I'd like to know whether this should be fixed by > > pci_get_dev_by_id() decrementing the refcount of from/dev_start, > > pci_get_subsys() decrementing 'from', or by bus_find_device() > > decrementing 'start'. It looks like bus_find_device() is the place > > where this should logically happen, but the kerneldoc doesn't document > > the intended behaviour. > > Ah, no the driver core isn't supposed to do this, it's something the pci > functions do out of "niceness" as that's how we can use them in an > iterator properly. > > Does the following (untested) patch fix the issue for you all? Perfect, yes. I applied it, and observed the refcount on the device I was using to debug this problem. I was able to modprobe acpiphp successfully, and echo 0 into the device's 'power' file. I then watched the rest of the hotplug core do its thing, decrementing the refcount properly along the way, and at the end, we did call pci_release_dev(), as I originally expected to. :) Just to verify, I toggled the device's power a few times (echoing a 1 and then a 0, etc.) and watched the refcounts. After each offline, we properly called pci_release_dev(). Thanks for fixing this. It fixes a pretty bad leak in the hotplug core (we were leaking an entire struct pci_dev * # of functions for each offlined card, the first time around; subsequent onlines/offlines were ok). Tested-by: Alex Chiang <achiang@xxxxxx> /ac > > thanks, > > greg k-h > > -------------- > Subject: PCI: fix reference leak in pci_get_dev_by_id() > > From: Greg Kroah-Hartman <gregkh@xxxxxxx> > > Alex Chiang and Matthew Wilcox pointed out that pci_get_dev_by_id() does > not properly decrement the reference on the from pointer if it is > present, like the documentation for the function states it will. > > Cc: Matthew Wilcox <matthew@xxxxxx> > Cc: Alex Chiang <achiang@xxxxxx> > Cc: stable <stable@xxxxxxxxxx> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx> > > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index 217814f..3b3b5f1 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -280,6 +280,8 @@ static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id, > match_pci_dev_by_id); > if (dev) > pdev = to_pci_dev(dev); > + if (from) > + pci_dev_put(from); > return pdev; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
