Re: USB PCI quirk issue

Re: USB PCI quirk issue

[Sarah Sharp]

Cc-ing the public Linux PCI and USB mailing lists.

On Fri, Apr 12, 2013 at 02:59:29PM -0400, Bulkow, David wrote:
> Susan,

I'm Sarah. :)

> While testing Linux 3.9 we ran into an issue which I believe is a
> conflict between a couple of PCI changes.  Stratus has hardware that
> can hot add/remove chunks of its PCI hierarchy which has tickled some
> of the newer code.  I am mailing you because I believe the second
> change I list below holds the key.
> 
> I believe we are experiencing a collision between two changes.  The
> first:
> 
> PCI: Put pci_dev in device tree as early as possible
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
> 
> is causing device_add to be called during pci_scan_slot.  The second:
> 
> USB: Fix handoff when BIOS disables host PCI device
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=cab928ee1f221c9cc48d6615070fefe2e444384a
> 
> is getting activated by device_add.

So this system includes a Panther Point or Lynx Point xHCI host?

I'm running 3.9-rc6 with a Panther Point xHCI host, and it works fine,
so this is probably related to hot-plug, or perhaps a hardware-specific
issue.

> We are seeing complaints during pci_scan_slot like "BIOS handoff
> failed", "device not available... can't reserve IO" or mem for all USB
> devices.  Furthermore we are seeing the enable_cnt <= 0 warning,
> "disabling already-disabled device" in pci_disable_device during
> subsequent bringdown (or hot remove) operations.  We are using the
> protocol pci_scan_slot -> <assign IO/mem resources> ->
> pci_add_bus_resources to hot-add devices after they have been
> initialized.  I believe the first set of messages are because we have
> not yet assigned resources, but to do so the pci devices must be
> established - in pci_scan_slot.  The disable message is happening
> because the first failures during pci_scan_slot are over-decrementing
> enable_cnt in pci_enable_device_flags when do_pci_enable_device fails.

