Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-26 Thread Yijing Wang 
On 2013/4/26 14:20, Yinghai Lu wrote:
> On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang  wrote:
>> Hi Yinghai,
>>We should not remove this additional pci_disable_device().
>> Because we enable pcie port device twice before. The first is 
>> pci_enable_brides(),
>> in x86, it was called in pci_assign_unassigned_resources(). The second in 
>> pcie_port_device_register().
>> So we should call pci_disable_device() twice for pci_dev->enable_cnt balance.
>>
>> But there is still a problem here. If we unbind a pcie port device pcie port 
>> driver, we can not
>> use its child devices again, because this pcie port device was disabled 
>> absolutely.
>>
>> So I think we should move the second pci_disable_device() to remove.c.
>>
>> I sent this patch to Bjorn and following is Bjorn reply
>> "And it's not clear to me whether unbinding the
>> pcie port driver should disable the bridge at all.  I think one could
>> argue that the bridge should remain functional even if the driver is
>> unloaded, because the PCI core *enables* the bridge even if the driver
>> is never loaded."
>>
>> Yinghai, how do you think about this issue?
> 

Hi Yinghai,
   Thanks for your comment!
We enable_bridges in PCI core code, so I think we should disable device in 
remove.c(PCI core level),
another reason is call second pci_disable_device() in pci_stop_bus_device() is 
safe, because all child device
has been stopped(unbind driver already).

> 1. we always enable bridges after assign unassigned resource for boot path
> and hotplug path.
> we should never call disable for that.

I agree "we should never call second disable" unless we stop this sub 
pci-tree().

Maybe the attached patch last letter is not safe enough, should wait pci bridge 
complete
to stop itself, then call the second pci_disable_device().

> 
> 2. driver should be keep enable/disable during probe/remove

I agree, use enable/disable balance is better.

> 
> looks like we need to rethink pci enable bridge.
> 
> if we want to enable one pci device, we should go up  to enable all bridges 
> till
> root.

Yes, now we enable pci bridges from root to end. like in 
pci_assign_unassigned_resources().

> 
> let if we disable one pci device, we need to go up to disable bridge if its 
> all
> pci device children get disabled.

Yes, This is what I think too. It seems like we only can do this in remove.c

> 
> if there is pci driver is bound with bridge device, those
> disable/enable bridge should be skipped.

Hmm, currently system achieve this by checking pci_dev->enable_cnt.

> 
> Thanks
> 
> Yinghai
> 
> .
> 


-- 
Thanks!
Yijing

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-26 Thread Yinghai Lu 
On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang  wrote:
> Hi Yinghai,
>We should not remove this additional pci_disable_device().
> Because we enable pcie port device twice before. The first is 
> pci_enable_brides(),
> in x86, it was called in pci_assign_unassigned_resources(). The second in 
> pcie_port_device_register().
> So we should call pci_disable_device() twice for pci_dev->enable_cnt balance.
>
> But there is still a problem here. If we unbind a pcie port device pcie port 
> driver, we can not
> use its child devices again, because this pcie port device was disabled 
> absolutely.
>
> So I think we should move the second pci_disable_device() to remove.c.
>
> I sent this patch to Bjorn and following is Bjorn reply
> "And it's not clear to me whether unbinding the
> pcie port driver should disable the bridge at all.  I think one could
> argue that the bridge should remain functional even if the driver is
> unloaded, because the PCI core *enables* the bridge even if the driver
> is never loaded."
>
> Yinghai, how do you think about this issue?

1. we always enable bridges after assign unassigned resource for boot path
and hotplug path.
we should never call disable for that.

2. driver should be keep enable/disable during probe/remove

looks like we need to rethink pci enable bridge.

if we want to enable one pci device, we should go up  to enable all bridges till
root.

let if we disable one pci device, we need to go up to disable bridge if its all
pci device children get disabled.

if there is pci driver is bound with bridge device, those
disable/enable bridge should be skipped.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-26 Thread Yinghai Lu 
On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang wangyij...@huawei.com wrote:
 Hi Yinghai,
We should not remove this additional pci_disable_device().
 Because we enable pcie port device twice before. The first is 
 pci_enable_brides(),
 in x86, it was called in pci_assign_unassigned_resources(). The second in 
 pcie_port_device_register().
 So we should call pci_disable_device() twice for pci_dev-enable_cnt balance.

 But there is still a problem here. If we unbind a pcie port device pcie port 
 driver, we can not
 use its child devices again, because this pcie port device was disabled 
 absolutely.

 So I think we should move the second pci_disable_device() to remove.c.

 I sent this patch to Bjorn and following is Bjorn reply
 And it's not clear to me whether unbinding the
 pcie port driver should disable the bridge at all.  I think one could
 argue that the bridge should remain functional even if the driver is
 unloaded, because the PCI core *enables* the bridge even if the driver
 is never loaded.

 Yinghai, how do you think about this issue?

