Re: [PATCH v4 5/6] pci: Use parent domain number when allocating child busses.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

Please see inline.

On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau <liviu@xxxxxxxxxxx> wrote:
> On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
>> Hello Liviu,
>>
>> Thanks for fixing up domain_nr. Now I have moved on further to a new
>> domain_nr related warning dump :-)
>>
>> -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up
>> pci_bus 0001:00: scanning bus
>> pci_setup_device:1101 domain_nr = 1
>> pci 0001:00:00.0: [e008:e004] type 01 class 0x060400
>> pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit]
>> pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80
>> pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0
>> pci 0001:00:00.0: supports D1 D2
>> pci_bus 0001:00: fixups for bus
>> pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0
>> pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring
>> pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1
>> ** pci_scan_bridge:855  pci_domain_nr(bus) = 1
>> ** pci_alloc_child_bus:681  pci_domain_nr(bus) = 1
>> pci_bus 0001:01: scanning bus
>> pci_setup_device:1101 domain_nr = 0
>
> Why does the domain_nr change here?

The bridge device pointer for parent and child should be same right? I
think this is not the case here. Please look at the log at the bottom
that I captured after trying your suggestions.

>
>> pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000
>> pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit]
>> pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref]
>> pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref]
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1 at
>> /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52
>> sysfs_warn_dup+0x80/0xc0()
>> sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0'
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40
>> Call trace:
>> [<ffffffc000088180>] dump_backtrace+0x0/0x140
>> [<ffffffc0000882d0>] show_stack+0x10/0x20
>> [<ffffffc0004f65ac>] dump_stack+0x74/0xc4
>> [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0
>> [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60
>> [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0
>> [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100
>> [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40
>> [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0
>> [<ffffffc000322f1c>] device_add+0x31c/0x520
>> [<ffffffc0002c444c>] pci_device_add+0xec/0x140
>> [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0
>> [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120
>> [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140
>> [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0
>> [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140
>> [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40
>> [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c
>> [<ffffffc000327d20>] platform_drv_probe+0x20/0x60
>> [<ffffffc000325e30>] really_probe+0xf0/0x220
>> [<ffffffc000326080>] __driver_attach+0xa0/0xc0
>> [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0
>> [<ffffffc0003258bc>] driver_attach+0x1c/0x40
>> [<ffffffc00032548c>] bus_add_driver+0x14c/0x220
>> [<ffffffc00032683c>] driver_register+0x5c/0x120
>> [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80
>> [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20
>> [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160
>> [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8
>> [<ffffffc0004f07ac>] kernel_init+0xc/0xe0
>> ---[ end trace 3ee052d463aab7f3 ]---
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1 at
>> /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380
>> pci_device_add+0x128/0x140()
>> -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> I have made a small fix above your patch. After the fix is applied,
>> dumps are gone and the enumeration finishes up smoothly for all the
>> ports.
>> Since the change is small, just pasting it here. Please review and
>> apply if it's clean.
>
> Honestly, I have no idea. I kept staring at the code for a better part of an hour
> trying to decipher what the intent of the code was, without too much progress. I
> still don't understand why the code in pci_alloc_child_bus() takes a shortcut when
> the bridge argument is NULL when in my opinion it should use parent->bridge instead
> and continue as normal.
>
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index a12cda5..aac8366 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct
>> pci_bus *parent,
>>         }
>>
>>         child->self = bridge;
>> -       child->bridge = get_device(&bridge->dev);
>> +       child->bridge = get_device(parent->bridge);
>>         child->dev.parent = child->bridge;
>
> Hmm, not sure why this is needed. What does get_device(&bridge->dev)
> return for you? The next line sets child->dev.parent to child->bridge,
> but with your change I'm not sure we end up using the correct parent.
>
> Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64
> to look like this:
>
> static inline int pci_domain_nr(struct pci_bus *bus)
> {
>         struct pci_host_bridge *bridge;
>
>         while (bus->parent)
>                 bus = bus->parent;
>
>         bridge = to_pci_host_bridge(bus->bridge);
>         if (bridge)
>                 return bridge->domain_nr;
>
>         return 0;
> }
>

This did not work for me.


> Please let me know what results you get.
>
I am printing following values

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a12cda5..c89f86a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -695,6 +695,8 @@ static struct pci_bus *pci_alloc_child_bus(struct
pci_bus *parent,
        child->self = bridge;
        child->bridge = get_device(&bridge->dev);
        child->dev.parent = child->bridge;
+       printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",
+               __func__, __LINE__, child, child->bridge,
pci_domain_nr(parent));
        pci_set_bus_of_node(child);
        pci_set_bus_speed(child);

@@ -1095,6 +1097,8 @@ int pci_setup_device(struct pci_dev *dev)
        dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
                     dev->bus->number, PCI_SLOT(dev->devfn),
                     PCI_FUNC(dev->devfn));
+       printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",
+               __func__, __LINE__, dev->bus, dev->bus->bridge,
pci_domain_nr(dev->bus));

        pci_read_config_dword(dev, PCI_CLASS_REVISION, &class);
        dev->revision = class & 0xff;

Following looks suspicious to me.

bridge_dev = ffffffc7e03ffc00 for bus 0 in domain 1 while bridge_dev =
ffffffc7e03f7098 for bus 1 in domain 1.

 Log -->
-----------------------------------------------------------------------------------------------------------------------------
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up
pci_bus 0001:00: scanning bus
pci_setup_device:1101 bus = ffffffc7e0060400 , bridge_dev =
ffffffc7e03ffc00, domain = 1
pci 0001:00:00.0: [e008:e004] type 01 class 0x060400
pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit]
pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80
pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0
pci 0001:00:00.0: supports D1 D2
pci_bus 0001:00: fixups for bus
pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0
pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring
pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1
pci_alloc_child_bus:699 bus = ffffffc7e0063000 , bridge_dev =
ffffffc7e03f7098, domain = 1
pci_bus 0001:01: scanning bus
pci_setup_device:1101 bus = ffffffc7e0063000 , bridge_dev =
ffffffc7e03f7098, domain = 0
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000
pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit]
pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref]
pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref]
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at
/home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52
sysfs_warn_dup+0x80/0xc0()
sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0'
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #50
-----------------------------------------------------------------------------------------------------------------------------


> Best regards,
> Liviu
>
>
>>         pci_set_bus_of_node(child);
>>         pci_set_bus_speed(child);
>>
>> Thanks,
>> Tanmay
>>
>> On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
>> > pci_alloc_child_bus() uses the newly allocated child bus to figure
>> > out the domain number that is going to use for setting the device
>> > name. A better option is to use the parent bus domain number.
>> >
>> > Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
>> >
>> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> > index 26237a0..a12cda5 100644
>> > --- a/drivers/pci/probe.c
>> > +++ b/drivers/pci/probe.c
>> > @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>> >          * now as the parent is not properly set up yet.
>> >          */
>> >         child->dev.class = &pcibus_class;
>> > -       dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
>> > +       dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr);
>> >
>> >         /*
>> >          * Set up the primary, secondary and subordinate
>> > --
>> > 1.9.0
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> -------------------
>    .oooO
>    (   )
>     \ (  Oooo.
>      \_) (   )
>           ) /
>          (_/
>
>  One small step
>    for me ...
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [CentOS ARM]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]     [Photos]