Patchwork powerpc/pci: fix PCI-e devices rescan issue on powerpc platform

login
register
mail settings
Submitter Yuanquan Chen
Date April 2, 2013, 11:26 a.m.
Message ID <1364902014-943-1-git-send-email-Yuanquan.Chen@freescale.com>
Download mbox | patch
Permalink /patch/232963/
State Not Applicable
Headers show

Comments

Yuanquan Chen - April 2, 2013, 11:26 a.m.
Powerpc initializes the DMA and IRQ information in pci_scan_child_bus()->
pcibios_fixup_bus()->pcibios_setup_bus_devices(). But for the devices
which are hotpluged, bus->is added has been set for the first scan of the
PCI-e bus, so the initialization code won't be called. Then the hotpluged
devices' driver will fail to load.

For example :
The PCI-e device 0001:03:00.0 is the Intel PCI-e e1000e network card, remove
it from the system:

    # echo 1 > /sys/bus/pci/devices/0001\:03\:00.0/remove
    # e1000e 0001:03:00.0 eth0: removed PHC

Rescan it from it's bus:

    # echo 1 > /sys/bus/pci/devices/0001\:02\:00.0/rescan
    ...
    e1000e 0001:03:00.0: Disabling ASPM L0s L1
    e1000e 0001:03:00.0: No usable DMA configuration, aborting
    e1000e: probe of 0001:03:00.0 failed with error -5

So we move the DMA & IRQ initialization code from pcibios_setup_devices() and
construct a new function pcibios_enable_device. We call this function in
pcibios_enable_device, which will be called by PCI-e rescan code. At the
meanwhile, we avoid the the impact on cardbus. I also validate this patch with
silicon's PCIe-sata which encounters the IRQ issue.

Signed-off-by: Yuanquan Chen <Yuanquan.Chen@freescale.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
---
 arch/powerpc/kernel/pci-common.c |   43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)
Benjamin Herrenschmidt - April 2, 2013, 3:10 p.m.
On Tue, 2013-04-02 at 19:26 +0800, Yuanquan Chen wrote:
> So we move the DMA & IRQ initialization code from pcibios_setup_devices() and
> construct a new function pcibios_enable_device. We call this function in
> pcibios_enable_device, which will be called by PCI-e rescan code. At the
> meanwhile, we avoid the the impact on cardbus. I also validate this patch with
> silicon's PCIe-sata which encounters the IRQ issue.

My worry is that this delays the setup of the IRQ and DMA to very late in
the process, possibly after the quirks have been run, which can be
problematic. We have platform hooks that might try to "fixup" specific
IRQ issues on some platforms (especially macs) which I worry might fail
if delayed that way (I may be wrong, I don't have a specific case in mind,
but I would feel better if we kept setting up these things earlier).

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
Yuanquan Chen - April 3, 2013, 4:08 a.m.
On 04/02/2013 11:10 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-04-02 at 19:26 +0800, Yuanquan Chen wrote:
>> So we move the DMA & IRQ initialization code from pcibios_setup_devices() and
>> construct a new function pcibios_enable_device. We call this function in
>> pcibios_enable_device, which will be called by PCI-e rescan code. At the
>> meanwhile, we avoid the the impact on cardbus. I also validate this patch with
>> silicon's PCIe-sata which encounters the IRQ issue.
> My worry is that this delays the setup of the IRQ and DMA to very late in
> the process, possibly after the quirks have been run, which can be
> problematic. We have platform hooks that might try to "fixup" specific
> IRQ issues on some platforms (especially macs) which I worry might fail
> if delayed that way (I may be wrong, I don't have a specific case in mind,
> but I would feel better if we kept setting up these things earlier).
>
> Cheers,
> Ben.
>

Hi Ben,

I have checked all the quirk functions which are declared in kernel 
arch/powerpc
with command :
grep DECLARE_PCI_FIXUP_ `find arch/powerpc/ *.[hc]`

All the quirk function are defined as DECLARE_PCI_FIXUP_EARLY , 
DECLARE_PCI_FIXUP_HEADER
and DECLARE_PCI_FIXUP_FINAL, except quirk_uli5229() in 
arch/powerpc/platforms/fsl_uli1575.c, which is
defined both as DECLARE_PCI_FIXUP_HEADER and DECLARE_PCI_FIXUP_RESUME. 
So the quirk_uli5229()
will also be called with PCI pm module. The quirk functions defined as 
xxx_FINAL, HEADER and EARLY,
will be called in the path:

pci_scan_child_bus()->pci_scan_slot()->pci_scan_single_device()->pci_scan_device()->pci_setup_device()
->pci_device_add()

the pci_scan_slot() is called earlier than pcibios_fixup_bus() even for 
the first scan of PCI-e bus, so the quirk
functions on powerpc platform is called before the DMA & IRQ fixup. So 
in reality, the delay of DMA & IRQ fixup
won't affect anything.

Regards,
Yuanquan

