diff mbox

PCI: iproc: Remove redundant function number check for PAXC

Message ID 20160129173041.GD12965@localhost
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Jan. 29, 2016, 5:30 p.m. UTC 
Hi Ray,

On Thu, Jan 28, 2016 at 03:37:20PM -0800, Ray Jui wrote:
> This patch removes the conditional check that limits the number of
> functions to be supported by the internally emulated endpoint device
> connected to PAXC. Investigation shows that the emulated PAXC endpoint
> device can properly reject request for unsupported functions, which
> makes this conditional check redundant
> 
> Signed-off-by: Ray Jui <rjui@broadcom.com>
> ---
>  drivers/pci/host/pcie-iproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 9ae43ed..b65185d 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -204,7 +204,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>  	 * allows only one device and supports a limited number of functions.
>  	 */
>  	if (pcie->type == IPROC_PCIE_PAXC)
> -		if (slot > 0 || fn >= MAX_NUM_PAXC_PF)
> +		if (slot > 0)
>  			return NULL;
>  
>  	/* EP device access */

Thanks for checking this out.  I removed the now-unused
MAX_NUM_PAXC_PF and folded this into the first patch, resulting in
the combined patch below.

I'm sorry to say that I have yet one more question.  It looks somewhat
hacky to have the PAXC-specific "slot > 0" test, and I'm not sure it
should be necessary (again, unless there's some implementation
deficiency in that PAXC embedded endpoint).  I'm looking at section
7.3 in the spec, and it seems like that endpoint *should* handle 
a config transaction with a non-zero Device Number, i.e., "slot", as
an Unsupported Request.  This should be standard behavior for all PCIe
endpoints -- we can generate config transactions like that on all root
complexes on all systems, so all endpoints should be able to handle
it.

Bjorn


commit 46560388c476c8471fde7712c10f9fad8d0d1875
Author: Ray Jui <rjui@broadcom.com>
Date:   Wed Jan 27 16:52:24 2016 -0600

    PCI: iproc: Allow multiple devices except on PAXC
    
    Commit 943ebae781f5 ("PCI: iproc: Add PAXC interface support") only allowed
    device 0, which is a regression on BCMA-based platforms.
    
    All systems support only one device, a Root Port at 00:00.0, on the root
    bus.  PAXC-based systems support only the Root Port (00:00.0) and a single
    device (with multiple functions) below it, e.g., 01:00.0, 01:00.1, etc.
    Non-PAXC systems support arbitrary devices below the Root Port.
    
    [bhelgaas: changelog, fold in removal of MAX_NUM_PAXC_PF check]
    Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support")
    Reported-by: Rafal Milecki <zajec5@gmail.com>
    Signed-off-by: Ray Jui <rjui@broadcom.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

--
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

Comments

Ray Jui Jan. 29, 2016, 5:53 p.m. UTC | #1 
Hi Bjorn,

On 1/29/2016 9:30 AM, Bjorn Helgaas wrote:
> Hi Ray,
>
> On Thu, Jan 28, 2016 at 03:37:20PM -0800, Ray Jui wrote:
>> This patch removes the conditional check that limits the number of
>> functions to be supported by the internally emulated endpoint device
>> connected to PAXC. Investigation shows that the emulated PAXC endpoint
>> device can properly reject request for unsupported functions, which
>> makes this conditional check redundant
>>
>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>> ---
>>   drivers/pci/host/pcie-iproc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 9ae43ed..b65185d 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -204,7 +204,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>>   	 * allows only one device and supports a limited number of functions.
>>   	 */
>>   	if (pcie->type == IPROC_PCIE_PAXC)
>> -		if (slot > 0 || fn >= MAX_NUM_PAXC_PF)
>> +		if (slot > 0)
>>   			return NULL;
>>
>>   	/* EP device access */
>
> Thanks for checking this out.  I removed the now-unused
> MAX_NUM_PAXC_PF and folded this into the first patch, resulting in
> the combined patch below.
>
> I'm sorry to say that I have yet one more question.

