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

2013-05-15 Thread Liu Jiang

On Wed 15 May 2013 02:52:51 AM CST, Yinghai Lu wrote:

On Tue, May 14, 2013 at 9:57 AM, Liu Jiang liu...@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 liu...@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-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-15 Thread Liu Jiang

On Wed 15 May 2013 10:43:02 PM CST, Yinghai Lu wrote:

On Wed, May 15, 2013 at 7:39 AM, Liu Jiang liu...@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 liu...@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 liu...@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-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-15 Thread Liu Jiang

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-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-14 Thread Liu Jiang

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 liu...@gmail.com wrote:

From: Gu Zheng guz.f...@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  0x;
 dev-device = (l  16)  0x;

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.

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



hanks for your correction.:)

Best regards,
Gu


Yinghai





--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-14 Thread Liu Jiang

On Tue 14 May 2013 11:10:33 PM CST, Yinghai Lu wrote:

On Tue, May 14, 2013 at 7:59 AM, Liu Jiang liu...@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-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html