>
>
>


--
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
Yuanquan Chen - April 10, 2013, 9:08 a.m.
On 04/03/2013 12:08 PM, Chen Yuanquan-B41889 wrote:
> On 04/02/2013 11:10 PM, Benjamin Herrenschmidt wrote:
>> On Tue, 2013-04-02 at 19:26 +0800, Yuanquan Chen wrote:
>>> So we move the DMA & IRQ initialization code from 
>>> pcibios_setup_devices() and
>>> construct a new function pcibios_enable_device. We call this 
>>> function in
>>> pcibios_enable_device, which will be called by PCI-e rescan code. At 
>>> the
>>> meanwhile, we avoid the the impact on cardbus. I also validate this 
>>> patch with
>>> silicon's PCIe-sata which encounters the IRQ issue.
>> My worry is that this delays the setup of the IRQ and DMA to very 
>> late in
>> the process, possibly after the quirks have been run, which can be
>> problematic. We have platform hooks that might try to "fixup" specific
>> IRQ issues on some platforms (especially macs) which I worry might fail
>> if delayed that way (I may be wrong, I don't have a specific case in 
>> mind,
>> but I would feel better if we kept setting up these things earlier).
>>
>> Cheers,
>> Ben.
>>
>
> Hi Ben,
>
> I have checked all the quirk functions which are declared in kernel 
> arch/powerpc
> with command :
> grep DECLARE_PCI_FIXUP_ `find arch/powerpc/ *.[hc]`
>
> All the quirk function are defined as DECLARE_PCI_FIXUP_EARLY , 
> DECLARE_PCI_FIXUP_HEADER
> and DECLARE_PCI_FIXUP_FINAL, except quirk_uli5229() in 
> arch/powerpc/platforms/fsl_uli1575.c, which is
> defined both as DECLARE_PCI_FIXUP_HEADER and DECLARE_PCI_FIXUP_RESUME. 
> So the quirk_uli5229()
> will also be called with PCI pm module. The quirk functions defined as 
> xxx_FINAL, HEADER and EARLY,
> will be called in the path:
>
> pci_scan_child_bus()->pci_scan_slot()->pci_scan_single_device()->pci_scan_device()->pci_setup_device() 
>
> ->pci_device_add()
>
> the pci_scan_slot() is called earlier than pcibios_fixup_bus() even 
> for the first scan of PCI-e bus, so the quirk
> functions on powerpc platform is called before the DMA & IRQ fixup. So 
> in reality, the delay of DMA & IRQ fixup
> won't affect anything.
>
> Regards,
> Yuanquan
>

Hi Ben,

How do you think about this? Do you have any comment?

Thanks,
Yuanquan

>>
>>
>>
>


--
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
Yuanquan Chen - April 23, 2013, 9:26 a.m.
On 04/10/2013 05:08 PM, Chen Yuanquan-B41889 wrote:
> On 04/03/2013 12:08 PM, Chen Yuanquan-B41889 wrote:
>> On 04/02/2013 11:10 PM, Benjamin Herrenschmidt wrote:
>>> On Tue, 2013-04-02 at 19:26 +0800, Yuanquan Chen wrote:
>>>> So we move the DMA & IRQ initialization code from 
>>>> pcibios_setup_devices() and
>>>> construct a new function pcibios_enable_device. We call this 
>>>> function in
>>>> pcibios_enable_device, which will be called by PCI-e rescan code. 
>>>> At the
>>>> meanwhile, we avoid the the impact on cardbus. I also validate this 
>>>> patch with
>>>> silicon's PCIe-sata which encounters the IRQ issue.
>>> My worry is that this delays the setup of the IRQ and DMA to very 
>>> late in
>>> the process, possibly after the quirks have been run, which can be
>>> problematic. We have platform hooks that might try to "fixup" specific
>>> IRQ issues on some platforms (especially macs) which I worry might fail
>>> if delayed that way (I may be wrong, I don't have a specific case in 
>>> mind,
>>> but I would feel better if we kept setting up these things earlier).
>>>
>>> Cheers,
>>> Ben.
>>>
>>
>> Hi Ben,
>>
>> I have checked all the quirk functions which are declared in kernel 
>> arch/powerpc
>> with command :
>> grep DECLARE_PCI_FIXUP_ `find arch/powerpc/ *.[hc]`
>>
>> All the quirk function are defined as DECLARE_PCI_FIXUP_EARLY , 
>> DECLARE_PCI_FIXUP_HEADER
>> and DECLARE_PCI_FIXUP_FINAL, except quirk_uli5229() in 
>> arch/powerpc/platforms/fsl_uli1575.c, which is
>> defined both as DECLARE_PCI_FIXUP_HEADER and 
>> DECLARE_PCI_FIXUP_RESUME. So the quirk_uli5229()
>> will also be called with PCI pm module. The quirk functions defined 
>> as xxx_FINAL, HEADER and EARLY,
>> will be called in the path:
>>
>> pci_scan_child_bus()->pci_scan_slot()->pci_scan_single_device()->pci_scan_device()->pci_setup_device() 
>>
>> ->pci_device_add()
>>
>> the pci_scan_slot() is called earlier than pcibios_fixup_bus() even 
>> for the first scan of PCI-e bus, so the quirk
>> functions on powerpc platform is called before the DMA & IRQ fixup. 
>> So in reality, the delay of DMA & IRQ fixup
>> won't affect anything.
>>
>> Regards,
>> Yuanquan
>>
>
> Hi Ben,
>
> How do you think about this? Do you have any comment?
>
> Thanks,
> Yuanquan
>