You don't need to feel sorry for asking these questions, :) It's your 
previous question that initiates the investigation and as a result, one 
more redundant check (and potential limitation for future iProc based 
SoCs) is removed from the driver. I really appreciate these questions 
from you.

> It looks somewhat
> hacky to have the PAXC-specific "slot > 0" test, and I'm not sure it
> should be necessary (again, unless there's some implementation
> deficiency in that PAXC embedded endpoint).  I'm looking at section
> 7.3 in the spec, and it seems like that endpoint *should* handle
> a config transaction with a non-zero Device Number, i.e., "slot", as
> an Unsupported Request.  This should be standard behavior for all PCIe
> endpoints -- we can generate config transactions like that on all root
> complexes on all systems, so all endpoints should be able to handle
> it.

Unfortunately, it looks like the integrated endpoint connected to PAXC 
is not fully compliant to the above described behavior.

I tested by removing the "slot > 0" test in the driver and added some 
debug prints, it appears that attempted access to slot 1, 2, 3 cannot be 
rejected properly and results an kernel crash.

Debugging prints are in the format of <bus>:<slot>:<func> offset:0x<where>

[    3.871332] 1:1:0 offset:0x0
[    3.874552] 1:2:0 offset:0x0
[    3.877759] 1:3:0 offset:0x0
[    3.881454] Bad mode in Error handler detected, code 0xbf000002 -- SError
[    3.888996] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0+ #117
[    3.895801] Hardware name: Broadcom NS2 SVK (DT)
[    3.900967] task: ffffffc0fb088000 ti: ffffffc0fb090000 task.ti: 
ffffffc0fb090000
[    3.909271] PC is at pci_generic_config_read32+0x74/0xa0
[    3.915190] LR is at pci_generic_config_read32+0x28/0xa0
[    3.921081] pc : [<ffffffc000374684>] lr : [<ffffffc000374638>] 
pstate: 200000c5
[    3.929309] sp : ffffffc0fb093900
[    3.932969] x29: ffffffc0fb093900 x28: ffffffc0fa9d2400
[    3.938864] x27: ffffffc07a93c090 x26: 0000000000000000
[    3.944838] x25: 0000000000000000 x24: ffffffc0fa9d2800
[    3.950776] x23: 0000000000000040 x22: ffffffc000883318
[    3.956678] x21: 0000000000000018 x20: ffffffc0fb093a0c
[    3.962589] x19: 0000000000000000 x18: 000000000000073f
[    3.968491] x17: ffffffffffffffff x16: 0000000000000011
[    3.974356] x15: 0000000000000000 x14: ffffffffffffffff
[    3.980311] x13: ffffffffffffffff x12: 0000000000000000
[    3.986258] x11: 00000000000002eb x10: 0000000000000006
[    3.992177] x9 : 00000000000002ec x8 : 3078303a74657366
[    3.998106] x7 : ffffffc000812a70 x6 : ffffffc0007d4dc4
[    4.004026] x5 : 000000000000000f x4 : ffffffc0fb09398c
[    4.009937] x3 : 0000000000000004 x2 : ffffff80000d41f8
[    4.015865] x1 : ffffff80000d4000 x0 : 0000000000000000

Therefore, we need to keep this 'hacky check' in the iProc host driver.

Thanks,