1. we always enable bridges after assign unassigned resource for boot path
and hotplug path.
we should never call disable for that.

2. driver should be keep enable/disable during probe/remove

looks like we need to rethink pci enable bridge.

if we want to enable one pci device, we should go up  to enable all bridges till
root.

let if we disable one pci device, we need to go up to disable bridge if its all
pci device children get disabled.

if there is pci driver is bound with bridge device, those
disable/enable bridge should be skipped.

Thanks

Yinghai
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-26 Thread Yijing Wang 
On 2013/4/26 14:20, Yinghai Lu wrote:
 On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang wangyij...@huawei.com wrote:
 Hi Yinghai,
We should not remove this additional pci_disable_device().
 Because we enable pcie port device twice before. The first is 
 pci_enable_brides(),
 in x86, it was called in pci_assign_unassigned_resources(). The second in 
 pcie_port_device_register().
 So we should call pci_disable_device() twice for pci_dev-enable_cnt balance.

 But there is still a problem here. If we unbind a pcie port device pcie port 
 driver, we can not
 use its child devices again, because this pcie port device was disabled 
 absolutely.

 So I think we should move the second pci_disable_device() to remove.c.

 I sent this patch to Bjorn and following is Bjorn reply
 And it's not clear to me whether unbinding the
 pcie port driver should disable the bridge at all.  I think one could
 argue that the bridge should remain functional even if the driver is
 unloaded, because the PCI core *enables* the bridge even if the driver
 is never loaded.

 Yinghai, how do you think about this issue?
 

Hi Yinghai,
   Thanks for your comment!
We enable_bridges in PCI core code, so I think we should disable device in 
remove.c(PCI core level),
another reason is call second pci_disable_device() in pci_stop_bus_device() is 
safe, because all child device
has been stopped(unbind driver already).

 1. we always enable bridges after assign unassigned resource for boot path
 and hotplug path.
 we should never call disable for that.

I agree we should never call second disable unless we stop this sub 
pci-tree().

Maybe the attached patch last letter is not safe enough, should wait pci bridge 
complete
to stop itself, then call the second pci_disable_device().

 
 2. driver should be keep enable/disable during probe/remove

I agree, use enable/disable balance is better.

 
 looks like we need to rethink pci enable bridge.
 
 if we want to enable one pci device, we should go up  to enable all bridges 
 till
 root.

Yes, now we enable pci bridges from root to end. like in 
pci_assign_unassigned_resources().

 
 let if we disable one pci device, we need to go up to disable bridge if its 
 all
 pci device children get disabled.

Yes, This is what I think too. It seems like we only can do this in remove.c

 
 if there is pci driver is bound with bridge device, those
 disable/enable bridge should be skipped.

Hmm, currently system achieve this by checking pci_dev-enable_cnt.

 
 Thanks
 
 Yinghai
 
 .
 


-- 
Thanks!
Yijing

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

Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-25 Thread Yijing Wang 
Hi Yinghai,
   We should not remove this additional pci_disable_device().
Because we enable pcie port device twice before. The first is 
pci_enable_brides(),
in x86, it was called in pci_assign_unassigned_resources(). The second in 
pcie_port_device_register().
So we should call pci_disable_device() twice for pci_dev->enable_cnt balance.

But there is still a problem here. If we unbind a pcie port device pcie port 
driver, we can not
use its child devices again, because this pcie port device was disabled 
absolutely.

So I think we should move the second pci_disable_device() to remove.c.

I sent this patch to Bjorn and following is Bjorn reply
"And it's not clear to me whether unbinding the
pcie port driver should disable the bridge at all.  I think one could
argue that the bridge should remain functional even if the driver is
unloaded, because the PCI core *enables* the bridge even if the driver
is never loaded."

Yinghai, how do you think about this issue?