Hi Bjorn,

There's no response from Ben. How do you think about this patch? What's 
your advice?

Thanks,
Yuanquan

>>>
>>>
>>>
>>
>
>
>


--
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 - April 23, 2013, 10:05 a.m.
On Tue, 2013-04-23 at 17:26 +0800, Chen Yuanquan-B41889 wrote:
> There's no response from Ben. How do you think about this patch?
> What's 
> your advice?

Didn't Michael put your patch in -next while I was on vacation ? I'll
check tomorrow.

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
Yuanquan Chen - April 23, 2013, 10:51 a.m.
On 04/23/2013 06:05 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-04-23 at 17:26 +0800, Chen Yuanquan-B41889 wrote:
>> There's no response from Ben. How do you think about this patch?
>> What's
>> your advice?
> Didn't Michael put your patch in -next while I was on vacation ? I'll
> check tomorrow.
>
> Cheers,
> Ben.
>

Hi Ben,

I have checked the latest kernel(v3.9-rc7). This patch has been applied.

Thanks,
Yuanquan

>
>
>


--
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
Michael Ellerman - April 23, 2013, 11:30 a.m.
On Tue, Apr 23, 2013 at 06:51:09PM +0800, Chen Yuanquan-B41889 wrote:
> On 04/23/2013 06:05 PM, Benjamin Herrenschmidt wrote:
> >On Tue, 2013-04-23 at 17:26 +0800, Chen Yuanquan-B41889 wrote:
> >>There's no response from Ben. How do you think about this patch?
> >>What's
> >>your advice?
> >Didn't Michael put your patch in -next while I was on vacation ? I'll
> >check tomorrow.
> >
> >Cheers,
> >Ben.
> >
> 
> Hi Ben,
> 
> I have checked the latest kernel(v3.9-rc7). This patch has been applied.

No it's not in v3.9-rc7.

It is in my test branch, which is in linux-next.

http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit?id=37f02195bee9c25ce44e25204f40b7961a6d7c9d

cheers
--
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

Patch

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index fa12ae4..0324758 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1023,6 +1023,27 @@  void pcibios_setup_bus_self(struct pci_bus *bus)
 		ppc_md.pci_dma_bus_setup(bus);
 }
 
+void pcibios_setup_device(struct pci_dev *dev)
+{
+	/* Fixup NUMA node as it may not be setup yet by the generic
+	 * code and is needed by the DMA init
+	 */
+	set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+	/* Hook up default DMA ops */
+	set_dma_ops(&dev->dev, pci_dma_ops);
+	set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+	/* Additional platform DMA/iommu setup */
+	if (ppc_md.pci_dma_dev_setup)
+		ppc_md.pci_dma_dev_setup(dev);
+
+	/* Read default IRQs and fixup if necessary */
+	pci_read_irq_line(dev);
+	if (ppc_md.pci_irq_fixup)
+		ppc_md.pci_irq_fixup(dev);
+}
+
 void pcibios_setup_bus_devices(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
@@ -1037,23 +1058,7 @@  void pcibios_setup_bus_devices(struct pci_bus *bus)
 		if (dev->is_added)
 			continue;
 
-		/* Fixup NUMA node as it may not be setup yet by the generic
-		 * code and is needed by the DMA init
-		 */
-		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
-		/* Hook up default DMA ops */
-		set_dma_ops(&dev->dev, pci_dma_ops);
-		set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
-
-		/* Additional platform DMA/iommu setup */
-		if (ppc_md.pci_dma_dev_setup)
-			ppc_md.pci_dma_dev_setup(dev);
-
-		/* Read default IRQs and fixup if necessary */
-		pci_read_irq_line(dev);
-		if (ppc_md.pci_irq_fixup)
-			ppc_md.pci_irq_fixup(dev);
+		pcibios_setup_device(dev);
 	}
 }
 
@@ -1494,6 +1499,10 @@  int pcibios_enable_device(struct pci_dev *dev, int mask)
 		if (ppc_md.pcibios_enable_device_hook(dev))
 			return -EINVAL;
 
+	/* avoid pcie irq fix up impact on cardbus */
+	if (dev->hdr_type != PCI_HEADER_TYPE_CARDBUS)
+		pcibios_setup_device(dev);
+
 	return pci_enable_resources(dev, mask);
 }