* [v2 PATCH] PCI: Do not use bus number zero from EA capability
@ 2019-11-04 6:57 sundeep.lkml
2019-11-11 23:37 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: sundeep.lkml @ 2019-11-04 6:57 UTC (permalink / raw)
To: helgaas, bhelgaas, linux-pci, linux-kernel
Cc: stable, sgoutham, Subbaraya Sundeep
From: Subbaraya Sundeep <sbhatta@marvell.com>
As per the spec, "Enhanced Allocation (EA) for Memory
and I/O Resources" ECN, approved 23 October 2014,
sec 6.9.1.2, fixed bus numbers of a bridge must be zero
when no function that uses EA is located behind it.
Hence assign bus numbers normally instead of assigning
zeroes from EA capability. Failing to do this and using
zeroes from EA would make the bridges non-functional.
Fixes: '2dbce5901179 ("PCI: Assign bus numbers present in
EA capability for bridges")'
Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
Cc: stable@vger.kernel.org # v5.2+
---
drivers/pci/probe.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3d5271a..116b276 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1090,27 +1090,28 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
* @sub: updated with subordinate bus number from EA
*
* If @dev is a bridge with EA capability, update @sec and @sub with
- * fixed bus numbers from the capability and return true. Otherwise,
- * return false.
+ * fixed bus numbers from the capability. Otherwise @sec and @sub
+ * will be zeroed.
*/
-static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
+static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
{
int ea, offset;
u32 dw;
+ *sec = *sub = 0;
+
if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
- return false;
+ return;
/* find PCI EA capability in list */
ea = pci_find_capability(dev, PCI_CAP_ID_EA);
if (!ea)
- return false;
+ return;
offset = ea + PCI_EA_FIRST_ENT;
pci_read_config_dword(dev, offset, &dw);
*sec = dw & PCI_EA_SEC_BUS_MASK;
*sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
- return true;
}
/*
@@ -1146,7 +1147,6 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
u16 bctl;
u8 primary, secondary, subordinate;
int broken = 0;
- bool fixed_buses;
u8 fixed_sec, fixed_sub;
int next_busnr;
@@ -1249,11 +1249,12 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
pci_write_config_word(dev, PCI_STATUS, 0xffff);
/* Read bus numbers from EA Capability (if present) */
- fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
- if (fixed_buses)
+ pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
+
+ next_busnr = max + 1;
+ /* Use secondary bus number in EA */
+ if (fixed_sec)
next_busnr = fixed_sec;
- else
- next_busnr = max + 1;
/*
* Prevent assigning a bus number that already exists.
@@ -1331,7 +1332,7 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
* If fixed subordinate bus number exists from EA
* capability then use it.
*/
- if (fixed_buses)
+ if (fixed_sub)
max = fixed_sub;
pci_bus_update_busn_res_end(child, max);
pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [v2 PATCH] PCI: Do not use bus number zero from EA capability 2019-11-04 6:57 [v2 PATCH] PCI: Do not use bus number zero from EA capability sundeep.lkml @ 2019-11-11 23:37 ` Bjorn Helgaas 2019-11-18 7:31 ` sundeep subbaraya 0 siblings, 1 reply; 3+ messages in thread From: Bjorn Helgaas @ 2019-11-11 23:37 UTC (permalink / raw) To: sundeep.lkml; +Cc: linux-pci, linux-kernel, stable, sgoutham, Subbaraya Sundeep On Mon, Nov 04, 2019 at 12:27:44PM +0530, sundeep.lkml@gmail.com wrote: > From: Subbaraya Sundeep <sbhatta@marvell.com> > > As per the spec, "Enhanced Allocation (EA) for Memory > and I/O Resources" ECN, approved 23 October 2014, > sec 6.9.1.2, fixed bus numbers of a bridge must be zero > when no function that uses EA is located behind it. > Hence assign bus numbers normally instead of assigning > zeroes from EA capability. Failing to do this and using > zeroes from EA would make the bridges non-functional. > > Fixes: '2dbce5901179 ("PCI: Assign bus numbers present in > EA capability for bridges")' > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> > Cc: stable@vger.kernel.org # v5.2+ Applied to pci/resource for v5.5, thanks! I tweaked it as below so the logic about how to interpret the EA Fixed Secondary Bus Number is more localized. Let me know if that doesn't make sense. > --- > drivers/pci/probe.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 3d5271a..116b276 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1090,27 +1090,28 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > * @sub: updated with subordinate bus number from EA > * > * If @dev is a bridge with EA capability, update @sec and @sub with > - * fixed bus numbers from the capability and return true. Otherwise, > - * return false. > + * fixed bus numbers from the capability. Otherwise @sec and @sub > + * will be zeroed. > */ > -static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > { > int ea, offset; > u32 dw; > > + *sec = *sub = 0; > + > if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) > - return false; > + return; > > /* find PCI EA capability in list */ > ea = pci_find_capability(dev, PCI_CAP_ID_EA); > if (!ea) > - return false; > + return; > > offset = ea + PCI_EA_FIRST_ENT; > pci_read_config_dword(dev, offset, &dw); > *sec = dw & PCI_EA_SEC_BUS_MASK; > *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; > - return true; > } > > /* > @@ -1146,7 +1147,6 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > u16 bctl; > u8 primary, secondary, subordinate; > int broken = 0; > - bool fixed_buses; > u8 fixed_sec, fixed_sub; > int next_busnr; > > @@ -1249,11 +1249,12 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > pci_write_config_word(dev, PCI_STATUS, 0xffff); > > /* Read bus numbers from EA Capability (if present) */ > - fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub); > - if (fixed_buses) > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub); > + > + next_busnr = max + 1; > + /* Use secondary bus number in EA */ > + if (fixed_sec) > next_busnr = fixed_sec; > - else > - next_busnr = max + 1; > > /* > * Prevent assigning a bus number that already exists. > @@ -1331,7 +1332,7 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > * If fixed subordinate bus number exists from EA > * capability then use it. > */ > - if (fixed_buses) > + if (fixed_sub) > max = fixed_sub; > pci_bus_update_busn_res_end(child, max); > pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); > -- > 2.7.4 > commit 25328e0447de ("PCI: Do not use bus number zero from EA capability") Author: Subbaraya Sundeep <sbhatta@marvell.com> Date: Mon Nov 4 12:27:44 2019 +0530 PCI: Do not use bus number zero from EA capability As per PCIe r5.0, sec 7.8.5.2, fixed bus numbers of a bridge must be zero when no function that uses EA is located behind it. Hence, if EA supplies bus numbers of zero, assign bus numbers normally. A secondary bus can never have a bus number of zero, so setting a bridge's Secondary Bus Number to zero makes downstream devices unreachable. [bhelgaas: retain bool return value so "zero is invalid" logic is local] Fixes: 2dbce5901179 ("PCI: Assign bus numbers present in EA capability for bridges") Link: https://lore.kernel.org/r/1572850664-9861-1-git-send-email-sundeep.lkml@gmail.com Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: stable@vger.kernel.org # v5.2+ diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index bdbc8490f962..d3033873395d 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1090,14 +1090,15 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, * @sec: updated with secondary bus number from EA * @sub: updated with subordinate bus number from EA * - * If @dev is a bridge with EA capability, update @sec and @sub with - * fixed bus numbers from the capability and return true. Otherwise, - * return false. + * If @dev is a bridge with EA capability that specifies valid secondary + * and subordinate bus numbers, return true with the bus numbers in @sec + * and @sub. Otherwise return false. */ static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) { int ea, offset; u32 dw; + u8 ea_sec, ea_sub; if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) return false; @@ -1109,8 +1110,13 @@ static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) offset = ea + PCI_EA_FIRST_ENT; pci_read_config_dword(dev, offset, &dw); - *sec = dw & PCI_EA_SEC_BUS_MASK; - *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; + ea_sec = dw & PCI_EA_SEC_BUS_MASK; + ea_sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; + if (ea_sec == 0 || ea_sub < ea_sec) + return false; + + *sec = ea_sec; + *sub = ea_sub; return true; } ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [v2 PATCH] PCI: Do not use bus number zero from EA capability 2019-11-11 23:37 ` Bjorn Helgaas @ 2019-11-18 7:31 ` sundeep subbaraya 0 siblings, 0 replies; 3+ messages in thread From: sundeep subbaraya @ 2019-11-18 7:31 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, linux-kernel, stable, sgoutham, Subbaraya Sundeep Hi Bjorn, On Tue, Nov 12, 2019 at 5:08 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Nov 04, 2019 at 12:27:44PM +0530, sundeep.lkml@gmail.com wrote: > > From: Subbaraya Sundeep <sbhatta@marvell.com> > > > > As per the spec, "Enhanced Allocation (EA) for Memory > > and I/O Resources" ECN, approved 23 October 2014, > > sec 6.9.1.2, fixed bus numbers of a bridge must be zero > > when no function that uses EA is located behind it. > > Hence assign bus numbers normally instead of assigning > > zeroes from EA capability. Failing to do this and using > > zeroes from EA would make the bridges non-functional. > > > > Fixes: '2dbce5901179 ("PCI: Assign bus numbers present in > > EA capability for bridges")' > > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> > > Cc: stable@vger.kernel.org # v5.2+ > > Applied to pci/resource for v5.5, thanks! > > I tweaked it as below so the logic about how to interpret the EA Fixed > Secondary Bus Number is more localized. Let me know if that doesn't > make sense. > Looks good. Thanks for applying the patch. Sundeep > > --- > > drivers/pci/probe.c | 25 +++++++++++++------------ > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 3d5271a..116b276 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1090,27 +1090,28 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > > * @sub: updated with subordinate bus number from EA > > * > > * If @dev is a bridge with EA capability, update @sec and @sub with > > - * fixed bus numbers from the capability and return true. Otherwise, > > - * return false. > > + * fixed bus numbers from the capability. Otherwise @sec and @sub > > + * will be zeroed. > > */ > > -static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > > { > > int ea, offset; > > u32 dw; > > > > + *sec = *sub = 0; > > + > > if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) > > - return false; > > + return; > > > > /* find PCI EA capability in list */ > > ea = pci_find_capability(dev, PCI_CAP_ID_EA); > > if (!ea) > > - return false; > > + return; > > > > offset = ea + PCI_EA_FIRST_ENT; > > pci_read_config_dword(dev, offset, &dw); > > *sec = dw & PCI_EA_SEC_BUS_MASK; > > *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; > > - return true; > > } > > > > /* > > @@ -1146,7 +1147,6 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > > u16 bctl; > > u8 primary, secondary, subordinate; > > int broken = 0; > > - bool fixed_buses; > > u8 fixed_sec, fixed_sub; > > int next_busnr; > > > > @@ -1249,11 +1249,12 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > > pci_write_config_word(dev, PCI_STATUS, 0xffff); > > > > /* Read bus numbers from EA Capability (if present) */ > > - fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub); > > - if (fixed_buses) > > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub); > > + > > + next_busnr = max + 1; > > + /* Use secondary bus number in EA */ > > + if (fixed_sec) > > next_busnr = fixed_sec; > > - else > > - next_busnr = max + 1; > > > > /* > > * Prevent assigning a bus number that already exists. > > @@ -1331,7 +1332,7 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > > * If fixed subordinate bus number exists from EA > > * capability then use it. > > */ > > - if (fixed_buses) > > + if (fixed_sub) > > max = fixed_sub; > > pci_bus_update_busn_res_end(child, max); > > pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); > > -- > > 2.7.4 > > > > commit 25328e0447de ("PCI: Do not use bus number zero from EA capability") > Author: Subbaraya Sundeep <sbhatta@marvell.com> > Date: Mon Nov 4 12:27:44 2019 +0530 > > PCI: Do not use bus number zero from EA capability > > As per PCIe r5.0, sec 7.8.5.2, fixed bus numbers of a bridge must be zero > when no function that uses EA is located behind it. Hence, if EA supplies > bus numbers of zero, assign bus numbers normally. A secondary bus can > never have a bus number of zero, so setting a bridge's Secondary Bus Number > to zero makes downstream devices unreachable. > > [bhelgaas: retain bool return value so "zero is invalid" logic is local] > Fixes: 2dbce5901179 ("PCI: Assign bus numbers present in EA capability for bridges") > Link: https://lore.kernel.org/r/1572850664-9861-1-git-send-email-sundeep.lkml@gmail.com > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: stable@vger.kernel.org # v5.2+ > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index bdbc8490f962..d3033873395d 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1090,14 +1090,15 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > * @sec: updated with secondary bus number from EA > * @sub: updated with subordinate bus number from EA > * > - * If @dev is a bridge with EA capability, update @sec and @sub with > - * fixed bus numbers from the capability and return true. Otherwise, > - * return false. > + * If @dev is a bridge with EA capability that specifies valid secondary > + * and subordinate bus numbers, return true with the bus numbers in @sec > + * and @sub. Otherwise return false. > */ > static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > { > int ea, offset; > u32 dw; > + u8 ea_sec, ea_sub; > > if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) > return false; > @@ -1109,8 +1110,13 @@ static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub) > > offset = ea + PCI_EA_FIRST_ENT; > pci_read_config_dword(dev, offset, &dw); > - *sec = dw & PCI_EA_SEC_BUS_MASK; > - *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; > + ea_sec = dw & PCI_EA_SEC_BUS_MASK; > + ea_sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT; > + if (ea_sec == 0 || ea_sub < ea_sec) > + return false; > + > + *sec = ea_sec; > + *sub = ea_sub; > return true; > } > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-18 7:31 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 6:57 [v2 PATCH] PCI: Do not use bus number zero from EA capability sundeep.lkml 2019-11-11 23:37 ` Bjorn Helgaas 2019-11-18 7:31 ` sundeep subbaraya
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).