* [PATCH] PCI: Do not use bus number zero from EA capability
@ 2019-09-02 15:30 sundeep.lkml
2019-09-23 7:00 ` sundeep subbaraya
2019-09-23 12:35 ` Andrew Murray
0 siblings, 2 replies; 4+ messages in thread
From: sundeep.lkml @ 2019-09-02 15:30 UTC (permalink / raw)
To: helgaas, linux-pci, linux-kernel, sean.stalley, bhelgaas
Cc: 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 can be zero
when no function that uses EA is located behind it.
Hence assign bus numbers sequentially when fixed bus
numbers are zero.
Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
---
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 a3c7338..c06ca4c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1095,27 +1095,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;
}
/*
@@ -1151,7 +1152,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;
@@ -1254,11 +1254,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.
@@ -1336,7 +1337,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] 4+ messages in thread
* Re: [PATCH] PCI: Do not use bus number zero from EA capability
2019-09-02 15:30 [PATCH] PCI: Do not use bus number zero from EA capability sundeep.lkml
@ 2019-09-23 7:00 ` sundeep subbaraya
2019-09-23 12:35 ` Andrew Murray
1 sibling, 0 replies; 4+ messages in thread
From: sundeep subbaraya @ 2019-09-23 7:00 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, linux-kernel, Stalley, Sean, Bjorn Helgaas
Cc: sgoutham, Subbaraya Sundeep
Hi Bjorn,
Please let me know if you have any comments on the patch.
Thanks,
Sundeep
On Mon, Sep 2, 2019 at 9:00 PM <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 can be zero
> when no function that uses EA is located behind it.
> Hence assign bus numbers sequentially when fixed bus
> numbers are zero.
>
> Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
>
> Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
> ---
> 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 a3c7338..c06ca4c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1095,27 +1095,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;
> }
>
> /*
> @@ -1151,7 +1152,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;
>
> @@ -1254,11 +1254,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.
> @@ -1336,7 +1337,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 [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: Do not use bus number zero from EA capability
2019-09-02 15:30 [PATCH] PCI: Do not use bus number zero from EA capability sundeep.lkml
2019-09-23 7:00 ` sundeep subbaraya
@ 2019-09-23 12:35 ` Andrew Murray
2019-09-24 9:56 ` sundeep subbaraya
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Murray @ 2019-09-23 12:35 UTC (permalink / raw)
To: sundeep.lkml
Cc: helgaas, linux-pci, linux-kernel, sean.stalley, bhelgaas,
sgoutham, Subbaraya Sundeep
On Mon, Sep 02, 2019 at 09:00:03PM +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 can be zero
s/can/must/
The spec uses the term *must*. "Can" implies that this is optional.
> when no function that uses EA is located behind it.
> Hence assign bus numbers sequentially when fixed bus
> numbers are zero.
Perhaps s/sequentially/as per normal/ or similar. As we're not doing
anything different here.
>
> Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
Is it worth describing what actually goes wrong without this patch - and
when this occurs? I guess it's possible for a bridge to have an EA
capability, but no devices using EA behind it - and thus in this suitation
the downstream devices have unnecessary bus number constraints?
>
> Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
Does this need to be CC'd to stable?
> ---
> 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 a3c7338..c06ca4c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1095,27 +1095,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;
Is there any value in doing any sanity checking here? E.g. sub !=0, sub > sec?
> - return true;
> }
>
> /*
> @@ -1151,7 +1152,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;
>
> @@ -1254,11 +1254,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;
There is a subtle style change here (assigning and then potentially reassigning
with a new value vs assigning once using both if/else). No idea if this matters
but I thought I'd point it out in case it wasn't intentional.
Thanks,
Andrew Murray
>
> /*
> * Prevent assigning a bus number that already exists.
> @@ -1336,7 +1337,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 [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: Do not use bus number zero from EA capability
2019-09-23 12:35 ` Andrew Murray
@ 2019-09-24 9:56 ` sundeep subbaraya
0 siblings, 0 replies; 4+ messages in thread
From: sundeep subbaraya @ 2019-09-24 9:56 UTC (permalink / raw)
To: Andrew Murray
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Stalley, Sean,
Bjorn Helgaas, sgoutham, Subbaraya Sundeep
Hi Andrew,
On Mon, Sep 23, 2019 at 6:05 PM Andrew Murray <andrew.murray@arm.com> wrote:
>
> On Mon, Sep 02, 2019 at 09:00:03PM +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 can be zero
>
> s/can/must/
>
> The spec uses the term *must*. "Can" implies that this is optional.
>
Yes will change to must.
> > when no function that uses EA is located behind it.
> > Hence assign bus numbers sequentially when fixed bus
> > numbers are zero.
>
> Perhaps s/sequentially/as per normal/ or similar. As we're not doing
> anything different here.
>
Ok will change
> >
> > Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
>
> Is it worth describing what actually goes wrong without this patch - and
> when this occurs? I guess it's possible for a bridge to have an EA
> capability, but no devices using EA behind it - and thus in this suitation
> the downstream devices have unnecessary bus number constraints?
>
EA is for functions which are permanently connected to host bridge.
In our case all the on chip devices use EA and bridges which are there for
connecting off chip devices use EA with fixed bus numbers as zero.
Bus numbers for those bridges need to be configured as per
normal enumeration, failing to do so makes those bridges not functional
because secondary and subordinate bus numbers are 0.
> >
> > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
>
> Does this need to be CC'd to stable?
>
Ok will CC stable from v2
> > ---
> > 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 a3c7338..c06ca4c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1095,27 +1095,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;
>
> Is there any value in doing any sanity checking here? E.g. sub !=0, sub > sec?
>
I don't think it is needed since we read hardwired values from HW.
> > - return true;
> > }
> >
> > /*
> > @@ -1151,7 +1152,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;
> >
> > @@ -1254,11 +1254,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;
>
> There is a subtle style change here (assigning and then potentially reassigning
> with a new value vs assigning once using both if/else). No idea if this matters
> but I thought I'd point it out in case it wasn't intentional.
>
This is intentional just to avoid else case.
Thanks for review. I will send v2.
Sundeep
> Thanks,
>
> Andrew Murray
>
> >
> > /*
> > * Prevent assigning a bus number that already exists.
> > @@ -1336,7 +1337,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 [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-24 9:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 15:30 [PATCH] PCI: Do not use bus number zero from EA capability sundeep.lkml
2019-09-23 7:00 ` sundeep subbaraya
2019-09-23 12:35 ` Andrew Murray
2019-09-24 9:56 ` 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).