| Message ID | 1515508941-20055-1-git-send-email-koen.vandeputte@ncentric.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jan 09, 2018 at 03:42:21PM +0100, Koen Vandeputte wrote: > The subordinate value indicates the highest bus number which can be > reached downstream though a certain device. > > Commit a20c7f36bd3d ("PCI: Do not allocate more buses than available in > parent") > ensures that downstream devices cannot assign busnumbers higher than the > upstream device subordinate number, which was indeed illogical. > > By default, dw_pcie_setup_rc() inits the Root Complex subordinate to a > value of 0x01. > > Due to this combined with above commit, enumeration stops digging deeper > downstream as soon as bus num 0x01 has been assigned, which is always > the case for a bridge device. > > This results in all devices behind a bridge bus to remain undetected, as > these would be connected to bus 0x02 or higher. > > Fix this by initializing the RC to a subordinate value of 0xff, meaning > that all busses [0x00-0xff] are reachable through this RC. This is not a correct description of the problem. AFAICS all busses are reachable through this RC _regardless_ of whatever subordinate bus number value you programme into it. You should extend the CC list to all dwc host submaintainers so that you can actually get it tested. > Fixes: a20c7f36bd3d ("PCI: Do not allocate more buses than available in > parent") > Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com> > Tested-by: Niklas Cassel <niklas.cassel@axis.com> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > > Will send separate patches to stable as this file got moved/renamed Fixes: commit appeared at v4.15-rc1 (and v4.15 has not been released yet) - there is no separate patch to be sent. Thanks, Lorenzo > drivers/pci/dwc/pcie-designware-host.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > index bf558df5b7b3..2b5470173196 100644 > --- a/drivers/pci/dwc/pcie-designware-host.c > +++ b/drivers/pci/dwc/pcie-designware-host.c > @@ -616,7 +616,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > /* setup bus numbers */ > val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); > val &= 0xff000000; > - val |= 0x00010100; > + val |= 0x00ff0100; > dw_pcie_writel_dbi(pci, PCI_PRIMARY_BUS, val); > > /* setup command register */ > -- > 2.7.4 >
On Tue, Jan 09, 2018 at 03:25:58PM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 09, 2018 at 03:42:21PM +0100, Koen Vandeputte wrote: > > The subordinate value indicates the highest bus number which can be > > reached downstream though a certain device. > > > > Commit a20c7f36bd3d ("PCI: Do not allocate more buses than available in > > parent") > > ensures that downstream devices cannot assign busnumbers higher than the > > upstream device subordinate number, which was indeed illogical. > > > > By default, dw_pcie_setup_rc() inits the Root Complex subordinate to a > > value of 0x01. > > > > Due to this combined with above commit, enumeration stops digging deeper > > downstream as soon as bus num 0x01 has been assigned, which is always > > the case for a bridge device. > > > > This results in all devices behind a bridge bus to remain undetected, as > > these would be connected to bus 0x02 or higher. > > > > Fix this by initializing the RC to a subordinate value of 0xff, meaning > > that all busses [0x00-0xff] are reachable through this RC. > > This is not a correct description of the problem. AFAICS all busses > are reachable through this RC _regardless_ of whatever subordinate > bus number value you programme into it. Type 1 (the ones directed to the other side of the bridge) configuration transactions are not forwarded if the bus number in the transaction is not included in secondary and subordinate numbers of the root bridge. I think the description here is pretty accurate as the bridges (anything higher than bus number 1) below are effectively hidden.
On Tue, Jan 09, 2018 at 10:00:25PM +0200, Mika Westerberg wrote: > On Tue, Jan 09, 2018 at 03:25:58PM +0000, Lorenzo Pieralisi wrote: > > On Tue, Jan 09, 2018 at 03:42:21PM +0100, Koen Vandeputte wrote: > > > The subordinate value indicates the highest bus number which can be > > > reached downstream though a certain device. > > > > > > Commit a20c7f36bd3d ("PCI: Do not allocate more buses than available in > > > parent") > > > ensures that downstream devices cannot assign busnumbers higher than the > > > upstream device subordinate number, which was indeed illogical. > > > > > > By default, dw_pcie_setup_rc() inits the Root Complex subordinate to a > > > value of 0x01. > > > > > > Due to this combined with above commit, enumeration stops digging deeper > > > downstream as soon as bus num 0x01 has been assigned, which is always > > > the case for a bridge device. > > > > > > This results in all devices behind a bridge bus to remain undetected, as > > > these would be connected to bus 0x02 or higher. > > > > > > Fix this by initializing the RC to a subordinate value of 0xff, meaning > > > that all busses [0x00-0xff] are reachable through this RC. > > > > This is not a correct description of the problem. AFAICS all busses > > are reachable through this RC _regardless_ of whatever subordinate > > bus number value you programme into it. > > Type 1 (the ones directed to the other side of the bridge) configuration > transactions are not forwarded if the bus number in the transaction is > not included in secondary and subordinate numbers of the root bridge. I > think the description here is pretty accurate as the bridges (anything > higher than bus number 1) below are effectively hidden. That's correct - but that's not what this patch is fixing which is what the commit log should describe. See: https://marc.info/?l=linux-arm-kernel&m=151540153522730&w=2
Hi Lorenzo, Apologies for the late reply, It's been really busy over here. On 2018-01-09 16:25, Lorenzo Pieralisi wrote: > <snip> >> Fix this by initializing the RC to a subordinate value of 0xff, meaning >> that all busses [0x00-0xff] are reachable through this RC. > This is not a correct description of the problem. AFAICS all busses > are reachable through this RC _regardless_ of whatever subordinate > bus number value you programme into it. Noted. This was written meaning: "as seen be the probing functions below" I'll try harder in V2 to actually include the message that it's not a HW related problem or influencing HW in any way, but really "tricking" the probing functions below > > You should extend the CC list to all dwc host submaintainers so > that you can actually get it tested. How to figure out who to include? (& please provide who to include) Reading online manuals on "how to send patches" only demo's "get_maintainer.pl" script >> <snip> >> --- >> >> Will send separate patches to stable as this file got moved/renamed > Fixes: commit appeared at v4.15-rc1 (and v4.15 has not been released > yet) - there is no separate patch to be sent. This is something typical which is hard to learn/understand by just reading "how to send patches" docs available everywhere. (like I did before sending this one over ;-) ) 2 questions basically: - As the commit causing it was included in 4.15-*RC1*, do I need to add a "Fixes; bla bla" at all? - As the commit causing it was backported to 4.9, (how) should I send a separate patch in order to get it fixed there? [0] > Thanks, > Lorenzo Probably annoying questions triggering a 'sigh' .. but I'm lacking experience here .. Thanks for your time & patience so far, Highly appreciated, Koen [0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=v4.9.76&id=9a4bf05126f42c2632729ab0da503021d74ed454
On Fri, Jan 12, 2018 at 04:56:33PM +0100, Koen Vandeputte wrote: > Hi Lorenzo, > > Apologies for the late reply, It's been really busy over here. > > > On 2018-01-09 16:25, Lorenzo Pieralisi wrote: > ><snip> > >>Fix this by initializing the RC to a subordinate value of 0xff, meaning > >>that all busses [0x00-0xff] are reachable through this RC. > >This is not a correct description of the problem. AFAICS all busses > >are reachable through this RC _regardless_ of whatever subordinate > >bus number value you programme into it. > Noted. > > This was written meaning: "as seen be the probing functions below" > I'll try harder in V2 to actually include the message that it's not > a HW related problem or influencing HW in any way, but really > "tricking" the probing functions below Ok, add it to the log please. > >You should extend the CC list to all dwc host submaintainers so > >that you can actually get it tested. > How to figure out who to include? (& please provide who to include) > Reading online manuals on "how to send patches" only demo's > "get_maintainer.pl" script Well, you can do it manually, by checking all pci/dwc submaintainers in MAINTAINERS. There are other ways of course but it is simple enough. > >><snip> > >>--- > >> > >>Will send separate patches to stable as this file got moved/renamed > >Fixes: commit appeared at v4.15-rc1 (and v4.15 has not been released > >yet) - there is no separate patch to be sent. > This is something typical which is hard to learn/understand by just > reading "how to send patches" docs available everywhere. (like I did > before sending this one over ;-) ) > > 2 questions basically: > - As the commit causing it was included in 4.15-*RC1*, do I need to > add a "Fixes; bla bla" at all? Yes. > - As the commit causing it was backported to 4.9, (how) should I > send a separate patch in order to get it fixed there? [0] So submitting a stable tag does make sense, sorry I missed that. Or we can send to stable kernel specific backports. > Probably annoying questions triggering a 'sigh' .. but I'm lacking > experience here .. No way, thank you for fixing it ! > Thanks for your time & patience so far, > Highly appreciated, Same here, thank you. Lorenzo
diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c index bf558df5b7b3..2b5470173196 100644 --- a/drivers/pci/dwc/pcie-designware-host.c +++ b/drivers/pci/dwc/pcie-designware-host.c @@ -616,7 +616,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp) /* setup bus numbers */ val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); val &= 0xff000000; - val |= 0x00010100; + val |= 0x00ff0100; dw_pcie_writel_dbi(pci, PCI_PRIMARY_BUS, val); /* setup command register */
The subordinate value indicates the highest bus number which can be reached downstream though a certain device. Commit a20c7f36bd3d ("PCI: Do not allocate more buses than available in parent") ensures that downstream devices cannot assign busnumbers higher than the upstream device subordinate number, which was indeed illogical. By default, dw_pcie_setup_rc() inits the Root Complex subordinate to a value of 0x01. Due to this combined with above commit, enumeration stops digging deeper downstream as soon as bus num 0x01 has been assigned, which is always the case for a bridge device. This results in all devices behind a bridge bus to remain undetected, as these would be connected to bus 0x02 or higher. Fix this by initializing the RC to a subordinate value of 0xff, meaning that all busses [0x00-0xff] are reachable through this RC. Fixes: a20c7f36bd3d ("PCI: Do not allocate more buses than available in parent") Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com> Tested-by: Niklas Cassel <niklas.cassel@axis.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> --- Will send separate patches to stable as this file got moved/renamed drivers/pci/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)