| Message ID | 20160129173041.GD12965@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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 --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) |