diff mbox

[v2,part,1,3/9] PCI: Convert alloc_pci_dev(void) to pci_alloc_dev(bus) instead

Message ID 51925136.5050302@gmail.com
State Superseded
Headers show

Commit Message

Jiang Liu May 14, 2013, 2:59 p.m. UTC 
On 05/14/2013 04:26 PM, Gu Zheng wrote:
> On 05/14/2013 01:23 AM, Yinghai Lu wrote:
>
>> On Mon, May 13, 2013 at 9:08 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 4f0bc0a..bc075a3 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -1131,6 +1131,7 @@ static void pci_release_dev(struct device *dev)
>>>          struct pci_dev *pci_dev;
>>>
>>>          pci_dev = to_pci_dev(dev);
>>> +       pci_bus_put(pci_dev->bus);
>>>          pci_release_capabilities(pci_dev);
>>>          pci_release_of_node(pci_dev);
>>>          kfree(pci_dev);
>>> @@ -1269,11 +1270,10 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>>>          if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
>>>                  return NULL;
>>>
>>> -       dev = alloc_pci_dev();
>>> +       dev = pci_alloc_dev(bus);
>>>          if (!dev)
>>>                  return NULL;
>>>
>>> -       dev->bus = bus;
>>>          dev->devfn = devfn;
>>>          dev->vendor = l & 0xffff;
>>>          dev->device = (l >> 16) & 0xffff;
>> in pci_setup_device() fail path, it release the ref to that bus.
> Yes, you're right, we need to release the bus' ref if pci_setup_device() failed.
Hi Zheng,
     I suggest to use pci_release_dev() instead because it also needs to 
release OF related resources.
I will update it in next version.



> hanks for your correction.:)
>
> Best regards,
> Gu
>
>> 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

Comments

Yinghai Lu May 14, 2013, 3:10 p.m. UTC | #1 
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
Jiang Liu May 14, 2013, 4:57 p.m. UTC | #2 
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
Yinghai Lu May 14, 2013, 6:52 p.m. UTC | #3 
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
Jiang Liu May 15, 2013, 2:39 p.m. UTC | #4 
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
Yinghai Lu May 15, 2013, 2:43 p.m. UTC | #5 
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
Jiang Liu May 15, 2013, 2:46 p.m. UTC | #6 
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
Yinghai Lu May 15, 2013, 2:58 p.m. UTC | #7 
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
Benjamin Herrenschmidt May 15, 2013, 9:29 p.m. UTC | #8 
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
Benjamin Herrenschmidt May 15, 2013, 9:32 p.m. UTC | #9 
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
Yinghai Lu May 15, 2013, 9:52 p.m. UTC | #10 
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
Jiang Liu May 15, 2013, 11:46 p.m. UTC | #11 
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 mbox

Patch

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