Ray
--
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
Bjorn Helgaas Jan. 29, 2016, 7:42 p.m. UTC | #2 
On Fri, Jan 29, 2016 at 09:53:08AM -0800, Ray Jui wrote:
> On 1/29/2016 9:30 AM, Bjorn Helgaas wrote:
> >It looks somewhat
> >hacky to have the PAXC-specific "slot > 0" test, and I'm not sure it
> >should be necessary (again, unless there's some implementation
> >deficiency in that PAXC embedded endpoint).  I'm looking at section
> >7.3 in the spec, and it seems like that endpoint *should* handle
> >a config transaction with a non-zero Device Number, i.e., "slot", as
> >an Unsupported Request.  This should be standard behavior for all PCIe
> >endpoints -- we can generate config transactions like that on all root
> >complexes on all systems, so all endpoints should be able to handle
> >it.
> 
> Unfortunately, it looks like the integrated endpoint connected to
> PAXC is not fully compliant to the above described behavior.
> 
> I tested by removing the "slot > 0" test in the driver and added
> some debug prints, it appears that attempted access to slot 1, 2, 3
> cannot be rejected properly and results an kernel crash.
> 
> Debugging prints are in the format of <bus>:<slot>:<func> offset:0x<where>
> 
> [    3.871332] 1:1:0 offset:0x0
> [    3.874552] 1:2:0 offset:0x0
> [    3.877759] 1:3:0 offset:0x0
> [    3.881454] Bad mode in Error handler detected, code 0xbf000002 -- SError
> [    3.888996] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0+ #117
> [    3.895801] Hardware name: Broadcom NS2 SVK (DT)
> [    3.900967] task: ffffffc0fb088000 ti: ffffffc0fb090000 task.ti:
> ffffffc0fb090000
> [    3.909271] PC is at pci_generic_config_read32+0x74/0xa0
> [    3.915190] LR is at pci_generic_config_read32+0x28/0xa0
> [    3.921081] pc : [<ffffffc000374684>] lr : [<ffffffc000374638>]
> pstate: 200000c5
> [    3.929309] sp : ffffffc0fb093900
> [    3.932969] x29: ffffffc0fb093900 x28: ffffffc0fa9d2400
> [    3.938864] x27: ffffffc07a93c090 x26: 0000000000000000
> [    3.944838] x25: 0000000000000000 x24: ffffffc0fa9d2800
> [    3.950776] x23: 0000000000000040 x22: ffffffc000883318
> [    3.956678] x21: 0000000000000018 x20: ffffffc0fb093a0c
> [    3.962589] x19: 0000000000000000 x18: 000000000000073f
> [    3.968491] x17: ffffffffffffffff x16: 0000000000000011
> [    3.974356] x15: 0000000000000000 x14: ffffffffffffffff
> [    3.980311] x13: ffffffffffffffff x12: 0000000000000000
> [    3.986258] x11: 00000000000002eb x10: 0000000000000006
> [    3.992177] x9 : 00000000000002ec x8 : 3078303a74657366
> [    3.998106] x7 : ffffffc000812a70 x6 : ffffffc0007d4dc4
> [    4.004026] x5 : 000000000000000f x4 : ffffffc0fb09398c
> [    4.009937] x3 : 0000000000000004 x2 : ffffff80000d41f8
> [    4.015865] x1 : ffffff80000d4000 x0 : 0000000000000000
> 
> Therefore, we need to keep this 'hacky check' in the iProc host driver.

OK, great, then we can finally put this one to bed.  Thanks for checking it
out!

Bjorn
--
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
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 5816bce..a576aee 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -64,7 +64,6 @@ 
 #define OARR_SIZE_CFG                BIT(OARR_SIZE_CFG_SHIFT)
 
 #define MAX_NUM_OB_WINDOWS           2
-#define MAX_NUM_PAXC_PF              4
 
 #define IPROC_PCIE_REG_INVALID 0xffff
 
@@ -170,20 +169,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 +184,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;
+
 		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 +198,14 @@  static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
 			return (pcie->base + offset);
 	}
 
+	/*
+	 * PAXC is connected to an internally emulated EP within the SoC.  It
+	 * allows only one device.
+	 */
+	if (pcie->type == IPROC_PCIE_PAXC)
+		if (slot > 0)
+			return NULL;
+
 	/* EP device access */
 	val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
 		(slot << CFG_ADDR_DEV_NUM_SHIFT) |