What are your BIOS settings for xHCI?  Some BIOSes have an option to
disable the xHCI host.  In that case, the BIOS won't respond to the xHCI
BIOS/OS handoff, and the xHCI PCI register that tells Linux which ports
are available to switch over from EHCI to xHCI will be all zeroes
(meaning we can't switch any ports over).

Originally, there was some talk of having the BIOS hide the PCI device
from the OS if the xHCI host was disabled in the BIOS.  I wonder if some
non-Intel BIOS vendor did that.

> What caught my eye in the second change, in usb quirk code, is that it
> does not differentiate between boot mode or hot-add operation. It is
> certainly possible we are doing PCI add incorrectly - things are
> moving quickly right now.

I don't understand why it should matter whether that code is run
directly after boot or on hot-add.  All that code does is read and write
the PCI MMIO registers, which should be available when the quirk is
called.

> Would you mind taking a look and correcting my understanding?

As for the other warnings, I think the PCI maintainer (Bjorn) is going
to have to address those, as I'm not a PCI expert.

Sarah Sharp

Re: USB PCI quirk issue

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Mon, Apr 15, 2013 at 11:26 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> Cc-ing the public Linux PCI and USB mailing lists.
>
> On Fri, Apr 12, 2013 at 02:59:29PM -0400, Bulkow, David wrote:
>> Susan,
>
> I'm Sarah. :)
>
>> While testing Linux 3.9 we ran into an issue which I believe is a
>> conflict between a couple of PCI changes.  Stratus has hardware that
>> can hot add/remove chunks of its PCI hierarchy which has tickled some
>> of the newer code.  I am mailing you because I believe the second
>> change I list below holds the key.
>>
>> I believe we are experiencing a collision between two changes.  The
>> first:
>>
>> PCI: Put pci_dev in device tree as early as possible
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
>>
>> is causing device_add to be called during pci_scan_slot.  The second:
>>
>> USB: Fix handoff when BIOS disables host PCI device
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=cab928ee1f221c9cc48d6615070fefe2e444384a
>>
>> is getting activated by device_add.

looks like we call quirk_final too early for hotadd path.

Please check if attached can workaround the problem.

Thanks

Yinghai

[-- Attachment #2: fix_qurik_final_hotadd.patch --]
[-- Type: application/octet-stream, Size: 753 bytes --]

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index bdc1e8b..1edffb7 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -201,6 +201,7 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 		/* Skip already-added devices */
 		if (dev->is_added)
 			continue;
+		pci_fixup_device(pci_fixup_final, dev);
 		retval = pci_bus_add_device(dev);
 	}
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 43ece5d..67cd045 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1341,7 +1341,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
 
-	pci_fixup_device(pci_fixup_final, dev);
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 

RE: USB PCI quirk issue

[Bulkow, David]

Excellent.  The quirk is no longer triggering resource issues.

This leaves an issue with dev->enable_cnt in pci_disable_device.  We
still get the message 'disabling already-disabled device'.  I was hoping
by not having the resource errors, enable_cnt would be more accurate.
This occurs after:

- clean boot
- hot remove devices (effectively half of machine's PCI devices)
- hot add devices (returned to service as described in original mail)
- hot remove devices *** this is where we get 'disabling
already-disabled device'

I'll dig into this tomorrow.  Thank you for this first step.

-----Original Message-----
From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
Yinghai Lu
Sent: Monday, April 15, 2013 4:41 PM
To: Sarah Sharp; Bulkow, David
Cc: Lawrence, Joe; linux-pci@vger.kernel.org; linux-usb@vger.kernel.org;
Bjorn Helgaas; Rafael J. Wysocki
Subject: Re: USB PCI quirk issue

On Mon, Apr 15, 2013 at 11:26 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> Cc-ing the public Linux PCI and USB mailing lists.
>
> On Fri, Apr 12, 2013 at 02:59:29PM -0400, Bulkow, David wrote:
>> Susan,
>
> I'm Sarah. :)
>
>> While testing Linux 3.9 we ran into an issue which I believe is a 
>> conflict between a couple of PCI changes.  Stratus has hardware that 
>> can hot add/remove chunks of its PCI hierarchy which has tickled some

>> of the newer code.  I am mailing you because I believe the second 
>> change I list below holds the key.
>>
>> I believe we are experiencing a collision between two changes.  The
>> first:
>>
>> PCI: Put pci_dev in device tree as early as possible
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commi
>> t/?id=4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
>>
>> is causing device_add to be called during pci_scan_slot.  The second:
>>
>> USB: Fix handoff when BIOS disables host PCI device 
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commi
>> t/?id=cab928ee1f221c9cc48d6615070fefe2e444384a
>>
>> is getting activated by device_add.

looks like we call quirk_final too early for hotadd path.

Please check if attached can workaround the problem.

Thanks

Yinghai

Re: USB PCI quirk issue

[Yinghai Lu]

[Adding Kenji]

On Wed, Apr 24, 2013 at 6:25 AM, Bulkow, David <David.Bulkow@stratus.com> wrote:
> By relocating the call to the quirk code, the resource problem is gone.
> Can we move forward on this change?

ok, will prepare one complete patch for that.

>
> The enable_cnt issue is now only associated with bridges managed by
> pcieport.
> For each bridge pci_disable_device() is getting called twice during
> hot-remove.
> This is happening because both pcie_portdrv_remove() and
> pcie_port_device_remove()
> call pci_disable_device().
>
> 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);
> }
>
> There are no ill effects from this so far but the warnings are
> not likely to be received well by customers.

Thanks for digging that out.

that extra pci_disable_device in pcie_port_device_remove() was added by
---
commit dc5351784eb36f1fec4efa88e01581be72c0b711
Author: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Date:   Wed Nov 25 21:04:00 2009 +0900

    PCI: portdrv: cleanup service irqs initialization

    This patch cleans up the service irqs initialization as follows:

     - Remove 'irq_mode' field in pcie_port_data and related definitions,
       which is not needed because we can get the same information from
       'is_msix', 'is_msi' and 'pin' fields in struct pci_dev.

     - Change the name of 'vectors' argument of assign_interrupt_mode() to
       'irqs' because it holds irq numbers actually. People might confuse
       it with CPU vector or MSI/MSI-X vector.

     - Change function name assign_interrupt_mode() to init_service_irqs()
       becasuse we no longer have 'irq_mode' data structure, and new name
       is more straightforward (IMO).
---

We should remove extra one in pcie_portdrv_remove.

Can you please prepare patch that delete that line in pcie_portdrv_remove() ?

Thanks

Yinghai

RE: USB PCI quirk issue

[Bulkow, David]

I removed the extra call to pci_disable_device in pcie_portdrv_remove.

The 'disabling already-disabled device' messages are gone and the
enable_cnt for bridges
survives bringdown/bringup cycles without going negative.  The only
anomaly I see right now
is that for a few of the bridges the enable_cnt is 2 after boot - before
hot adds or
removes - but this resolves itself after the first hot remove, probably
because
the dev is destroyed.

