* [PATCH] PCI: iproc: Remove redundant function number check for PAXC @ 2016-01-28 23:37 Ray Jui 2016-01-29 17:30 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Ray Jui @ 2016-01-28 23:37 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Rafal Milecki, Hante Meuleman, Hauke Mehrtens, linux-kernel, bcm-kernel-feedback-list, linux-pci, Ray Jui 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 */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: iproc: Remove redundant function number check for PAXC 2016-01-28 23:37 [PATCH] PCI: iproc: Remove redundant function number check for PAXC Ray Jui @ 2016-01-29 17:30 ` Bjorn Helgaas 2016-01-29 17:53 ` Ray Jui 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2016-01-29 17:30 UTC (permalink / raw) To: Ray Jui Cc: Bjorn Helgaas, Rafal Milecki, Hante Meuleman, Hauke Mehrtens, linux-kernel, bcm-kernel-feedback-list, linux-pci 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> 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) | ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: iproc: Remove redundant function number check for PAXC 2016-01-29 17:30 ` Bjorn Helgaas @ 2016-01-29 17:53 ` Ray Jui 2016-01-29 19:42 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Ray Jui @ 2016-01-29 17:53 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Rafal Milecki, Hante Meuleman, Hauke Mehrtens, linux-kernel, bcm-kernel-feedback-list, linux-pci 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: iproc: Remove redundant function number check for PAXC 2016-01-29 17:53 ` Ray Jui @ 2016-01-29 19:42 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2016-01-29 19:42 UTC (permalink / raw) To: Ray Jui Cc: Bjorn Helgaas, Rafal Milecki, Hante Meuleman, Hauke Mehrtens, linux-kernel, bcm-kernel-feedback-list, linux-pci 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-29 19:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-28 23:37 [PATCH] PCI: iproc: Remove redundant function number check for PAXC Ray Jui 2016-01-29 17:30 ` Bjorn Helgaas 2016-01-29 17:53 ` Ray Jui 2016-01-29 19:42 ` Bjorn Helgaas
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).