From: Subbaraya Sundeep <[email protected]>
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 <[email protected]>
Cc: [email protected] # 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
On Mon, Nov 04, 2019 at 12:27:44PM +0530, [email protected] wrote:
> From: Subbaraya Sundeep <[email protected]>
>
> 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 <[email protected]>
> Cc: [email protected] # 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 <[email protected]>
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/[email protected]
Signed-off-by: Subbaraya Sundeep <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: [email protected] # 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;
}
Hi Bjorn,
On Tue, Nov 12, 2019 at 5:08 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Mon, Nov 04, 2019 at 12:27:44PM +0530, [email protected] wrote:
> > From: Subbaraya Sundeep <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: [email protected] # 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 <[email protected]>
> 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/[email protected]
> Signed-off-by: Subbaraya Sundeep <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: [email protected] # 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;
> }
>