-----Original Message-----
From: yhlu.kernel@gmail.com [mailto:yhlu.kernel@gmail.com] On Behalf Of
Yinghai Lu
Sent: Wednesday, April 24, 2013 1:08 PM
To: Bulkow, David
Cc: Sarah Sharp; Lawrence, Joe; linux-pci@vger.kernel.org;
linux-usb@vger.kernel.org; Bjorn Helgaas; Rafael J. Wysocki; Kenji
Kaneshige
Subject: Re: USB PCI quirk issue

[Adding Kenji]

On Wed, Apr 24, 2013 at 6:25 AM, Bulkow, David
<David.Bulkow@stratus.com> wrote:
> By relocating the call to the quirk code, the resource problem is
gone.
> Can we move forward on this change?

ok, will prepare one complete patch for that.

>
> The enable_cnt issue is now only associated with bridges managed by 
> pcieport.
> For each bridge pci_disable_device() is getting called twice during 
> hot-remove.
> This is happening because both pcie_portdrv_remove() and
> pcie_port_device_remove()
> call pci_disable_device().
>
> 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);
> }
>
> There are no ill effects from this so far but the warnings are not 
> likely to be received well by customers.

Thanks for digging that out.

that extra pci_disable_device in pcie_port_device_remove() was added by
---
commit dc5351784eb36f1fec4efa88e01581be72c0b711
Author: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Date:   Wed Nov 25 21:04:00 2009 +0900

    PCI: portdrv: cleanup service irqs initialization

    This patch cleans up the service irqs initialization as follows:

     - Remove 'irq_mode' field in pcie_port_data and related
definitions,
       which is not needed because we can get the same information from
       'is_msix', 'is_msi' and 'pin' fields in struct pci_dev.

     - Change the name of 'vectors' argument of assign_interrupt_mode()
to
       'irqs' because it holds irq numbers actually. People might
confuse
       it with CPU vector or MSI/MSI-X vector.

     - Change function name assign_interrupt_mode() to
init_service_irqs()
       becasuse we no longer have 'irq_mode' data structure, and new
name
       is more straightforward (IMO).
---

We should remove extra one in pcie_portdrv_remove.

Can you please prepare patch that delete that line in
pcie_portdrv_remove() ?

Thanks

Yinghai

[PATCH] PCI: Remove duplicate pci_disable_device for pcie port

[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.kenji@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.Bulkow@stratus.com>
Signed-off-by: Yinghai Lu <yinghai@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)

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

[Yijing Wang]

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

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.kenji@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.Bulkow@stratus.com>
> Signed-off-by: Yinghai Lu <yinghai@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 majordomo@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

