Re: [PATCH v2] PCI: iproc: Fix BCMA PCIe bus scanning regression

[Ray Jui]

On 1/27/2016 2:52 PM, Bjorn Helgaas wrote:
> On Tue, Jan 26, 2016 at 03:31:40PM -0800, Ray Jui wrote:
> > Commit 943ebae781f5 ("PCI: iproc: Add PAXC interface support") causes
> > regression on EP device detection on BCMA based platforms. This patch
> > fixes the issue by allowing multiple devices to be configured on the
> > same bus, for all PAXB based child buses. In addition, this patch also
> > adds check to prevent non-zero function from being used on bus 0 (root
> > bus).
> >
> > Function 'iproc_pcie_device_is_valid' is now removed with checks
> > folding into 'iproc_pcie_map_cfg_bus' to make them more clear and less
> > error-prone
> >
> > Reported-by: Rafal Milecki <zajec5@xxxxxxxxx>
> > Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support")
> > Signed-off-by: Ray Jui <rjui@xxxxxxxxxxxx>
>
> Since this fixes a regression, I applied this to for-linus for v4.5,
> thanks, Ray.

Thanks!

> I still have one clarification question below.
>
> > ---
> >   drivers/pci/host/pcie-iproc.c | 28 +++++++++++-----------------
> >   1 file changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> > index 5816bce..67396ab 100644
> > --- a/drivers/pci/host/pcie-iproc.c
> > +++ b/drivers/pci/host/pcie-iproc.c
> > @@ -170,20 +170,6 @@ static inline void iproc_pcie_ob_write(struct iproc_pcie *pcie,
> >   	writel(val, pcie->base + offset + (window * 8));
> >   }
> >
> > -static inline bool iproc_pcie_device_is_valid(struct iproc_pcie *pcie,
> > -					      unsigned int slot,
> > -					      unsigned int fn)
> > -{
> > -	if (slot > 0)
> > -		return false;
> > -
> > -	/* PAXC can only support limited number of functions */
> > -	if (pcie->type == IPROC_PCIE_PAXC && fn >= MAX_NUM_PAXC_PF)
> > -		return false;
> > -
> > -	return true;
> > -}
> > -
> >   /**
> >    * Note access to the configuration registers are protected at the higher layer
> >    * by 'pci_lock' in drivers/pci/access.c
> > @@ -199,11 +185,11 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
> >   	u32 val;
> >   	u16 offset;
> >
> > -	if (!iproc_pcie_device_is_valid(pcie, slot, fn))
> > -		return NULL;
> > -
> >   	/* root complex access */
> >   	if (busno == 0) {
> > +		if (slot > 0 || fn > 0)
> > +			return NULL;
> > +
>
> This looks good and makes sense since config access to root bus is
> fundamentally different from other access.

The new code does look cleaner. Thanks for the suggestion!


> >   		iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_IND_ADDR,
> >   				     where & CFG_IND_ADDR_MASK);
> >   		offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_IND_DATA);
> > @@ -213,6 +199,14 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
> >   			return (pcie->base + offset);
> >   	}
> >
> > +	/*
> > +	 * PAXC is connected to internally emulated EP within the SoC. It
> > +	 * allows only one device and supports limited number of functions
> > +	 */
> > +	if (pcie->type == IPROC_PCIE_PAXC)
> > +		if (slot > 0 || fn >= MAX_NUM_PAXC_PF)
> > +			return NULL;
>
> Is this really necessary?  I assume 00:00.0 is a Root Port leading to
> bus 01, and 01:00.0, 01:00.1, 01:00.2, and 01:00.3 are the functions
> of the internal EP.  So this test prevents us from issuing a config
> request to devices like 01:00.4.
>
> I would assume the Root Port is standard and would handle a config
> request for 01:00.4 correctly, i.e., convert the type 1 request to
> type 0 (since it targets the Root Port's secondary bus), and forward
> it to the link.
>
> The endpoint should be responsible for handling it as an Unsupported
> Request, since it addresses an unimplemented function.  But maybe this
> embedded EP doesn't do that correctly?

Okay. I'll need to do slightly more investigation and experiment on this 
and after that I'll get back to you. It might take a while since I'm now 
extremely busy with some other tasks.... :(

In addition, this behavior might change slightly between A0 and B0 
revision of our chip....

> Also, assuming we *do* need this PAXC testing, do you want to test for
> "busno == 1" as well?  The PCI core shouldn't try to access bus 2
> unless there's a bridge from bus 1 to bus 2, but a user could use
> things like setpci to issue random config requests.
>
> > +
> >   	/* EP device access */
> >   	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
> >   		(slot << CFG_ADDR_DEV_NUM_SHIFT) |
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html