On 2013/4/26 9:47, Yinghai Lu wrote:
> During chasing one PCI xHCI hotplug problem, David Bulkow found
> 
>   static void pcie_portdrv_remove(struct pci_dev *dev)
>   {
>   pcie_port_device_remove(dev);
>   pci_disable_device(dev);
>   }
> and
>   void pcie_port_device_remove(struct pci_dev *dev)
>   {
>   device_for_each_child(>dev, NULL, remove_iter);
>   cleanup_service_irqs(dev);
>   pci_disable_device(dev);
>   }
> 
> that extra pci_disable_device in pcie_port_device_remove() was added by
> | commit dc5351784eb36f1fec4efa88e01581be72c0b711
> | Author: Kenji Kaneshige 
> | Date:   Wed Nov 25 21:04:00 2009 +0900
> |
> |PCI: portdrv: cleanup service irqs initialization
> 
> so pci_dsiable_device is called two times.
> 
> We should remove extra one in pcie_portdrv_remove.
> 
> Reported-by: David Bulkow 
> Signed-off-by: Yinghai Lu 
> 
> ---
>  drivers/pci/pcie/portdrv_pci.c |1 -
>  1 file changed, 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pcie/portdrv_pci.c
> ===
> --- linux-2.6.orig/drivers/pci/pcie/portdrv_pci.c
> +++ linux-2.6/drivers/pci/pcie/portdrv_pci.c
> @@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci
>  static void pcie_portdrv_remove(struct pci_dev *dev)
>  {
>   pcie_port_device_remove(dev);
> - pci_disable_device(dev);
>  }
>  
>  static int error_detected_iter(struct device *device, void *data)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> .
> 


-- 
Thanks!
Yijing
>From 44914e0e39dbe51e1a932492d6b4909d5967308e Mon Sep 17 00:00:00 2001
From: Yijing Wang 
Date: Tue, 16 Apr 2013 11:41:47 +0800
Subject: [PATCH] PCI: move second pci_disable_device into pci_stop_bus_device() for symmetry

Currently, we enable and disable pcie port device is not symmetrical. If
we unbind the pcie port driver for pcie port device, we will call pci_disable_device()
twice. Then the pcie port device is disabled, if there are some child devices
under it, the child device maybe cannot transmit data anymore. This patch move the
second pci_disable_device() int pci_stop_bus_device() to avoid this bug.

Signed-off-by: Yijing Wang 
---
 drivers/pci/pcie/portdrv_pci.c |1 -
 drivers/pci/remove.c   |1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index ed4d094..2ca1a0b 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
 	pcie_port_device_remove(dev);
-	pci_disable_device(dev);
 }
 
 static int error_detected_iter(struct device *device, void *data)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index cc875e6..e8f7c3c 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -73,6 +73,7 @@ static void pci_stop_bus_device(struct pci_dev *dev)
 		list_for_each_entry_safe_reverse(child, tmp,
 		 >devices, bus_list)
 			pci_stop_bus_device(child);
+			pci_disable_device(dev);
 	}
 
 	pci_stop_dev(dev);
-- 
1.7.1

[PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-25 Thread Yinghai Lu 
During chasing one PCI xHCI hotplug problem, David Bulkow found

static void pcie_portdrv_remove(struct pci_dev *dev)
{
pcie_port_device_remove(dev);
pci_disable_device(dev);
}
and
void pcie_port_device_remove(struct pci_dev *dev)
{
device_for_each_child(>dev, NULL, remove_iter);
cleanup_service_irqs(dev);
pci_disable_device(dev);
}

that extra pci_disable_device in pcie_port_device_remove() was added by
| commit dc5351784eb36f1fec4efa88e01581be72c0b711
| Author: Kenji Kaneshige 
| Date:   Wed Nov 25 21:04:00 2009 +0900
|
|PCI: portdrv: cleanup service irqs initialization

so pci_dsiable_device is called two times.

We should remove extra one in pcie_portdrv_remove.

Reported-by: David Bulkow 
Signed-off-by: Yinghai Lu 

---
 drivers/pci/pcie/portdrv_pci.c |1 -
 1 file changed, 1 deletion(-)

Index: linux-2.6/drivers/pci/pcie/portdrv_pci.c
===
--- linux-2.6.orig/drivers/pci/pcie/portdrv_pci.c
+++ linux-2.6/drivers/pci/pcie/portdrv_pci.c
@@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
pcie_port_device_remove(dev);
-   pci_disable_device(dev);
 }
 
 static int error_detected_iter(struct device *device, void *data)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-25 Thread Yinghai Lu 
During chasing one PCI xHCI hotplug problem, David Bulkow found

static void pcie_portdrv_remove(struct pci_dev *dev)
{
pcie_port_device_remove(dev);
pci_disable_device(dev);
}
and
void pcie_port_device_remove(struct pci_dev *dev)
{
device_for_each_child(dev-dev, NULL, remove_iter);
cleanup_service_irqs(dev);
pci_disable_device(dev);
}

that extra pci_disable_device in pcie_port_device_remove() was added by
| commit dc5351784eb36f1fec4efa88e01581be72c0b711
| Author: Kenji Kaneshige kaneshige.ke...@jp.fujitsu.com
| Date:   Wed Nov 25 21:04:00 2009 +0900
|
|PCI: portdrv: cleanup service irqs initialization

so pci_dsiable_device is called two times.

We should remove extra one in pcie_portdrv_remove.

Reported-by: David Bulkow david.bul...@stratus.com
Signed-off-by: Yinghai Lu ying...@kernel.org

