| Message ID | 51925136.5050302@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: > On 05/14/2013 04:26 PM, Gu Zheng wrote: > I suggest to use pci_release_dev() instead because it also needs to > release OF related resources. > I will update it in next version. > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index bc075a3..2ac6338 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus > *bus > pci_set_of_node(dev); > > if (pci_setup_device(dev)) { > - kfree(dev); > + pci_release_dev(&dev->dev); > return NULL; no, should move pci_set_of_node calling into pci_setup_device. 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 Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote: > On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: >> On 05/14/2013 04:26 PM, Gu Zheng wrote: >> I suggest to use pci_release_dev() instead because it also needs to >> release OF related resources. >> I will update it in next version. >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index bc075a3..2ac6338 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus >> *bus >> pci_set_of_node(dev); >> >> if (pci_setup_device(dev)) { >> - kfree(dev); >> + pci_release_dev(&dev->dev); >> return NULL; > > no, should move pci_set_of_node calling into pci_setup_device. > > Yinghai I'm not sure whether we should call pci_set_of_node() for SR-IOV devices too, any suggestions here? -- 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 14, 2013 at 9:57 AM, Liu Jiang <liuj97@gmail.com> wrote: > On Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote: >> >> On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: >>> >>> On 05/14/2013 04:26 PM, Gu Zheng wrote: >>> I suggest to use pci_release_dev() instead because it also needs to >>> release OF related resources. >>> I will update it in next version. >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index bc075a3..2ac6338 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct >>> pci_bus >>> *bus >>> pci_set_of_node(dev); >>> >>> if (pci_setup_device(dev)) { >>> - kfree(dev); >>> + pci_release_dev(&dev->dev); >>> return NULL; >> >> >> no, should move pci_set_of_node calling into pci_setup_device. >> >> Yinghai > > > I'm not sure whether we should call pci_set_of_node() for SR-IOV devices > too, > any suggestions here? or just move down pci_set_of_node after pci_setup_device? anyway that is another bug. 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 Wed 15 May 2013 02:52:51 AM CST, Yinghai Lu wrote: > On Tue, May 14, 2013 at 9:57 AM, Liu Jiang <liuj97@gmail.com> wrote: >> On Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote: >>> >>> On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: >>>> >>>> On 05/14/2013 04:26 PM, Gu Zheng wrote: >>>> I suggest to use pci_release_dev() instead because it also needs to >>>> release OF related resources. >>>> I will update it in next version. >>>> >>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>> index bc075a3..2ac6338 100644 >>>> --- a/drivers/pci/probe.c >>>> +++ b/drivers/pci/probe.c >>>> @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct >>>> pci_bus >>>> *bus >>>> pci_set_of_node(dev); >>>> >>>> if (pci_setup_device(dev)) { >>>> - kfree(dev); >>>> + pci_release_dev(&dev->dev); >>>> return NULL; >>> >>> >>> no, should move pci_set_of_node calling into pci_setup_device. >>> >>> Yinghai >> >> >> I'm not sure whether we should call pci_set_of_node() for SR-IOV devices >> too, >> any suggestions here? > > or just move down pci_set_of_node after pci_setup_device? > > anyway that is another bug. > > Yinghai I'm not familiar with the OF logic and can't make sure whether pci_setup_device() has dependency on dev->of_node. Feel it's more safe to call pci_release_of_node() on failing path instead of tuning call-site of pci_set_of_node(). -- 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 Wed, May 15, 2013 at 7:39 AM, Liu Jiang <liuj97@gmail.com> wrote: > On Wed 15 May 2013 02:52:51 AM CST, Yinghai Lu wrote: >> >> On Tue, May 14, 2013 at 9:57 AM, Liu Jiang <liuj97@gmail.com> wrote: >>> >>> On Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote: >>>> >>>> >>>> On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: >>>>> >>>>> >>>>> On 05/14/2013 04:26 PM, Gu Zheng wrote: >>>>> I suggest to use pci_release_dev() instead because it also needs >>>>> to >>>>> release OF related resources. >>>>> I will update it in next version. >>>>> >>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>>> index bc075a3..2ac6338 100644 >>>>> --- a/drivers/pci/probe.c >>>>> +++ b/drivers/pci/probe.c >>>>> @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct >>>>> pci_bus >>>>> *bus >>>>> pci_set_of_node(dev); >>>>> >>>>> if (pci_setup_device(dev)) { >>>>> - kfree(dev); >>>>> + pci_release_dev(&dev->dev); >>>>> return NULL; >>>> >>>> >>>> >>>> no, should move pci_set_of_node calling into pci_setup_device. >>>> >>>> Yinghai >>> >>> >>> >>> I'm not sure whether we should call pci_set_of_node() for SR-IOV devices >>> too, >>> any suggestions here? >> >> >> or just move down pci_set_of_node after pci_setup_device? >> >> anyway that is another bug. > I'm not familiar with the OF logic and can't make sure whether > pci_setup_device() > has dependency on dev->of_node. Feel it's more safe to call > pci_release_of_node() > on failing path instead of tuning call-site of pci_set_of_node(). that is another bug, let of guy handle it. 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 Wed 15 May 2013 10:43:02 PM CST, Yinghai Lu wrote: > On Wed, May 15, 2013 at 7:39 AM, Liu Jiang <liuj97@gmail.com> wrote: >> On Wed 15 May 2013 02:52:51 AM CST, Yinghai Lu wrote: >>> >>> On Tue, May 14, 2013 at 9:57 AM, Liu Jiang <liuj97@gmail.com> wrote: >>>> >>>> On Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote: >>>>> >>>>> >>>>> On Tue, May 14, 2013 at 7:59 AM, Liu Jiang <liuj97@gmail.com> wrote: >>>>>> >>>>>> >>>>>> On 05/14/2013 04:26 PM, Gu Zheng wrote: >>>>>> I suggest to use pci_release_dev() instead because it also needs >>>>>> to >>>>>> release OF related resources. >>>>>> I will update it in next version. >>>>>> >>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>>>> index bc075a3..2ac6338 100644 >>>>>> --- a/drivers/pci/probe.c >>>>>> +++ b/drivers/pci/probe.c >>>>>> @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct >>>>>> pci_bus >>>>>> *bus >>>>>> pci_set_of_node(dev); >>>>>> >>>>>> if (pci_setup_device(dev)) { >>>>>> - kfree(dev); >>>>>> + pci_release_dev(&dev->dev); >>>>>> return NULL; >>>>> >>>>> >>>>> >>>>> no, should move pci_set_of_node calling into pci_setup_device. >>>>> >>>>> Yinghai >>>> >>>> >>>> >>>> I'm not sure whether we should call pci_set_of_node() for SR-IOV devices >>>> too, >>>> any suggestions here? >>> >>> >>> or just move down pci_set_of_node after pci_setup_device? >>> >>> anyway that is another bug. > >> I'm not familiar with the OF logic and can't make sure whether >> pci_setup_device() >> has dependency on dev->of_node. Feel it's more safe to call >> pci_release_of_node() >> on failing path instead of tuning call-site of pci_set_of_node(). > > that is another bug, let of guy handle it. > > Yinghai Hi Yinghai, I don't know any OF exports, could you please help to CC some OF experts? Thanks, Gerry -- 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 Wed, May 15, 2013 at 7:46 AM, Liu Jiang <liuj97@gmail.com> wrote: > On Wed 15 May 2013 10:43:02 PM CST, Yinghai Lu wrote: >> > >> that is another bug, let of guy handle it. >> >> Yinghai > > Hi Yinghai, > I don't know any OF exports, could you please help to CC > some OF experts? powerpc and sparc are using that. Ben, in drivers/pci/probe.c::pci_scan_device() there is pci_set_of_node(dev); if (pci_setup_device(dev)) { kfree(dev); return NULL; } so if pci_setup_device fails, there is one dev reference is not release. please check you can just move down pci_set_of_node down after that failing path, like if (pci_setup_device(dev)) { kfree(dev); return NULL; } pci_set_of_node(dev); 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 Wed, 2013-05-15 at 22:46 +0800, Liu Jiang wrote: > I don't know any OF exports, could you please help to CC > some OF experts? I wrote that code I think. Sorry, I've missed the beginning of the thread, what is the problem ? Cheers, Ben. -- 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 Wed, 2013-05-15 at 07:58 -0700, Yinghai Lu wrote: > Ben, > > in drivers/pci/probe.c::pci_scan_device() there is > > pci_set_of_node(dev); > > if (pci_setup_device(dev)) { > kfree(dev); > return NULL; > } > > so if pci_setup_device fails, there is one dev reference is not release. > > please check you can just move down pci_set_of_node down after that > failing path, like > > > if (pci_setup_device(dev)) { > kfree(dev); > return NULL; > } > > pci_set_of_node(dev); No, we want the OF node set when we run the quirks, we intentionally do that early, the right thing to do is to to call pci_release_of_node() in the error path (it's safe to call even if the node is NULL). Cheers, Ben. -- 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 Wed, May 15, 2013 at 2:32 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Wed, 2013-05-15 at 07:58 -0700, Yinghai Lu wrote: > >> Ben, >> >> in drivers/pci/probe.c::pci_scan_device() there is >> >> pci_set_of_node(dev); >> >> if (pci_setup_device(dev)) { >> kfree(dev); >> return NULL; >> } >> >> so if pci_setup_device fails, there is one dev reference is not release. >> >> please check you can just move down pci_set_of_node down after that >> failing path, like >> >> >> if (pci_setup_device(dev)) { >> kfree(dev); >> return NULL; >> } >> >> pci_set_of_node(dev); > > No, we want the OF node set when we run the quirks, we intentionally do > that early, the right thing to do is to to call pci_release_of_node() > in the error path (it's safe to call even if the node is NULL). > Good. We have two options. 1. can you please submit one complete patch, and get it merged into v3.10. 2. put it together with pci_alloc_dev patchset towards to v3.11? 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 Thu 16 May 2013 05:29:31 AM CST, Benjamin Herrenschmidt wrote: > On Wed, 2013-05-15 at 22:46 +0800, Liu Jiang wrote: >> I don't know any OF exports, could you please help to CC >> some OF experts? > > I wrote that code I think. Sorry, I've missed the beginning of the > thread, what is the problem ? > > Cheers, > Ben. > > Hi, Just found a little memory leak issue that we should call pci_release_of_node() on error recovery path in function pci_scan_device(). pci_set_of_node(dev); if (pci_setup_device(dev)) { kfree(dev); return NULL; } Regards! Gerry -- 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/probe.c b/drivers/pci/probe.c index bc075a3..2ac6338 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1281,7 +1281,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus pci_set_of_node(dev); if (pci_setup_device(dev)) { - kfree(dev); + pci_release_dev(&dev->dev); return NULL; }