pciehp: Add archdata setting

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
<snip>
[   65.493662] pci 0000:03:00.0: BAR 2: set to [io 0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03] [   65.508555] pcieport 0000:02:01.0:   bridge window [io 0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem 0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform [   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002) [   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting [   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5 -sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip>  // There is no network interface for 82572EI

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Signed-off-by Hiroo MATSUMOTO <[email protected]>

drivers/pci/hotplug/pciehp_pci.c +1 -0

index a4031df..a021fbc 100644
@@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
                        pciehp_add_bridge(dev);
                }
                pci_dev_put(dev);
+               dev->dev.archdata = bridge->dev.archdata;
        }
 
        pci_assign_unassigned_bridge_resources(bridge);

Quoted 33 lines

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
<snip>

[   65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[   65.508555] pcieport 0000:02:01.0:   bridge window [io
0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem
0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform
[   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
[   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5

-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip> // There is no network interface for 82572EI

Quoted 7 lines

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Interesting. It would be nice if we could set archdata the same way for both devices present at boot and those hot-added later. Can you figure out where archdata is set for devices present at boot?

Bjorn

Quoted 14 lines

Signed-off-by Hiroo MATSUMOTO <[email protected]>

drivers/pci/hotplug/pciehp_pci.c +1 -0

index a4031df..a021fbc 100644
@@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
                       pciehp_add_bridge(dev);
               }
               pci_dev_put(dev);
+               dev->dev.archdata = bridge->dev.archdata;
       }
 
       pci_assign_unassigned_bridge_resources(bridge);

Hi

Thanks for your comment.

I have mistake.
archdata.dma_ops is always NULL on x86 platform.
On x86 platform, probe with pciehp is same as probe with initcall. I'm so sorry.

But archdata on powerpc platform is set with initcall. Call stack is as below.
1. pcibios_init
2. pcibios_scan_phb
3. pci_scan_child_bus
4. pci_scan_bridge
5. pci_scan_child_bus
6. pci_scan_bridge
7. pci_scan_child_bus
8. pcibios_setup_bus_devices

In pcibios_setup_bus_devices, archdata is set with set_dma_ops and set_dma_offset. But set_dma_ops and set_dma_offset are defined only at powerpc and microblaze (Why...).

Can architecture code, like arch/powerpc/kernel/pci.c, be called from pciehp ?

Regards.

Hiroo MATSUMOTO (FUJITSU COMPUTER TECHNOLOGIES LIMITED) [email protected]

Quoted 62 lines

Quoted 33 lines

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1 > /sys/bus/pci/slots/1/power
<snip>

[ 65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[ 65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[ 65.508555] pcieport 0000:02:01.0: bridge window [io
0xff7ee000-0xff7eefff]
[ 65.515785] pcieport 0000:02:01.0: bridge window [mem
0xa0100000-0xa01fffff]
[ 65.523015] pcieport 0000:02:01.0: bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[ 65.531238] pci 0000:03:00.0: no hotplug settings from platform
[ 65.538361] e1000e 0000:03:00.0: Disabling ASPM L1
[ 65.543616] e1000e 0000:03:00.0: enabling device (0000 -> 0002)
[ 65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[ 65.557194] e1000e: probe of 0000:03:00.0 failed with error -5

-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip> // There is no network interface for 82572EI

Quoted 7 lines

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Interesting. It would be nice if we could set archdata the same way for both devices present at boot and those hot-added later. Can you figure out where archdata is set for devices present at boot?

Bjorn

Quoted 14 lines

Signed-off-by Hiroo MATSUMOTO <[email protected]>

drivers/pci/hotplug/pciehp_pci.c +1 -0

index a4031df..a021fbc 100644
@@ -89,6 +89,7 @@ int pciehp_configure_device(struct slot *p_slot)
 pciehp_add_bridge(dev);
 }
 pci_dev_put(dev);
+ dev->dev.archdata = bridge->dev.archdata;
 }
 
 pci_assign_unassigned_bridge_resources(bridge);

Quoted 37 lines · 松本博郎

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1> /sys/bus/pci/slots/1/power
<snip>

[   65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[   65.508555] pcieport 0000:02:01.0:   bridge window [io
0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem
0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform
[   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
[   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5

-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip> // There is no network interface for 82572EI

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

Quoted 3 lines

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Interesting. It would be nice if we could set archdata the same way for both devices present at boot and those hot-added later.

Can you
figure out where archdata is set for devices present at boot?

Quoted 37 lines

Quoted 37 lines

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1> /sys/bus/pci/slots/1/power
<snip>

[   65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[   65.508555] pcieport 0000:02:01.0:   bridge window [io
0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem
0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform
[   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
[   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5

-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip> // There is no network interface for 82572EI

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

Quoted 6 lines

Quoted 3 lines

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Interesting. It would be nice if we could set archdata the same way for both devices present at boot and those hot-added later.

Yes. But it might be not so easy...

Quoted 2 lines

Can you
figure out where archdata is set for devices present at boot?

I read some x86 and powerpc code to setup archdata. x86 and powerpc have different way to setup archdata. And it seems archdata is not set in one location, each member in the archdata is setup in the different locations.

On x86, archdata.dma_ops is set in the pci_iommu_init() code path, which is called through rootfs_initcall(). On powerpc, archdata.dma_ops is set in pcibios_fixup_bus() code path.

Here are my notes.

x86 (calgary iommu)
===================
rootfs_initcall()
=> pci_iommu_init()

     => x86_init.iommu.iommu_init() (== calgary_iommu_init())
         => calgary_init()

calgary_init()

{
        for_each_pci_dev() {
                        dev->dev.archdata.dma_ops = &calgary_dma_ops;
        }
}

x86 (amd iommu)
===============
rootfs_initcall()
=> pci_iommu_init()

     => x86_init.iommu.iommu_init() (== amd_iommu_init())
         => amd_iommu_init_dma_ops()
             => device_dma_ops_init()

device_dma_ops_init()

{
    for_each_pci_dev() {
        pdev->dev.archdata.dma_ops = &amd_iommu_dma_ops;
}

PowerPC
=======
pci_scan_child_bus()
=> pcibios_fixup_bus()
=> pcibios_setup_bus_devices()
=> set_dma_ops() # for each device on the bus
(default)
=> ppc_md.pci_dma_dev_setup() # for each device on the bus
(machine depend)

static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)

{
        dev->archdata.dma_ops = ops;
}

One possible idea is that to setup archdata in the pcibios_xxx() hook. (for example, use existing pcibios_enable_device(), or provide new hook like pcibios_setup_device() which is called by pci_setup_device).

Another idea is that to add support for hot-plug to each iommu (dma) code. Maybe it can be implemented by using bus notifier.

Regards,
Kenji Kaneshige

Hi, Bjorn and Kaneshige

Thanks for your comments and proposal.
I will rethink a way of setting archdata with pcibios_xxx() hook or bus notifier.

Regards.

Hiroo MATSUMOTO

Quoted 114 lines

Quoted 37 lines

Quoted 37 lines

I'm trying to use pciehp on powerpc platform.
I set e1000e card to PCI Express bridge that has PCI Express HotPlug Capability.

There is problem when poweron PCI Express HotPlug slot with pciehp. e1000e driver needs dma_ops in struct dev_archdata, but archdata isn't set (dma_ops is NULL) when probe from pciehp.
Then e1000e driver returns error as below.

-sh-3.2# echo 1> /sys/bus/pci/slots/1/power
<snip>

[   65.493662] pci 0000:03:00.0: BAR 2: set to [io
0xff7ee000-0xff7ee01f] (PCI address [0x1000-0x101f])
[   65.502890] pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
[   65.508555] pcieport 0000:02:01.0:   bridge window [io
0xff7ee000-0xff7eefff]
[   65.515785] pcieport 0000:02:01.0:   bridge window [mem
0xa0100000-0xa01fffff]
[   65.523015] pcieport 0000:02:01.0:   bridge window [mem
0xa0200000-0xa02fffff 64bit pref]
[   65.531238] pci 0000:03:00.0: no hotplug settings from platform
[   65.538361] e1000e 0000:03:00.0: Disabling ASPM  L1
[   65.543616] e1000e 0000:03:00.0: enabling device (0000 ->  0002)
[   65.549876] e1000e 0000:03:00.0: No usable DMA configuration, aborting
[   65.557194] e1000e: probe of 0000:03:00.0 failed with error -5

-sh-3.2# lspci
<snip>
0000:03:00.0 Ethernet controller: Intel Corporation 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
<snip>
-sh-3.2# ifconfig -a
<snip> // There is no network interface for 82572EI

If archdata.dma_ops is NULL on x86 platform, e1000e will get dma_ops without archdata.
So e1000e driver doesn't return error.

Quoted 6 lines

Quoted 3 lines

I think that archdata should be set before driver probe (e.g pciehp). I tried to fix it (pciehp-add-archdata.patch), but I'm not good at PCI driver..., so please give better way.

Interesting. It would be nice if we could set archdata the same way for both devices present at boot and those hot-added later.

Yes. But it might be not so easy...

Quoted 2 lines

Can you
figure out where archdata is set for devices present at boot?

I read some x86 and powerpc code to setup archdata. x86 and powerpc have different way to setup archdata. And it seems archdata is not set in one location, each member in the archdata is setup in the different locations.

On x86, archdata.dma_ops is set in the pci_iommu_init() code path, which is called through rootfs_initcall(). On powerpc, archdata.dma_ops is set in pcibios_fixup_bus() code path.

Here are my notes.

x86 (calgary iommu)
===================
rootfs_initcall()
=> pci_iommu_init()

     => x86_init.iommu.iommu_init() (== calgary_iommu_init())
         => calgary_init()

calgary_init()

{
        for_each_pci_dev() {
                        dev->dev.archdata.dma_ops = &calgary_dma_ops;
        }
}

x86 (amd iommu)
===============
rootfs_initcall()
=> pci_iommu_init()

     => x86_init.iommu.iommu_init() (== amd_iommu_init())
         => amd_iommu_init_dma_ops()
             => device_dma_ops_init()

device_dma_ops_init()

{
    for_each_pci_dev() {
        pdev->dev.archdata.dma_ops = &amd_iommu_dma_ops;
}

PowerPC
=======
pci_scan_child_bus()
=> pcibios_fixup_bus()
=> pcibios_setup_bus_devices()
=> set_dma_ops() # for each device on the bus
(default)
=> ppc_md.pci_dma_dev_setup() # for each device on the bus
(machine depend)

static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)

{
        dev->archdata.dma_ops = ops;
}

One possible idea is that to setup archdata in the pcibios_xxx() hook. (for example, use existing pcibios_enable_device(), or provide new hook like pcibios_setup_device() which is called by pci_setup_device).

Another idea is that to add support for hot-plug to each iommu (dma) code. Maybe it can be implemented by using bus notifier.

Regards,
Kenji Kaneshige

Hi, Kaneshige

You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in arch/powerpc/kernel/pci-common.c.
It works good.

Quoted 11 lines · Hiroo Matsumoto

Hi, Kaneshige

You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in arch/powerpc/kernel/pci-common.c.
It works good.

When I researched this, I thought the best route was to use the bus_register_notifier() path, as amd_iommu_init_notifier() does.

We're filling in a struct device field, not a PCI field, and bus_register_notifier() seems like a more generic path than relying on pcibios_enable_device().

Bjorn

Hi, Bjorn

Thanks for your research and comment.

As you said, a way of registering code with bus_register_notifier(), which will be called in device_add(), is better one than pcibios_enable_device(). I will try to write code with bus_register_notifier().

Regards.

Hiroo MATSUMOTO

Quoted 24 lines

Quoted 11 lines · Hiroo Matsumoto

Hi, Kaneshige

You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in arch/powerpc/kernel/pci-common.c.
It works good.

When I researched this, I thought the best route was to use the bus_register_notifier() path, as amd_iommu_init_notifier() does.

We're filling in a struct device field, not a PCI field, and bus_register_notifier() seems like a more generic path than relying on pcibios_enable_device().

Bjorn

Hi, Bjorn

I wrote a code with bus_register_notifier().
A function that calls bus_register_notifer() is called from rootfs_initcall as well as amd_iommu is.

Please review this.

Regards.

Hiroo MATSUMOTO

Quoted 5 lines

Hi, Bjorn

Thanks for your research and comment.

Quoted 36 lines

As you said, a way of registering code with bus_register_notifier(), which will be called in device_add(), is better one than pcibios_enable_device(). I will try to write code with bus_register_notifier().

Regards.

Hiroo MATSUMOTO

Quoted 26 lines

Quoted 4 lines · Hiroo Matsumoto

Hi, Kaneshige

Quoted 8 lines

You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in arch/powerpc/kernel/pci-common.c.
It works good.

When I researched this, I thought the best route was to use the bus_register_notifier() path, as amd_iommu_init_notifier() does.

We're filling in a struct device field, not a PCI field, and bus_register_notifier() seems like a more generic path than relying on pcibios_enable_device().

Bjorn

Signed-off-by Hiroo MATSUMOTO <[email protected]>

arch/powerpc/kernel/pci-common.c +11 -0

index 32656f1..1fe1e25 100644
@@ -1083,6 +1083,17 @@ void __devinit pcibios_setup_bus_self(struct pci_bus
 *bus)
                ppc_md.pci_dma_bus_setup(bus);
  }
 
+static inline void pcibios_set_dma_ops(struct pci_dev *dev)
+{
+       /* 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);
+}
+
  void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
  {

       struct pci_dev *dev;

arch/powerpc/kernel/pci-common.c +1 -7

@@ -1102,13 +1113,7 @@ void __devinit pcibios_setup_bus_devices(struct
 pci_bus *bus)
                 */
                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);
+               pcibios_set_dma_ops(dev);
 
                /* Read default IRQs and fixup if necessary */

               pci_read_irq_line(dev);

arch/powerpc/kernel/pci-common.c +0 -0

@@ -1749,3 +1754,33 @@ static void fixup_hide_host_resource_fsl(struct
 pci_dev *dev)
  }
  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID,

fixup_hide_host_resource_fsl); DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);

+
+       switch (action) {
+       case BUS_NOTIFY_ADD_DEVICE:
+               if (!get_dma_ops(&dev->dev))
+                       pcibios_set_dma_ops(dev);
+               break;
+       default:
+               break;

+       }

arch/powerpc/kernel/pci-common.c +3 -0

+
+       return 0;
+}

Quoted 101 lines · Hiroo Matsumoto

Hi, Bjorn

I wrote a code with bus_register_notifier().
A function that calls bus_register_notifer() is called from rootfs_initcall as well as amd_iommu is.

Please review this.

Regards.

Hiroo MATSUMOTO

Quoted 5 lines

Hi, Bjorn

Thanks for your research and comment.

Quoted 33 lines

As you said, a way of registering code with bus_register_notifier(), which will be called in device_add(), is better one than pcibios_enable_device(). I will try to write code with bus_register_notifier().

Regards.

Hiroo MATSUMOTO

Quoted 24 lines

Quoted 11 lines · Hiroo Matsumoto

Hi, Kaneshige

You are right!
Setting dma_ops in pcibios_enable_device is a nice way.
In PCI driver, pci_enable_device_xxx that calls pcibios_enable_device is called before checking archdata.dma_ops.

I added code of checking and setting dma_ops to pcibios_enable_device in arch/powerpc/kernel/pci-common.c.
It works good.

When I researched this, I thought the best route was to use the bus_register_notifier() path, as amd_iommu_init_notifier() does.

We're filling in a struct device field, not a PCI field, and bus_register_notifier() seems like a more generic path than relying on pcibios_enable_device().

Bjorn

Signed-off-by Hiroo MATSUMOTO <[email protected]>

arch/powerpc/kernel/pci-common.c +11 -0

index 32656f1..1fe1e25 100644
@@ -1083,6 +1083,17 @@ void __devinit pcibios_setup_bus_self(struct pci_bus
 *bus)
               ppc_md.pci_dma_bus_setup(bus);
 }
 
+static inline void pcibios_set_dma_ops(struct pci_dev *dev)
+{
+       /* 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);
+}
+
 void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
 {

       struct pci_dev *dev;

arch/powerpc/kernel/pci-common.c +1 -7

@@ -1102,13 +1113,7 @@ void __devinit pcibios_setup_bus_devices(struct
 pci_bus *bus)
                */
               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);
+               pcibios_set_dma_ops(dev);
 
               /* Read default IRQs and fixup if necessary */

               pci_read_irq_line(dev);

arch/powerpc/kernel/pci-common.c +0 -0

@@ -1749,3 +1754,33 @@ static void fixup_hide_host_resource_fsl(struct
 pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID,

fixup_hide_host_resource_fsl);
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID,
fixup_hide_host_resource_fsl);

arch/powerpc/kernel/pci-common.c static void fixup_hide_host_resource_fsl(struct pci_dev *dev) · line 1764

Quoted 8 lines

arch/powerpc/kernel/pci-common.c +8 -0

+
+static int pcibios_device_change_notifier(struct notifier_block *nb,
+                                         unsigned long action, void *data)
+{
+       struct pci_dev *dev = to_pci_dev(data);
+
+       if (!dev)
+               return 0;

I don't think you need this check.

arch/powerpc/kernel/pci-common.c static void fixup_hide_host_resource_fsl(struct pci_dev *dev) · line 1772

Quoted 8 lines

arch/powerpc/kernel/pci-common.c +8 -0

+
+       switch (action) {
+       case BUS_NOTIFY_ADD_DEVICE:
+               if (!get_dma_ops(&dev->dev))
+                       pcibios_set_dma_ops(dev);
+               break;
+       default:
+               break;

The "default:" case is superfluous.

Quoted 4 lines

arch/powerpc/kernel/pci-common.c +4 -0

+       }
+
+       return 0;
+}

arch/powerpc/kernel/pci-common.c static void fixup_hide_host_resource_fsl(struct pci_dev *dev) · line 1786

Quoted 10 lines

arch/powerpc/kernel/pci-common.c +10 -0

+
+static struct notifier_block device_nb = {
+       .notifier_call = pcibios_device_change_notifier,
+};
+
+static int pcibios_bus_notifier_init(void)
+{
+       return bus_register_notifier(&pci_bus_type, &device_nb);
+}
+rootfs_initcall(pcibios_bus_notifier_init);

Instead of making this a rootfs_initcall(), can you call bus_register_notifier() explicitly in pcibios_init()? That way pcibios_device_change_notifier() should get called for *all* new PCI devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the pcibios_setup_bus_devices() code into the pcibios_device_change_notifier() path, including the NUMA node and IRQ fixups.

Your patch will also need a changelog.

Bjorn

Instead of making this a rootfs_initcall(), can you call bus_register_notifier() explicitly in pcibios_init()? That way pcibios_device_change_notifier() should get called for *all* new PCI devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the pcibios_setup_bus_devices() code into the pcibios_device_change_notifier() path, including the NUMA node and IRQ fixups.

Your patch will also need a changelog.

Bjorn

Hi, Bjorn

Refer to your review and comment, I updated bus notifier code. This works good. Thanks for your comments.
And I think this way can be applied to microblaze. Please review this patch.

* Changes
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
2. Calling bus_register_notifier in pcibios_init.
3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.

* Not patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16 <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Not patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5

* Patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003

Regards.

Hiroo MATSUMOTO

Quoted 15 lines

Instead of making this a rootfs_initcall(), can you call bus_register_notifier() explicitly in pcibios_init()? That way pcibios_device_change_notifier() should get called for *all* new PCI devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the pcibios_setup_bus_devices() code into the pcibios_device_change_notifier() path, including the NUMA node and IRQ fixups.

Your patch will also need a changelog.

Bjorn

Hi,

I have some comments.

- I think the patch should be split into two, for microblaze and for powerpc.

- The following code looks redundant. I think dev->is_added is always false at the BUS_NOTIFY_ADD_DEVICE time.

+               /* Cardbus can call us to add new devices to a bus, so ignore
+                * those who are already fully discovered
+                */
+               if (dev->is_added)
+                       break;

- You need to add patch description.

Quoted 77 lines · Hiroo Matsumoto

Hi, Bjorn

Refer to your review and comment, I updated bus notifier code. This works good. Thanks for your comments.
And I think this way can be applied to microblaze. Please review this patch.

* Changes
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
2. Calling bus_register_notifier in pcibios_init.
3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.

* Not patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16 <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Not patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5

* Patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003

Quoted 20 lines

Regards.

Hiroo MATSUMOTO

Quoted 15 lines

Instead of making this a rootfs_initcall(), can you call bus_register_notifier() explicitly in pcibios_init()? That way pcibios_device_change_notifier() should get called for *all* new PCI devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the pcibios_setup_bus_devices() code into the pcibios_device_change_notifier() path, including the NUMA node and IRQ fixups.

Your patch will also need a changelog.

Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Kenji Kaneshige

Hi,

I have some comments.

- I think the patch should be split into two, for microblaze and for powerpc.

- The following code looks redundant. I think dev->is_added is always false at the BUS_NOTIFY_ADD_DEVICE time.

arch/powerpc/kernel/pci-common.c void __devinit pcibios_scan_phb(struct pci_controller *hose)

Quoted 7 lines

arch/powerpc/kernel/pci-common.c +5 -1

+               /* Cardbus can call us to add new devices to a bus, so ignore
+                * those who are already fully discovered
+                */
+               if (dev->is_added)
+                       break;
 
- You need to add patch description.

Thanks for doing this; I think the code looks great. Please address Kenji-san's dev->is_added comment.

He's also right that we need this in two patches. Add cc: Benjamin Herrenschmidt <[email protected]> when you repost it so he can ack/nack the powerpc part.

Quoted 102 lines

Quoted 77 lines · Hiroo Matsumoto

Hi, Bjorn

Refer to your review and comment, I updated bus notifier code. This works good. Thanks for your comments.
And I think this way can be applied to microblaze. Please review this patch.

* Changes
1. Moving pcibios_setup_bus_devices code to pcibios_device_change_notifier so that boot and hotplug use same code. This will change boot message on powerpc platform but there is no difference. Please see "Not patched boot message" and "Patched message".
2. Calling bus_register_notifier in pcibios_init.
3. Using bus notifier not only on powerpc platform but also on microblaze platform because microblaze pcibios_setup_bus_devices is a similer way with powerpc.
4. Removing caller and callee of pcibios_setup_bus_devices because bus notifier works instead of pcibios_setup_bus_devices.

* Not patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16 <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled)
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Patched boot message
PCI: Probing PCI hardware
<snip>
pci 0000:00:00.0: PCI bridge to [bus 01-ff]
pci 0000:00:00.0: bridge window [io 0x0000-0x0000] (disabled) pci 0000:00:00.0: bridge window [mem 0xa0000000-0xa01fffff]
pci 0000:00:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
pci 0000:01:00.0: PCI bridge to [bus 02-ff]
pci 0000:01:00.0: bridge window [io 0x1000-0x1fff]
pci 0000:01:00.0: bridge window [mem 0xa0100000-0xa01fffff]
pci 0000:01:00.0: bridge window [mem 0x10000000-0x000fffff pref] (disabled) <snip>
irq: irq 4 on host /soc@ffe00000/pic@40000 mapped to virtual irq 16
irq: irq 5 on host /soc@ffe00000/pic@40000 mapped to virtual irq 17
irq: irq 7 on host /soc@ffe00000/pic@40000 mapped to virtual irq 18

* Not patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5

* Patched pciehp message on powerpc platform

# echo 1 > /sys/bus/pci/slots/1/power
<snip>
pcieport 0000:02:01.0: PCI bridge to [bus 03-03]
pcieport 0000:02:01.0: bridge window [io 0xff7ee000-0xff7eefff]
pcieport 0000:02:01.0: bridge window [mem 0xa0100000-0xa01fffff]
pcieport 0000:02:01.0: bridge window [mem 0xa0200000-0xa02fffff 64bit pref]
pci 0000:03:00.0: no hotplug settings from platform
e1000e 0000:03:00.0: Disabling ASPM L1
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003

Quoted 20 lines

Regards.

Hiroo MATSUMOTO

Quoted 15 lines

Instead of making this a rootfs_initcall(), can you call bus_register_notifier() explicitly in pcibios_init()? That way pcibios_device_change_notifier() should get called for *all* new PCI devices, whether we find them at boot-time or they're hot-added later.

If you do that, I think you can move all the pcibios_setup_bus_devices() code into the pcibios_device_change_notifier() path, including the NUMA node and IRQ fixups.

Your patch will also need a changelog.

Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html