---
 drivers/pci/pcie/portdrv_pci.c |1 -
 1 file changed, 1 deletion(-)

Index: linux-2.6/drivers/pci/pcie/portdrv_pci.c
===
--- linux-2.6.orig/drivers/pci/pcie/portdrv_pci.c
+++ linux-2.6/drivers/pci/pcie/portdrv_pci.c
@@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
pcie_port_device_remove(dev);
-   pci_disable_device(dev);
 }
 
 static int error_detected_iter(struct device *device, void *data)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PCI: Remove duplicate pci_disable_device for pcie port

2013-04-25 Thread Yijing Wang 
Hi Yinghai,
   We should not remove this additional pci_disable_device().
Because we enable pcie port device twice before. The first is 
pci_enable_brides(),
in x86, it was called in pci_assign_unassigned_resources(). The second in 
pcie_port_device_register().
So we should call pci_disable_device() twice for pci_dev-enable_cnt balance.

But there is still a problem here. If we unbind a pcie port device pcie port 
driver, we can not
use its child devices again, because this pcie port device was disabled 
absolutely.

So I think we should move the second pci_disable_device() to remove.c.

I sent this patch to Bjorn and following is Bjorn reply
And it's not clear to me whether unbinding the
pcie port driver should disable the bridge at all.  I think one could
argue that the bridge should remain functional even if the driver is
unloaded, because the PCI core *enables* the bridge even if the driver
is never loaded.

Yinghai, how do you think about this issue?



On 2013/4/26 9:47, Yinghai Lu wrote:
 During chasing one PCI xHCI hotplug problem, David Bulkow found
 
   static void pcie_portdrv_remove(struct pci_dev *dev)
   {
   pcie_port_device_remove(dev);
   pci_disable_device(dev);
   }
 and
   void pcie_port_device_remove(struct pci_dev *dev)
   {
   device_for_each_child(dev-dev, NULL, remove_iter);
   cleanup_service_irqs(dev);
   pci_disable_device(dev);
   }
 
 that extra pci_disable_device in pcie_port_device_remove() was added by
 | commit dc5351784eb36f1fec4efa88e01581be72c0b711
 | Author: Kenji Kaneshige kaneshige.ke...@jp.fujitsu.com
 | Date:   Wed Nov 25 21:04:00 2009 +0900
 |
 |PCI: portdrv: cleanup service irqs initialization
 
 so pci_dsiable_device is called two times.
 
 We should remove extra one in pcie_portdrv_remove.
 
 Reported-by: David Bulkow david.bul...@stratus.com
 Signed-off-by: Yinghai Lu ying...@kernel.org
 
 ---
  drivers/pci/pcie/portdrv_pci.c |1 -
  1 file changed, 1 deletion(-)
 
 Index: linux-2.6/drivers/pci/pcie/portdrv_pci.c
 ===
 --- linux-2.6.orig/drivers/pci/pcie/portdrv_pci.c
 +++ linux-2.6/drivers/pci/pcie/portdrv_pci.c
 @@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci
  static void pcie_portdrv_remove(struct pci_dev *dev)
  {
   pcie_port_device_remove(dev);
 - pci_disable_device(dev);
  }
  
  static int error_detected_iter(struct device *device, void *data)
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 
 .
 


-- 
Thanks!
Yijing
From 44914e0e39dbe51e1a932492d6b4909d5967308e Mon Sep 17 00:00:00 2001
From: Yijing Wang wangyij...@huawei.com
Date: Tue, 16 Apr 2013 11:41:47 +0800
Subject: [PATCH] PCI: move second pci_disable_device into pci_stop_bus_device() for symmetry

Currently, we enable and disable pcie port device is not symmetrical. If
we unbind the pcie port driver for pcie port device, we will call pci_disable_device()
twice. Then the pcie port device is disabled, if there are some child devices
under it, the child device maybe cannot transmit data anymore. This patch move the
second pci_disable_device() int pci_stop_bus_device() to avoid this bug.

Signed-off-by: Yijing Wang wangyij...@huawei.com
---
 drivers/pci/pcie/portdrv_pci.c |1 -
 drivers/pci/remove.c   |1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index ed4d094..2ca1a0b 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
 	pcie_port_device_remove(dev);
-	pci_disable_device(dev);
 }
 
 static int error_detected_iter(struct device *device, void *data)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index cc875e6..e8f7c3c 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -73,6 +73,7 @@ static void pci_stop_bus_device(struct pci_dev *dev)
 		list_for_each_entry_safe_reverse(child, tmp,
 		 bus-devices, bus_list)
 			pci_stop_bus_device(child);
+			pci_disable_device(dev);
 	}
 
 	pci_stop_dev(dev);
-- 
1.7.1