| Message ID | 1453851100-15196-1-git-send-email-rjui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. 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. > 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? 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 -- 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 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 -- 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
Hi Bjorn, On 1/27/2016 3:01 PM, Ray Jui wrote: > > > 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> ... ... >>> 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.... > It turns out I do manage to find some time to test this today, :) Bjorn, you are right. The additional check for function number is indeed redundant. It doesn't cause any issue now but will limit the number of physical functions to be supported on PAXC in our next-gen SoC. I will submit another patch to remove the check. 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
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; + 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; + /* EP device access */ val = (busno << CFG_ADDR_BUS_NUM_SHIFT) | (slot << CFG_ADDR_DEV_NUM_SHIFT) |
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> --- drivers/pci/host/pcie-iproc.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)