[-- Attachment #2: 0001-PCI-move-second-pci_disable_device-into-pci_stop_bus.patch --]
[-- Type: text/x-patch, Size: 1661 bytes --]

>From 44914e0e39dbe51e1a932492d6b4909d5967308e Mon Sep 17 00:00:00 2001
From: Yijing Wang <wangyijing@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 <wangyijing@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

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

[Yinghai Lu]

On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang <wangyijing@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

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

[Yijing Wang]

On 2013/4/26 14:20, Yinghai Lu wrote:
> On Thu, Apr 25, 2013 at 9:02 PM, Yijing Wang <wangyijing@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

[PATCH] PCI: move down pci_fixup_final for hotplug path

[Yinghai Lu]

David found some resource conflict issue after
| PCI: Put pci_dev in device tree as early as possible
| commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4

and
| USB: Fix handoff when BIOS disables host PCI device
| commit: cab928ee1f221c9cc48d6615070fefe2e444384a

for usb qirks for hotplug path.

After checking pci_fixup_device() with pci_fixup_final,
now we have different path for boot path and hotadd path.

Boot path: because pci_apply_fix_final_quirks is not set yet,
	so pci_fixup_device(pci_fixup_final) will be skipped
	from pci_device_add().
        And later pci_apply_final_quirks will be called for all
	pci devices via fs_initcall.
	That is after pci_assign_unassign resource.
	In that case quirk could use bars with problem.

Hotadd path: pci_fixup_device(pci_fixup_final) will be executed
	via pci_device_add(), and that is too early for hotplug
	path, as pci bar for hot add devices is not assigned yet
	after commit 4f535093.

So we need to move down that for hotplug path, call that in
pci_bus_add_devices instead, as at that time just before
drivers get attached. 
And that is simliar calling place for pci_device_add before
commit 4f535093 is applied.

We should apply this fix for v3.9, but is too late now.
so get it into v3.10 and could get into v3.9 stable instead.

Reported-by: David Bulkow <David.Bulkow@stratus.com>
Tested-by: David Bulkow <David.Bulkow@stratus.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/bus.c   |    1 +
 drivers/pci/probe.c |    1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/bus.c
+++ linux-2.6/drivers/pci/bus.c
@@ -201,6 +201,7 @@ void pci_bus_add_devices(const struct pc
 		/* Skip already-added devices */
 		if (dev->is_added)
 			continue;
+		pci_fixup_device(pci_fixup_final, dev);
 		retval = pci_bus_add_device(dev);
 		if (retval)
 			dev_err(&dev->dev, "Error adding device (%d)\n",
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -1341,7 +1341,6 @@ void pci_device_add(struct pci_dev *dev,
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
 
-	pci_fixup_device(pci_fixup_final, dev);
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 

Re: [PATCH] PCI: move down pci_fixup_final for hotplug path

[Bjorn Helgaas]

On Thu, Apr 25, 2013 at 06:47:07PM -0700, Yinghai Lu wrote:
> David found some resource conflict issue after
> | PCI: Put pci_dev in device tree as early as possible
> | commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
> 
> and
> | USB: Fix handoff when BIOS disables host PCI device
> | commit: cab928ee1f221c9cc48d6615070fefe2e444384a
> 
> for usb qirks for hotplug path.
> 
> After checking pci_fixup_device() with pci_fixup_final,
> now we have different path for boot path and hotadd path.
> 
> Boot path: because pci_apply_fix_final_quirks is not set yet,
> 	so pci_fixup_device(pci_fixup_final) will be skipped
> 	from pci_device_add().
>         And later pci_apply_final_quirks will be called for all
> 	pci devices via fs_initcall.
> 	That is after pci_assign_unassign resource.
> 	In that case quirk could use bars with problem.
> 
> Hotadd path: pci_fixup_device(pci_fixup_final) will be executed
> 	via pci_device_add(), and that is too early for hotplug
> 	path, as pci bar for hot add devices is not assigned yet
> 	after commit 4f535093.
> 
> So we need to move down that for hotplug path, call that in
> pci_bus_add_devices instead, as at that time just before
> drivers get attached. 
> And that is simliar calling place for pci_device_add before
> commit 4f535093 is applied.
> 
> We should apply this fix for v3.9, but is too late now.
> so get it into v3.10 and could get into v3.9 stable instead.
> 
> Reported-by: David Bulkow <David.Bulkow@stratus.com>
> Tested-by: David Bulkow <David.Bulkow@stratus.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

I applied the following slightly tweaked patch to for-linus and will
ask Linus to pull it for v3.10.  Let me know if it looks OK.

Bjorn

commit a939563f3fd9cd6226c7fb7135d0b93507c53888
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Tue May 7 14:35:44 2013 -0600

    PCI: Delay final fixups until resources are assigned
    
    Commit 4f535093cf "PCI: Put pci_dev in device tree as early as possible"
    moved final fixups from pci_bus_add_device() to pci_device_add().  But
    pci_device_add() happens before resource assignment, so BARs may not be
    valid yet.
    
    Typical flow for hot-add:
    
        pciehp_configure_device
          pci_scan_slot
            pci_scan_single_device
              pci_device_add
                pci_fixup_device(pci_fixup_final, dev)  # previous location
          # resource assignment happens here
          pci_bus_add_devices
            pci_bus_add_device
              pci_fixup_device(pci_fixup_final, dev)    # new location
    
    [bhelgaas: changelog, move fixups to pci_bus_add_device()]
    Reference: https://lkml.kernel.org/r/20130415182614.GB9224@xanatos
    Reported-by: David Bulkow <David.Bulkow@stratus.com>
    Tested-by: David Bulkow <David.Bulkow@stratus.com>
    Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: stable@vger.kernel.org	# v3.9+

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 748f8f3..32e66a6 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -174,6 +174,7 @@ int pci_bus_add_device(struct pci_dev *dev)
 	 * Can not put in pci_device_add yet because resources
 	 * are not assigned yet for some devices.
 	 */
+	pci_fixup_device(pci_fixup_final, dev);
 	pci_create_sysfs_dev_files(dev);
 
 	dev->match_driver = true;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 43ece5d..67cd045 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1341,7 +1341,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	list_add_tail(&dev->bus_list, &bus->devices);
 	up_write(&pci_bus_sem);
 
-	pci_fixup_device(pci_fixup_final, dev);
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 

Re: [PATCH] PCI: move down pci_fixup_final for hotplug path

[Bjorn Helgaas]

On Tue, May 7, 2013 at 2:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Apr 25, 2013 at 06:47:07PM -0700, Yinghai Lu wrote:
>> David found some resource conflict issue after
>> | PCI: Put pci_dev in device tree as early as possible
>> | commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4
>>
>> and
>> | USB: Fix handoff when BIOS disables host PCI device
>> | commit: cab928ee1f221c9cc48d6615070fefe2e444384a
>>
>> for usb qirks for hotplug path.
>>
>> After checking pci_fixup_device() with pci_fixup_final,
>> now we have different path for boot path and hotadd path.
>>
>> Boot path: because pci_apply_fix_final_quirks is not set yet,
>>       so pci_fixup_device(pci_fixup_final) will be skipped
>>       from pci_device_add().
>>         And later pci_apply_final_quirks will be called for all
>>       pci devices via fs_initcall.
>>       That is after pci_assign_unassign resource.
>>       In that case quirk could use bars with problem.
>>
>> Hotadd path: pci_fixup_device(pci_fixup_final) will be executed
>>       via pci_device_add(), and that is too early for hotplug
>>       path, as pci bar for hot add devices is not assigned yet
>>       after commit 4f535093.
>>
>> So we need to move down that for hotplug path, call that in
>> pci_bus_add_devices instead, as at that time just before
>> drivers get attached.
>> And that is simliar calling place for pci_device_add before
>> commit 4f535093 is applied.
>>
>> We should apply this fix for v3.9, but is too late now.
>> so get it into v3.10 and could get into v3.9 stable instead.
>>
>> Reported-by: David Bulkow <David.Bulkow@stratus.com>
>> Tested-by: David Bulkow <David.Bulkow@stratus.com>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>
> I applied the following slightly tweaked patch to for-linus and will
> ask Linus to pull it for v3.10.  Let me know if it looks OK.
>
> Bjorn
>
> commit a939563f3fd9cd6226c7fb7135d0b93507c53888
> Author: Bjorn Helgaas <bhelgaas@google.com>

(I did already fix the Author: to be Yinghai, sorry about that)

> Date:   Tue May 7 14:35:44 2013 -0600
>
>     PCI: Delay final fixups until resources are assigned
>
>     Commit 4f535093cf "PCI: Put pci_dev in device tree as early as possible"
>     moved final fixups from pci_bus_add_device() to pci_device_add().  But
>     pci_device_add() happens before resource assignment, so BARs may not be
>     valid yet.
>
>     Typical flow for hot-add:
>
>         pciehp_configure_device
>           pci_scan_slot
>             pci_scan_single_device
>               pci_device_add
>                 pci_fixup_device(pci_fixup_final, dev)  # previous location
>           # resource assignment happens here
>           pci_bus_add_devices
>             pci_bus_add_device
>               pci_fixup_device(pci_fixup_final, dev)    # new location
>
>     [bhelgaas: changelog, move fixups to pci_bus_add_device()]
>     Reference: https://lkml.kernel.org/r/20130415182614.GB9224@xanatos
>     Reported-by: David Bulkow <David.Bulkow@stratus.com>
>     Tested-by: David Bulkow <David.Bulkow@stratus.com>
>     Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     CC: stable@vger.kernel.org  # v3.9+
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 748f8f3..32e66a6 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -174,6 +174,7 @@ int pci_bus_add_device(struct pci_dev *dev)
>          * Can not put in pci_device_add yet because resources
>          * are not assigned yet for some devices.
>          */
> +       pci_fixup_device(pci_fixup_final, dev);
>         pci_create_sysfs_dev_files(dev);
>
>         dev->match_driver = true;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 43ece5d..67cd045 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1341,7 +1341,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>         list_add_tail(&dev->bus_list, &bus->devices);
>         up_write(&pci_bus_sem);
>
> -       pci_fixup_device(pci_fixup_final, dev);
>         ret = pcibios_add_device(dev);
>         WARN_ON(ret < 0);
>

Re: [PATCH] PCI: move down pci_fixup_final for hotplug path

[Yinghai Lu]

On Tue, May 7, 2013 at 2:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, May 7, 2013 at 2:32 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>     PCI: Delay final fixups until resources are assigned
>>
>>     Commit 4f535093cf "PCI: Put pci_dev in device tree as early as possible"
>>     moved final fixups from pci_bus_add_device() to pci_device_add().  But
>>     pci_device_add() happens before resource assignment, so BARs may not be
>>     valid yet.
>>
>>     Typical flow for hot-add:
>>
>>         pciehp_configure_device
>>           pci_scan_slot
>>             pci_scan_single_device
>>               pci_device_add
>>                 pci_fixup_device(pci_fixup_final, dev)  # previous location
>>           # resource assignment happens here
>>           pci_bus_add_devices
>>             pci_bus_add_device
>>               pci_fixup_device(pci_fixup_final, dev)    # new location
>>
>>     [bhelgaas: changelog, move fixups to pci_bus_add_device()]

Yes, that is right fix.

Thanks

Yinghai