From patchwork Tue Mar 4 00:42:11 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tanmay Inamdar X-Patchwork-Id: 326097 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id EAB432C00AF for ; Tue, 4 Mar 2014 11:42:15 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755866AbaCDAmN (ORCPT ); Mon, 3 Mar 2014 19:42:13 -0500 Received: from exprod5og112.obsmtp.com ([64.18.0.24]:53948 "HELO exprod5og112.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1755343AbaCDAmM (ORCPT ); Mon, 3 Mar 2014 19:42:12 -0500 Received: from mail-ve0-f182.google.com ([209.85.128.182]) (using TLSv1) by exprod5ob112.postini.com ([64.18.4.12]) with SMTP ID DSNKUxUhZCVWs5cF+7MBkyT/eqV4eiAC/G+G@postini.com; Mon, 03 Mar 2014 16:42:12 PST Received: by mail-ve0-f182.google.com with SMTP id jw12so1813456veb.27 for ; Mon, 03 Mar 2014 16:42:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:content-type; bh=bX77NgxePciZ5F0KSbQVNHrEC0M7S7AKBhPw/GJ4HBQ=; b=LXeM6/jZoBekKfThRQT9DGwi1sDO7FZfMy4RfJurIla5CAdyytn3tr4AjFMKOi3wy7 F5ALEOQT0Y9uF3+M8BSDPDT8xZxz8L1ILKsy1MOMEM+vfv6tfa71kPkAYePGvP6Fl0Pv dN2Wxko7EYqpm1JiOvv2oncPqANUTjcQgRwiUVgeFUJ0RauQ9/Y4I7Huqe8wjk4a/u6z NRizilrWhmI7EedQfrurj0LJ8sYIvqFXZqHt0cLlQ5bpE7iFTBVAQR31iD5SuuGlF/Tf xpqbL9ftwbjlxIv2zQC/q49Qqy5BZ5Vwo/3BJMimdqSMfa9yEP3mBBZccmJRf3tx81Yr D06A== X-Gm-Message-State: ALoCoQk/Bkthyp+jWNuUbCIBniWmWnp61NDSd5GJ1fWOlVmUfjdxJAya6/wnRZQh+GQXZvFysMPvkQhxdjz996uqzmlWmJoTakmtaco2WYTFDbdDrYkcdYUB+wmJWlbEcenyZcWZeKKuHtnsbLdNkp85xXztjILp9K/wCNOVzsnTJPFdy8rHS/I= X-Received: by 10.220.88.204 with SMTP id b12mr19514817vcm.3.1393893731702; Mon, 03 Mar 2014 16:42:11 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.220.88.204 with SMTP id b12mr19514799vcm.3.1393893731591; Mon, 03 Mar 2014 16:42:11 -0800 (PST) Received: by 10.58.66.134 with HTTP; Mon, 3 Mar 2014 16:42:11 -0800 (PST) In-Reply-To: <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> <20140303235132.GA13582@bart.dudau.co.uk> Date: Mon, 3 Mar 2014 16:42:11 -0800 Message-ID: Subject: Re: [PATCH v4 5/6] pci: Use parent domain number when allocating child busses. From: Tanmay Inamdar To: Tanmay Inamdar , Liviu Dudau , linux-pci , Bjorn Helgaas , Catalin Marinas , Will Deacon , linaro-kernel , Benjamin Herrenschmidt , LKML , "devicetree@vger.kernel.org" , LAKML Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hello, Please see inline. On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau 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: >> [] 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; > } > This did not work for me. > Please let me know what results you get. > I am printing following values 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 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 ... > --- 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 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);