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

[Ray Jui <rjui@broadcom.com>]

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@gmail.com>
>> Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support")
>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>
> 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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html