From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liviu Dudau Subject: Re: [PATCH v4 5/6] pci: Use parent domain number when allocating child busses. Date: Mon, 3 Mar 2014 23:51:32 +0000 Message-ID: <20140303235132.GA13582@bart.dudau.co.uk> References: <1393867996-32299-1-git-send-email-Liviu.Dudau@arm.com> <1393867996-32299-6-git-send-email-Liviu.Dudau@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Tanmay Inamdar Cc: "devicetree@vger.kernel.org" , linaro-kernel , Benjamin Herrenschmidt , linux-pci , Liviu Dudau , LKML , Will Deacon , Catalin Marinas , Bjorn Helgaas , LAKML List-Id: devicetree@vger.kernel.org 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? > 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: > [] dump_backtrace+0x0/0x140 > [] show_stack+0x10/0x20 > [] dump_stack+0x74/0xc4 > [] warn_slowpath_common+0x84/0xc0 > [] warn_slowpath_fmt+0x4c/0x60 > [] sysfs_warn_dup+0x7c/0xc0 > [] sysfs_do_create_link_sd.isra.2+0xf4/0x100 > [] sysfs_create_link+0x1c/0x40 > [] bus_add_device+0x110/0x1c0 > [] device_add+0x31c/0x520 > [] pci_device_add+0xec/0x140 > [] pci_scan_single_device+0x98/0xe0 > [] pci_scan_slot+0x48/0x120 > [] pci_scan_child_bus+0x48/0x140 > [] pci_scan_bridge+0x4ec/0x5e0 > [] pci_scan_child_bus+0xa8/0x140 > [] pci_rescan_bus+0x10/0x40 > [] xgene_pcie_probe_bridge+0x660/0x72c > [] platform_drv_probe+0x20/0x60 > [] really_probe+0xf0/0x220 > [] __driver_attach+0xa0/0xc0 > [] bus_for_each_dev+0x54/0xa0 > [] driver_attach+0x1c/0x40 > [] bus_add_driver+0x14c/0x220 > [] driver_register+0x5c/0x120 > [] __platform_driver_register+0x5c/0x80 > [] xgene_pcie_driver_init+0x14/0x20 > [] do_one_initcall+0xe0/0x160 > [] kernel_init_freeable+0x134/0x1d8 > [] 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; } Please let me know what results you get. 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 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 > > > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- ------------------- .oooO ( ) \ ( Oooo. \_) ( ) ) / (_/ One small step for me ...