* [PATCH 0/3] PCI: iproc: Add fixes to pcie iproc @ 2020-03-26 7:07 ` Srinath Mannam 0 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Srinath Mannam This patch series contains fixes and improvements to pcie iproc driver. This patch set is based on Linux-5.5-rc1. Bharat Gooty (1): PCI: iproc: fix out of bound array access Roman Bacik (1): PCI: iproc: fix invalidating PAXB address mapping Srinath Mannam (1): PCI: iproc: Display PCIe Link information drivers/pci/controller/pcie-iproc.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/3] PCI: iproc: Add fixes to pcie iproc @ 2020-03-26 7:07 ` Srinath Mannam 0 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: linux-pci, Srinath Mannam, bcm-kernel-feedback-list, linux-kernel, linux-arm-kernel This patch series contains fixes and improvements to pcie iproc driver. This patch set is based on Linux-5.5-rc1. Bharat Gooty (1): PCI: iproc: fix out of bound array access Roman Bacik (1): PCI: iproc: fix invalidating PAXB address mapping Srinath Mannam (1): PCI: iproc: Display PCIe Link information drivers/pci/controller/pcie-iproc.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] PCI: iproc: fix out of bound array access 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 7:07 ` Srinath Mannam -1 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Bharat Gooty From: Bharat Gooty <bharat.gooty@broadcom.com> Declare the full size array for all revisions of PAX register sets to avoid potentially out of bound access of the register array when they are being initialized in the 'iproc_pcie_rev_init' function. Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index 0a468c7..6972ca4 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -307,7 +307,7 @@ enum iproc_pcie_reg { }; /* iProc PCIe PAXB BCMA registers */ -static const u16 iproc_pcie_reg_paxb_bcma[] = { +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { }; /* iProc PCIe PAXB registers */ -static const u16 iproc_pcie_reg_paxb[] = { +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { }; /* iProc PCIe PAXB v2 registers */ -static const u16 iproc_pcie_reg_paxb_v2[] = { +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { }; /* iProc PCIe PAXC v1 registers */ -static const u16 iproc_pcie_reg_paxc[] = { +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { }; /* iProc PCIe PAXC v2 registers */ -static const u16 iproc_pcie_reg_paxc_v2[] = { +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_MSI_GIC_MODE] = 0x050, [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, -- 2.7.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/3] PCI: iproc: fix out of bound array access @ 2020-03-26 7:07 ` Srinath Mannam 0 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: linux-pci, bcm-kernel-feedback-list, linux-kernel, linux-arm-kernel, Bharat Gooty From: Bharat Gooty <bharat.gooty@broadcom.com> Declare the full size array for all revisions of PAX register sets to avoid potentially out of bound access of the register array when they are being initialized in the 'iproc_pcie_rev_init' function. Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index 0a468c7..6972ca4 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -307,7 +307,7 @@ enum iproc_pcie_reg { }; /* iProc PCIe PAXB BCMA registers */ -static const u16 iproc_pcie_reg_paxb_bcma[] = { +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { }; /* iProc PCIe PAXB registers */ -static const u16 iproc_pcie_reg_paxb[] = { +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { }; /* iProc PCIe PAXB v2 registers */ -static const u16 iproc_pcie_reg_paxb_v2[] = { +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x120, [IPROC_PCIE_CFG_IND_DATA] = 0x124, @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { }; /* iProc PCIe PAXC v1 registers */ -static const u16 iproc_pcie_reg_paxc[] = { +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_CLK_CTRL] = 0x000, [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { }; /* iProc PCIe PAXC v2 registers */ -static const u16 iproc_pcie_reg_paxc_v2[] = { +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_MSI_GIC_MODE] = 0x050, [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 19:48 ` Bjorn Helgaas -1 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 19:48 UTC (permalink / raw) To: Srinath Mannam Cc: Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Bharat Gooty Change subject to match convention, e.g., PCI: iproc: Fix out-of-bound array accesses On Thu, Mar 26, 2020 at 12:37:25PM +0530, Srinath Mannam wrote: > From: Bharat Gooty <bharat.gooty@broadcom.com> > > Declare the full size array for all revisions of PAX register sets > to avoid potentially out of bound access of the register array > when they are being initialized in the 'iproc_pcie_rev_init' > function. s/the 'iproc_pcie_rev_init' function/iproc_pcie_rev_init()/ It's outside the scope of this patch, but I'm not really a fan of the pcie->reg_offsets[] scheme this driver uses to deal with these differences. There usually seems to be *something* that keeps the driver from referencing registers that don't exist, but it doesn't seem like the mechanism is very consistent or robust: - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. iproc_pcie_check_link() avoids using it if "ep_is_internal", which is set for PAXC and PAXC_V2. Not an obvious connection. - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", so it *doesn't* use it for PAXC_V1, which does implement it. Maybe a bug, maybe intentional; I can't tell. - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but AFAICT, we always call iproc_pcie_enable() and rely on iproc_pcie_write_reg() silently drop the write to it on PAXC. - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which is set if there's a "brcm,pcie-ob" DT property. No clear connection to PAXB. I think it would be more readable if we used a single variant identifier consistently, e.g., the "pcie->type" already used in iproc_pcie_msi_steer(), or maybe a set of variant-specific function pointers as pcie-qcom.c does. > Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") > Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 0a468c7..6972ca4 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -307,7 +307,7 @@ enum iproc_pcie_reg { > }; > > /* iProc PCIe PAXB BCMA registers */ > -static const u16 iproc_pcie_reg_paxb_bcma[] = { > +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { > }; > > /* iProc PCIe PAXB registers */ > -static const u16 iproc_pcie_reg_paxb[] = { > +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { > }; > > /* iProc PCIe PAXB v2 registers */ > -static const u16 iproc_pcie_reg_paxb_v2[] = { > +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { > }; > > /* iProc PCIe PAXC v1 registers */ > -static const u16 iproc_pcie_reg_paxc[] = { > +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, > [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, > @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { > }; > > /* iProc PCIe PAXC v2 registers */ > -static const u16 iproc_pcie_reg_paxc_v2[] = { > +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_MSI_GIC_MODE] = 0x050, > [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, > [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access @ 2020-03-26 19:48 ` Bjorn Helgaas 0 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 19:48 UTC (permalink / raw) To: Srinath Mannam Cc: Florian Fainelli, Bharat Gooty, linux-pci, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, Ray Jui, Andrew Murray, linux-arm-kernel Change subject to match convention, e.g., PCI: iproc: Fix out-of-bound array accesses On Thu, Mar 26, 2020 at 12:37:25PM +0530, Srinath Mannam wrote: > From: Bharat Gooty <bharat.gooty@broadcom.com> > > Declare the full size array for all revisions of PAX register sets > to avoid potentially out of bound access of the register array > when they are being initialized in the 'iproc_pcie_rev_init' > function. s/the 'iproc_pcie_rev_init' function/iproc_pcie_rev_init()/ It's outside the scope of this patch, but I'm not really a fan of the pcie->reg_offsets[] scheme this driver uses to deal with these differences. There usually seems to be *something* that keeps the driver from referencing registers that don't exist, but it doesn't seem like the mechanism is very consistent or robust: - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. iproc_pcie_check_link() avoids using it if "ep_is_internal", which is set for PAXC and PAXC_V2. Not an obvious connection. - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", so it *doesn't* use it for PAXC_V1, which does implement it. Maybe a bug, maybe intentional; I can't tell. - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but AFAICT, we always call iproc_pcie_enable() and rely on iproc_pcie_write_reg() silently drop the write to it on PAXC. - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which is set if there's a "brcm,pcie-ob" DT property. No clear connection to PAXB. I think it would be more readable if we used a single variant identifier consistently, e.g., the "pcie->type" already used in iproc_pcie_msi_steer(), or maybe a set of variant-specific function pointers as pcie-qcom.c does. > Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") > Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 0a468c7..6972ca4 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -307,7 +307,7 @@ enum iproc_pcie_reg { > }; > > /* iProc PCIe PAXB BCMA registers */ > -static const u16 iproc_pcie_reg_paxb_bcma[] = { > +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { > }; > > /* iProc PCIe PAXB registers */ > -static const u16 iproc_pcie_reg_paxb[] = { > +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { > }; > > /* iProc PCIe PAXB v2 registers */ > -static const u16 iproc_pcie_reg_paxb_v2[] = { > +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x120, > [IPROC_PCIE_CFG_IND_DATA] = 0x124, > @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { > }; > > /* iProc PCIe PAXC v1 registers */ > -static const u16 iproc_pcie_reg_paxc[] = { > +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_CLK_CTRL] = 0x000, > [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, > [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, > @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { > }; > > /* iProc PCIe PAXC v2 registers */ > -static const u16 iproc_pcie_reg_paxc_v2[] = { > +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_MSI_GIC_MODE] = 0x050, > [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, > [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access 2020-03-26 19:48 ` Bjorn Helgaas @ 2020-03-26 20:27 ` Ray Jui -1 siblings, 0 replies; 24+ messages in thread From: Ray Jui @ 2020-03-26 20:27 UTC (permalink / raw) To: Bjorn Helgaas, Srinath Mannam Cc: Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Bharat Gooty Hi Bjorn, On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: > Change subject to match convention, e.g., > > PCI: iproc: Fix out-of-bound array accesses > > On Thu, Mar 26, 2020 at 12:37:25PM +0530, Srinath Mannam wrote: >> From: Bharat Gooty <bharat.gooty@broadcom.com> >> >> Declare the full size array for all revisions of PAX register sets >> to avoid potentially out of bound access of the register array >> when they are being initialized in the 'iproc_pcie_rev_init' >> function. > > s/the 'iproc_pcie_rev_init' function/iproc_pcie_rev_init()/ > > It's outside the scope of this patch, but I'm not really a fan of the > pcie->reg_offsets[] scheme this driver uses to deal with these > differences. There usually seems to be *something* that keeps the > driver from referencing registers that don't exist, but it doesn't > seem like the mechanism is very consistent or robust: > > - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. > iproc_pcie_check_link() avoids using it if "ep_is_internal", which > is set for PAXC and PAXC_V2. Not an obvious connection. > > - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not > PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", > so it *doesn't* use it for PAXC_V1, which does implement it. > Maybe a bug, maybe intentional; I can't tell. > > - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but > AFAICT, we always call iproc_pcie_enable() and rely on > iproc_pcie_write_reg() silently drop the write to it on PAXC. > > - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by > iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which > is set if there's a "brcm,pcie-ob" DT property. No clear > connection to PAXB. > > I think it would be more readable if we used a single variant > identifier consistently, e.g., the "pcie->type" already used in > iproc_pcie_msi_steer(), or maybe a set of variant-specific function > pointers as pcie-qcom.c does. > It is not possible to use a single variant identifier consistently, i.e., 'pcie->type'. Many of these features are controller revision specific, and certain revisions of the controllers may all have a certain feature, while other revisions of the controllers do not. In addition, there are overlap in features across different controllers. IMO, it makes sense to have feature specific flags or booleans, and have those features enabled or disabled based on 'pcie->type', which is what the current driver does, but like you pointed out, what the driver failed is to do this consistently. The IPROC_PCIE_INTX_EN example you pointed out is a good example. I agree with you that we shouldn't rely on iproc_pcie_write_reg to silently drop the operation for PAXC. We should add code to make it explictly obvious that legacy interrupt is not supported in all PAXC controllers. pcie->pcie->reg_offsets[] scheme was not intended to be used to silently drop register access that are activated based on features. It's a mistake that should be fixed if some code in the driver is done that way, as you pointed out. The intention of reg_offsets[] is to allow many of the code in this driver be made generic, and shared between different revisions of the driver. Thanks, Ray >> Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") >> Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> >> --- >> drivers/pci/controller/pcie-iproc.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c >> index 0a468c7..6972ca4 100644 >> --- a/drivers/pci/controller/pcie-iproc.c >> +++ b/drivers/pci/controller/pcie-iproc.c >> @@ -307,7 +307,7 @@ enum iproc_pcie_reg { >> }; >> >> /* iProc PCIe PAXB BCMA registers */ >> -static const u16 iproc_pcie_reg_paxb_bcma[] = { >> +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { >> }; >> >> /* iProc PCIe PAXB registers */ >> -static const u16 iproc_pcie_reg_paxb[] = { >> +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { >> }; >> >> /* iProc PCIe PAXB v2 registers */ >> -static const u16 iproc_pcie_reg_paxb_v2[] = { >> +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { >> }; >> >> /* iProc PCIe PAXC v1 registers */ >> -static const u16 iproc_pcie_reg_paxc[] = { >> +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, >> [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, >> @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { >> }; >> >> /* iProc PCIe PAXC v2 registers */ >> -static const u16 iproc_pcie_reg_paxc_v2[] = { >> +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_MSI_GIC_MODE] = 0x050, >> [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, >> [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access @ 2020-03-26 20:27 ` Ray Jui 0 siblings, 0 replies; 24+ messages in thread From: Ray Jui @ 2020-03-26 20:27 UTC (permalink / raw) To: Bjorn Helgaas, Srinath Mannam Cc: Florian Fainelli, Bharat Gooty, linux-pci, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, Ray Jui, Andrew Murray, linux-arm-kernel Hi Bjorn, On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: > Change subject to match convention, e.g., > > PCI: iproc: Fix out-of-bound array accesses > > On Thu, Mar 26, 2020 at 12:37:25PM +0530, Srinath Mannam wrote: >> From: Bharat Gooty <bharat.gooty@broadcom.com> >> >> Declare the full size array for all revisions of PAX register sets >> to avoid potentially out of bound access of the register array >> when they are being initialized in the 'iproc_pcie_rev_init' >> function. > > s/the 'iproc_pcie_rev_init' function/iproc_pcie_rev_init()/ > > It's outside the scope of this patch, but I'm not really a fan of the > pcie->reg_offsets[] scheme this driver uses to deal with these > differences. There usually seems to be *something* that keeps the > driver from referencing registers that don't exist, but it doesn't > seem like the mechanism is very consistent or robust: > > - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. > iproc_pcie_check_link() avoids using it if "ep_is_internal", which > is set for PAXC and PAXC_V2. Not an obvious connection. > > - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not > PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", > so it *doesn't* use it for PAXC_V1, which does implement it. > Maybe a bug, maybe intentional; I can't tell. > > - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but > AFAICT, we always call iproc_pcie_enable() and rely on > iproc_pcie_write_reg() silently drop the write to it on PAXC. > > - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by > iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which > is set if there's a "brcm,pcie-ob" DT property. No clear > connection to PAXB. > > I think it would be more readable if we used a single variant > identifier consistently, e.g., the "pcie->type" already used in > iproc_pcie_msi_steer(), or maybe a set of variant-specific function > pointers as pcie-qcom.c does. > It is not possible to use a single variant identifier consistently, i.e., 'pcie->type'. Many of these features are controller revision specific, and certain revisions of the controllers may all have a certain feature, while other revisions of the controllers do not. In addition, there are overlap in features across different controllers. IMO, it makes sense to have feature specific flags or booleans, and have those features enabled or disabled based on 'pcie->type', which is what the current driver does, but like you pointed out, what the driver failed is to do this consistently. The IPROC_PCIE_INTX_EN example you pointed out is a good example. I agree with you that we shouldn't rely on iproc_pcie_write_reg to silently drop the operation for PAXC. We should add code to make it explictly obvious that legacy interrupt is not supported in all PAXC controllers. pcie->pcie->reg_offsets[] scheme was not intended to be used to silently drop register access that are activated based on features. It's a mistake that should be fixed if some code in the driver is done that way, as you pointed out. The intention of reg_offsets[] is to allow many of the code in this driver be made generic, and shared between different revisions of the driver. Thanks, Ray >> Fixes: 06324ede76cdf ("PCI: iproc: Improve core register population") >> Signed-off-by: Bharat Gooty <bharat.gooty@broadcom.com> >> --- >> drivers/pci/controller/pcie-iproc.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c >> index 0a468c7..6972ca4 100644 >> --- a/drivers/pci/controller/pcie-iproc.c >> +++ b/drivers/pci/controller/pcie-iproc.c >> @@ -307,7 +307,7 @@ enum iproc_pcie_reg { >> }; >> >> /* iProc PCIe PAXB BCMA registers */ >> -static const u16 iproc_pcie_reg_paxb_bcma[] = { >> +static const u16 iproc_pcie_reg_paxb_bcma[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -318,7 +318,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { >> }; >> >> /* iProc PCIe PAXB registers */ >> -static const u16 iproc_pcie_reg_paxb[] = { >> +static const u16 iproc_pcie_reg_paxb[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -334,7 +334,7 @@ static const u16 iproc_pcie_reg_paxb[] = { >> }; >> >> /* iProc PCIe PAXB v2 registers */ >> -static const u16 iproc_pcie_reg_paxb_v2[] = { >> +static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x120, >> [IPROC_PCIE_CFG_IND_DATA] = 0x124, >> @@ -363,7 +363,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { >> }; >> >> /* iProc PCIe PAXC v1 registers */ >> -static const u16 iproc_pcie_reg_paxc[] = { >> +static const u16 iproc_pcie_reg_paxc[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_CLK_CTRL] = 0x000, >> [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0, >> [IPROC_PCIE_CFG_IND_DATA] = 0x1f4, >> @@ -372,7 +372,7 @@ static const u16 iproc_pcie_reg_paxc[] = { >> }; >> >> /* iProc PCIe PAXC v2 registers */ >> -static const u16 iproc_pcie_reg_paxc_v2[] = { >> +static const u16 iproc_pcie_reg_paxc_v2[IPROC_PCIE_MAX_NUM_REG] = { >> [IPROC_PCIE_MSI_GIC_MODE] = 0x050, >> [IPROC_PCIE_MSI_BASE_ADDR] = 0x074, >> [IPROC_PCIE_MSI_WINDOW_SIZE] = 0x078, >> -- >> 2.7.4 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access 2020-03-26 20:27 ` Ray Jui @ 2020-03-26 20:48 ` Bjorn Helgaas -1 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 20:48 UTC (permalink / raw) To: Ray Jui Cc: Srinath Mannam, Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Bharat Gooty On Thu, Mar 26, 2020 at 01:27:36PM -0700, Ray Jui wrote: > On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: > > ... > > It's outside the scope of this patch, but I'm not really a fan of the > > pcie->reg_offsets[] scheme this driver uses to deal with these > > differences. There usually seems to be *something* that keeps the > > driver from referencing registers that don't exist, but it doesn't > > seem like the mechanism is very consistent or robust: > > > > - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. > > iproc_pcie_check_link() avoids using it if "ep_is_internal", which > > is set for PAXC and PAXC_V2. Not an obvious connection. > > > > - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not > > PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", > > so it *doesn't* use it for PAXC_V1, which does implement it. > > Maybe a bug, maybe intentional; I can't tell. > > > > - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but > > AFAICT, we always call iproc_pcie_enable() and rely on > > iproc_pcie_write_reg() silently drop the write to it on PAXC. > > > > - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by > > iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which > > is set if there's a "brcm,pcie-ob" DT property. No clear > > connection to PAXB. > > > > I think it would be more readable if we used a single variant > > identifier consistently, e.g., the "pcie->type" already used in > > iproc_pcie_msi_steer(), or maybe a set of variant-specific function > > pointers as pcie-qcom.c does. > > It is not possible to use a single variant identifier consistently, > i.e., 'pcie->type'. Many of these features are controller revision > specific, and certain revisions of the controllers may all have a > certain feature, while other revisions of the controllers do not. In > addition, there are overlap in features across different controllers. > > IMO, it makes sense to have feature specific flags or booleans, and have > those features enabled or disabled based on 'pcie->type', which is what > the current driver does, but like you pointed out, what the driver > failed is to do this consistently. There are several drivers that have the same problem of dealing with different revisions of hardware. It would be nice to do it in a consistent style, whatever that is. > The IPROC_PCIE_INTX_EN example you pointed out is a good example. I > agree with you that we shouldn't rely on iproc_pcie_write_reg to > silently drop the operation for PAXC. We should add code to make it > explictly obvious that legacy interrupt is not supported in all PAXC > controllers. > > pcie->pcie->reg_offsets[] scheme was not intended to be used to silently > drop register access that are activated based on features. It's a > mistake that should be fixed if some code in the driver is done that > way, as you pointed out. That's actually why I dug into this a bit -- the iproc_pcie_reg_is_invalid() case is really a design-time error, so it seemed like there should be a WARN() there instead of silently returning 0 or ignoring a write. Bjorn ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access @ 2020-03-26 20:48 ` Bjorn Helgaas 0 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 20:48 UTC (permalink / raw) To: Ray Jui Cc: Srinath Mannam, Florian Fainelli, Bharat Gooty, Ray Jui, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, linux-pci, Andrew Murray, linux-arm-kernel On Thu, Mar 26, 2020 at 01:27:36PM -0700, Ray Jui wrote: > On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: > > ... > > It's outside the scope of this patch, but I'm not really a fan of the > > pcie->reg_offsets[] scheme this driver uses to deal with these > > differences. There usually seems to be *something* that keeps the > > driver from referencing registers that don't exist, but it doesn't > > seem like the mechanism is very consistent or robust: > > > > - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. > > iproc_pcie_check_link() avoids using it if "ep_is_internal", which > > is set for PAXC and PAXC_V2. Not an obvious connection. > > > > - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not > > PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", > > so it *doesn't* use it for PAXC_V1, which does implement it. > > Maybe a bug, maybe intentional; I can't tell. > > > > - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but > > AFAICT, we always call iproc_pcie_enable() and rely on > > iproc_pcie_write_reg() silently drop the write to it on PAXC. > > > > - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by > > iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which > > is set if there's a "brcm,pcie-ob" DT property. No clear > > connection to PAXB. > > > > I think it would be more readable if we used a single variant > > identifier consistently, e.g., the "pcie->type" already used in > > iproc_pcie_msi_steer(), or maybe a set of variant-specific function > > pointers as pcie-qcom.c does. > > It is not possible to use a single variant identifier consistently, > i.e., 'pcie->type'. Many of these features are controller revision > specific, and certain revisions of the controllers may all have a > certain feature, while other revisions of the controllers do not. In > addition, there are overlap in features across different controllers. > > IMO, it makes sense to have feature specific flags or booleans, and have > those features enabled or disabled based on 'pcie->type', which is what > the current driver does, but like you pointed out, what the driver > failed is to do this consistently. There are several drivers that have the same problem of dealing with different revisions of hardware. It would be nice to do it in a consistent style, whatever that is. > The IPROC_PCIE_INTX_EN example you pointed out is a good example. I > agree with you that we shouldn't rely on iproc_pcie_write_reg to > silently drop the operation for PAXC. We should add code to make it > explictly obvious that legacy interrupt is not supported in all PAXC > controllers. > > pcie->pcie->reg_offsets[] scheme was not intended to be used to silently > drop register access that are activated based on features. It's a > mistake that should be fixed if some code in the driver is done that > way, as you pointed out. That's actually why I dug into this a bit -- the iproc_pcie_reg_is_invalid() case is really a design-time error, so it seemed like there should be a WARN() there instead of silently returning 0 or ignoring a write. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access 2020-03-26 20:48 ` Bjorn Helgaas @ 2020-03-30 17:04 ` Ray Jui -1 siblings, 0 replies; 24+ messages in thread From: Ray Jui @ 2020-03-30 17:04 UTC (permalink / raw) To: Bjorn Helgaas Cc: Srinath Mannam, Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Bharat Gooty On 3/26/2020 1:48 PM, Bjorn Helgaas wrote: > On Thu, Mar 26, 2020 at 01:27:36PM -0700, Ray Jui wrote: >> On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: >>> ... >>> It's outside the scope of this patch, but I'm not really a fan of the >>> pcie->reg_offsets[] scheme this driver uses to deal with these >>> differences. There usually seems to be *something* that keeps the >>> driver from referencing registers that don't exist, but it doesn't >>> seem like the mechanism is very consistent or robust: >>> >>> - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. >>> iproc_pcie_check_link() avoids using it if "ep_is_internal", which >>> is set for PAXC and PAXC_V2. Not an obvious connection. >>> >>> - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not >>> PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", >>> so it *doesn't* use it for PAXC_V1, which does implement it. >>> Maybe a bug, maybe intentional; I can't tell. >>> >>> - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but >>> AFAICT, we always call iproc_pcie_enable() and rely on >>> iproc_pcie_write_reg() silently drop the write to it on PAXC. >>> >>> - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by >>> iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which >>> is set if there's a "brcm,pcie-ob" DT property. No clear >>> connection to PAXB. >>> >>> I think it would be more readable if we used a single variant >>> identifier consistently, e.g., the "pcie->type" already used in >>> iproc_pcie_msi_steer(), or maybe a set of variant-specific function >>> pointers as pcie-qcom.c does. >> >> It is not possible to use a single variant identifier consistently, >> i.e., 'pcie->type'. Many of these features are controller revision >> specific, and certain revisions of the controllers may all have a >> certain feature, while other revisions of the controllers do not. In >> addition, there are overlap in features across different controllers. >> >> IMO, it makes sense to have feature specific flags or booleans, and have >> those features enabled or disabled based on 'pcie->type', which is what >> the current driver does, but like you pointed out, what the driver >> failed is to do this consistently. > > There are several drivers that have the same problem of dealing with > different revisions of hardware. It would be nice to do it in a > consistent style, whatever that is. > Sure, agree with you that it should be handled in a consistent way within this driver, and the current driver is not handling this consistently. >> The IPROC_PCIE_INTX_EN example you pointed out is a good example. I >> agree with you that we shouldn't rely on iproc_pcie_write_reg to >> silently drop the operation for PAXC. We should add code to make it >> explictly obvious that legacy interrupt is not supported in all PAXC >> controllers. >> >> pcie->pcie->reg_offsets[] scheme was not intended to be used to silently >> drop register access that are activated based on features. It's a >> mistake that should be fixed if some code in the driver is done that >> way, as you pointed out. > > That's actually why I dug into this a bit -- the > iproc_pcie_reg_is_invalid() case is really a design-time error, so it > seemed like there should be a WARN() there instead of silently > returning 0 or ignoring a write. > I think 'iproc_pcie_reg_is_invalid' is a fall back protection. We should aim to prevent this from happening in the first place using whatever means we determined appropriate, and do that consistently. In addition, I also agree with you that there should be a WARN instead of silently returning zero (for reads) and dropping the writes. We'll be looking into improving this as you suggested when we have a chance. In the mean time, I think both of us agree this is out of the scope of the issue that this patch is trying to fix, which is actually a pretty critical issue that can cause potential corruption of memory and the fix should be picked up ASAP (and for older LTS kernels too). Thanks, Ray > Bjorn > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] PCI: iproc: fix out of bound array access @ 2020-03-30 17:04 ` Ray Jui 0 siblings, 0 replies; 24+ messages in thread From: Ray Jui @ 2020-03-30 17:04 UTC (permalink / raw) To: Bjorn Helgaas Cc: Srinath Mannam, Florian Fainelli, Bharat Gooty, Ray Jui, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, linux-pci, Andrew Murray, linux-arm-kernel On 3/26/2020 1:48 PM, Bjorn Helgaas wrote: > On Thu, Mar 26, 2020 at 01:27:36PM -0700, Ray Jui wrote: >> On 3/26/2020 12:48 PM, Bjorn Helgaas wrote: >>> ... >>> It's outside the scope of this patch, but I'm not really a fan of the >>> pcie->reg_offsets[] scheme this driver uses to deal with these >>> differences. There usually seems to be *something* that keeps the >>> driver from referencing registers that don't exist, but it doesn't >>> seem like the mechanism is very consistent or robust: >>> >>> - IPROC_PCIE_LINK_STATUS is implemented by PAXB but not PAXC. >>> iproc_pcie_check_link() avoids using it if "ep_is_internal", which >>> is set for PAXC and PAXC_V2. Not an obvious connection. >>> >>> - IPROC_PCIE_CLK_CTRL is implemented for PAXB and PAXC_V1, but not >>> PAXC_V2. iproc_pcie_perst_ctrl() avoids using it ep_is_internal", >>> so it *doesn't* use it for PAXC_V1, which does implement it. >>> Maybe a bug, maybe intentional; I can't tell. >>> >>> - IPROC_PCIE_INTX_EN is only implemented by PAXB (not PAXC), but >>> AFAICT, we always call iproc_pcie_enable() and rely on >>> iproc_pcie_write_reg() silently drop the write to it on PAXC. >>> >>> - IPROC_PCIE_OARR0 is implemented by PAXB and PAXB_V2 and used by >>> iproc_pcie_map_ranges(), which is called if "need_ob_cfg", which >>> is set if there's a "brcm,pcie-ob" DT property. No clear >>> connection to PAXB. >>> >>> I think it would be more readable if we used a single variant >>> identifier consistently, e.g., the "pcie->type" already used in >>> iproc_pcie_msi_steer(), or maybe a set of variant-specific function >>> pointers as pcie-qcom.c does. >> >> It is not possible to use a single variant identifier consistently, >> i.e., 'pcie->type'. Many of these features are controller revision >> specific, and certain revisions of the controllers may all have a >> certain feature, while other revisions of the controllers do not. In >> addition, there are overlap in features across different controllers. >> >> IMO, it makes sense to have feature specific flags or booleans, and have >> those features enabled or disabled based on 'pcie->type', which is what >> the current driver does, but like you pointed out, what the driver >> failed is to do this consistently. > > There are several drivers that have the same problem of dealing with > different revisions of hardware. It would be nice to do it in a > consistent style, whatever that is. > Sure, agree with you that it should be handled in a consistent way within this driver, and the current driver is not handling this consistently. >> The IPROC_PCIE_INTX_EN example you pointed out is a good example. I >> agree with you that we shouldn't rely on iproc_pcie_write_reg to >> silently drop the operation for PAXC. We should add code to make it >> explictly obvious that legacy interrupt is not supported in all PAXC >> controllers. >> >> pcie->pcie->reg_offsets[] scheme was not intended to be used to silently >> drop register access that are activated based on features. It's a >> mistake that should be fixed if some code in the driver is done that >> way, as you pointed out. > > That's actually why I dug into this a bit -- the > iproc_pcie_reg_is_invalid() case is really a design-time error, so it > seemed like there should be a WARN() there instead of silently > returning 0 or ignoring a write. > I think 'iproc_pcie_reg_is_invalid' is a fall back protection. We should aim to prevent this from happening in the first place using whatever means we determined appropriate, and do that consistently. In addition, I also agree with you that there should be a WARN instead of silently returning zero (for reads) and dropping the writes. We'll be looking into improving this as you suggested when we have a chance. In the mean time, I think both of us agree this is out of the scope of the issue that this patch is trying to fix, which is actually a pretty critical issue that can cause potential corruption of memory and the fix should be picked up ASAP (and for older LTS kernels too). Thanks, Ray > Bjorn > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 7:07 ` Srinath Mannam -1 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Roman Bacik From: Roman Bacik <roman.bacik@broadcom.com> Second stage bootloader prior to Linux boot may use all inbound windows including IARR1/IMAP1. We need to ensure all previous configuration of inbound windows are invalidated during the initialization stage of the Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was missed in previous patch. Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index 6972ca4..e7f0d58 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_OMAP3] = 0xdf8, [IPROC_PCIE_IARR0] = 0xd00, [IPROC_PCIE_IMAP0] = 0xc00, + [IPROC_PCIE_IARR1] = 0xd08, + [IPROC_PCIE_IMAP1] = 0xd70, [IPROC_PCIE_IARR2] = 0xd10, [IPROC_PCIE_IMAP2] = 0xcc0, [IPROC_PCIE_IARR3] = 0xe00, -- 2.7.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping @ 2020-03-26 7:07 ` Srinath Mannam 0 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: linux-pci, Roman Bacik, bcm-kernel-feedback-list, linux-kernel, linux-arm-kernel From: Roman Bacik <roman.bacik@broadcom.com> Second stage bootloader prior to Linux boot may use all inbound windows including IARR1/IMAP1. We need to ensure all previous configuration of inbound windows are invalidated during the initialization stage of the Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was missed in previous patch. Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index 6972ca4..e7f0d58 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { [IPROC_PCIE_OMAP3] = 0xdf8, [IPROC_PCIE_IARR0] = 0xd00, [IPROC_PCIE_IMAP0] = 0xc00, + [IPROC_PCIE_IARR1] = 0xd08, + [IPROC_PCIE_IMAP1] = 0xd70, [IPROC_PCIE_IARR2] = 0xd10, [IPROC_PCIE_IMAP2] = 0xcc0, [IPROC_PCIE_IARR3] = 0xe00, -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 15:28 ` Bjorn Helgaas -1 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:28 UTC (permalink / raw) To: Srinath Mannam Cc: Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Roman Bacik [fixed Andrew's email addr] Change subject to match convention, e.g., PCI: iproc: Invalidate correct PAXB inbound windows On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > From: Roman Bacik <roman.bacik@broadcom.com> > > Second stage bootloader prior to Linux boot may use all inbound windows > including IARR1/IMAP1. We need to ensure all previous configuration of > inbound windows are invalidated during the initialization stage of the > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > missed in previous patch. > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 6972ca4..e7f0d58 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_OMAP3] = 0xdf8, > [IPROC_PCIE_IARR0] = 0xd00, > [IPROC_PCIE_IMAP0] = 0xc00, > + [IPROC_PCIE_IARR1] = 0xd08, > + [IPROC_PCIE_IMAP1] = 0xd70, Wow, this is a little too subtle for my taste. 9415743e4c8a added this loop: + for (idx = 0; idx < ib->nr_regions; idx++) { + iproc_pcie_write_reg(pcie, + MAP_REG(IPROC_PCIE_IARR0, idx), 0); This patch doesn't change the loop or the limit, so I *guess* previously we invalidated IPROC_PCIE_IARR0, IPROC_PCIE_IARR2, ..., (skipping IPROC_PCIE_IARR1), and now we will invalidate IPROC_PCIE_IARR0, IPROC_PCIE_IARR1, ... instead? > [IPROC_PCIE_IARR2] = 0xd10, > [IPROC_PCIE_IMAP2] = 0xcc0, > [IPROC_PCIE_IARR3] = 0xe00, ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping @ 2020-03-26 15:28 ` Bjorn Helgaas 0 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:28 UTC (permalink / raw) To: Srinath Mannam Cc: Florian Fainelli, Roman Bacik, linux-pci, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, Ray Jui, linux-arm-kernel, Andrew Murray [fixed Andrew's email addr] Change subject to match convention, e.g., PCI: iproc: Invalidate correct PAXB inbound windows On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > From: Roman Bacik <roman.bacik@broadcom.com> > > Second stage bootloader prior to Linux boot may use all inbound windows > including IARR1/IMAP1. We need to ensure all previous configuration of > inbound windows are invalidated during the initialization stage of the > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > missed in previous patch. > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 6972ca4..e7f0d58 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_OMAP3] = 0xdf8, > [IPROC_PCIE_IARR0] = 0xd00, > [IPROC_PCIE_IMAP0] = 0xc00, > + [IPROC_PCIE_IARR1] = 0xd08, > + [IPROC_PCIE_IMAP1] = 0xd70, Wow, this is a little too subtle for my taste. 9415743e4c8a added this loop: + for (idx = 0; idx < ib->nr_regions; idx++) { + iproc_pcie_write_reg(pcie, + MAP_REG(IPROC_PCIE_IARR0, idx), 0); This patch doesn't change the loop or the limit, so I *guess* previously we invalidated IPROC_PCIE_IARR0, IPROC_PCIE_IARR2, ..., (skipping IPROC_PCIE_IARR1), and now we will invalidate IPROC_PCIE_IARR0, IPROC_PCIE_IARR1, ... instead? > [IPROC_PCIE_IARR2] = 0xd10, > [IPROC_PCIE_IMAP2] = 0xcc0, > [IPROC_PCIE_IARR3] = 0xe00, _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 15:33 ` Bjorn Helgaas -1 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:33 UTC (permalink / raw) To: Srinath Mannam Cc: Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Roman Bacik On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > From: Roman Bacik <roman.bacik@broadcom.com> > > Second stage bootloader prior to Linux boot may use all inbound windows > including IARR1/IMAP1. We need to ensure all previous configuration of > inbound windows are invalidated during the initialization stage of the > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > missed in previous patch. > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 6972ca4..e7f0d58 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_OMAP3] = 0xdf8, > [IPROC_PCIE_IARR0] = 0xd00, > [IPROC_PCIE_IMAP0] = 0xc00, > + [IPROC_PCIE_IARR1] = 0xd08, > + [IPROC_PCIE_IMAP1] = 0xd70, And paxb_v2_ib_map[] has a comment saying "IARR1/IMAP1 (currently unused)". Is that comment now wrong? > [IPROC_PCIE_IARR2] = 0xd10, > [IPROC_PCIE_IMAP2] = 0xcc0, > [IPROC_PCIE_IARR3] = 0xe00, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping @ 2020-03-26 15:33 ` Bjorn Helgaas 0 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:33 UTC (permalink / raw) To: Srinath Mannam Cc: Florian Fainelli, Roman Bacik, linux-pci, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, Ray Jui, Andrew Murray, linux-arm-kernel On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > From: Roman Bacik <roman.bacik@broadcom.com> > > Second stage bootloader prior to Linux boot may use all inbound windows > including IARR1/IMAP1. We need to ensure all previous configuration of > inbound windows are invalidated during the initialization stage of the > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > missed in previous patch. > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 6972ca4..e7f0d58 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > [IPROC_PCIE_OMAP3] = 0xdf8, > [IPROC_PCIE_IARR0] = 0xd00, > [IPROC_PCIE_IMAP0] = 0xc00, > + [IPROC_PCIE_IARR1] = 0xd08, > + [IPROC_PCIE_IMAP1] = 0xd70, And paxb_v2_ib_map[] has a comment saying "IARR1/IMAP1 (currently unused)". Is that comment now wrong? > [IPROC_PCIE_IARR2] = 0xd10, > [IPROC_PCIE_IMAP2] = 0xcc0, > [IPROC_PCIE_IARR3] = 0xe00, > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping 2020-03-26 15:33 ` Bjorn Helgaas @ 2020-03-26 15:38 ` Roman Bacik -1 siblings, 0 replies; 24+ messages in thread From: Roman Bacik @ 2020-03-26 15:38 UTC (permalink / raw) To: Bjorn Helgaas Cc: Srinath Mannam, Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel On Thu, Mar 26, 2020 at 8:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > > From: Roman Bacik <roman.bacik@broadcom.com> > > > > Second stage bootloader prior to Linux boot may use all inbound windows > > including IARR1/IMAP1. We need to ensure all previous configuration of > > inbound windows are invalidated during the initialization stage of the > > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > > missed in previous patch. > > > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > > --- > > drivers/pci/controller/pcie-iproc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > > index 6972ca4..e7f0d58 100644 > > --- a/drivers/pci/controller/pcie-iproc.c > > +++ b/drivers/pci/controller/pcie-iproc.c > > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > > [IPROC_PCIE_OMAP3] = 0xdf8, > > [IPROC_PCIE_IARR0] = 0xd00, > > [IPROC_PCIE_IMAP0] = 0xc00, > > + [IPROC_PCIE_IARR1] = 0xd08, > > + [IPROC_PCIE_IMAP1] = 0xd70, > > And paxb_v2_ib_map[] has a comment saying "IARR1/IMAP1 (currently > unused)". Is that comment now wrong? > The comment is still correct, IARR1/IMAP1 is unused in Linux. But it may need to be invalidated in case it was modified by bootloaders. > > [IPROC_PCIE_IARR2] = 0xd10, > > [IPROC_PCIE_IMAP2] = 0xcc0, > > [IPROC_PCIE_IARR3] = 0xe00, > > -- > > 2.7.4 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping @ 2020-03-26 15:38 ` Roman Bacik 0 siblings, 0 replies; 24+ messages in thread From: Roman Bacik @ 2020-03-26 15:38 UTC (permalink / raw) To: Bjorn Helgaas Cc: Srinath Mannam, Florian Fainelli, Ray Jui, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, linux-pci, Andrew Murray, linux-arm-kernel On Thu, Mar 26, 2020 at 8:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Mar 26, 2020 at 12:37:26PM +0530, Srinath Mannam wrote: > > From: Roman Bacik <roman.bacik@broadcom.com> > > > > Second stage bootloader prior to Linux boot may use all inbound windows > > including IARR1/IMAP1. We need to ensure all previous configuration of > > inbound windows are invalidated during the initialization stage of the > > Linux iProc PCIe driver. Add fix to invalidate IARR1/IMAP1 because it was > > missed in previous patch. > > > > Fixes: 9415743e4c8a ("PCI: iproc: Invalidate PAXB address mapping") > > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com> > > --- > > drivers/pci/controller/pcie-iproc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > > index 6972ca4..e7f0d58 100644 > > --- a/drivers/pci/controller/pcie-iproc.c > > +++ b/drivers/pci/controller/pcie-iproc.c > > @@ -351,6 +351,8 @@ static const u16 iproc_pcie_reg_paxb_v2[IPROC_PCIE_MAX_NUM_REG] = { > > [IPROC_PCIE_OMAP3] = 0xdf8, > > [IPROC_PCIE_IARR0] = 0xd00, > > [IPROC_PCIE_IMAP0] = 0xc00, > > + [IPROC_PCIE_IARR1] = 0xd08, > > + [IPROC_PCIE_IMAP1] = 0xd70, > > And paxb_v2_ib_map[] has a comment saying "IARR1/IMAP1 (currently > unused)". Is that comment now wrong? > The comment is still correct, IARR1/IMAP1 is unused in Linux. But it may need to be invalidated in case it was modified by bootloaders. > > [IPROC_PCIE_IARR2] = 0xd10, > > [IPROC_PCIE_IMAP2] = 0xcc0, > > [IPROC_PCIE_IARR3] = 0xe00, > > -- > > 2.7.4 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] PCI: iproc: Display PCIe Link information 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 7:07 ` Srinath Mannam -1 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel, Srinath Mannam Add more comprehensive information to show PCIe link speed and link width to the console. Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index e7f0d58..ed41357 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -823,6 +823,8 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) #define PCI_TARGET_LINK_SPEED_MASK 0xf #define PCI_TARGET_LINK_SPEED_GEN2 0x2 #define PCI_TARGET_LINK_SPEED_GEN1 0x1 +#define PCI_TARGET_LINK_WIDTH_MASK 0x3f +#define PCI_TARGET_LINK_WIDTH_OFFSET 0x4 iproc_pci_raw_config_read32(pcie, 0, IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2, 4, &link_ctrl); @@ -843,7 +845,14 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) } } - dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN"); + if (link_is_active) { + dev_info(dev, "link UP @ Speed Gen-%d and width-x%d\n", + link_status & PCI_TARGET_LINK_SPEED_MASK, + (link_status >> PCI_TARGET_LINK_WIDTH_OFFSET) & + PCI_TARGET_LINK_WIDTH_MASK); + } else { + dev_info(dev, "link DOWN\n"); + } return link_is_active ? 0 : -ENODEV; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/3] PCI: iproc: Display PCIe Link information @ 2020-03-26 7:07 ` Srinath Mannam 0 siblings, 0 replies; 24+ messages in thread From: Srinath Mannam @ 2020-03-26 7:07 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas, Florian Fainelli, Ray Jui, Andrew Murray Cc: linux-pci, Srinath Mannam, bcm-kernel-feedback-list, linux-kernel, linux-arm-kernel Add more comprehensive information to show PCIe link speed and link width to the console. Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com> --- drivers/pci/controller/pcie-iproc.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c index e7f0d58..ed41357 100644 --- a/drivers/pci/controller/pcie-iproc.c +++ b/drivers/pci/controller/pcie-iproc.c @@ -823,6 +823,8 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) #define PCI_TARGET_LINK_SPEED_MASK 0xf #define PCI_TARGET_LINK_SPEED_GEN2 0x2 #define PCI_TARGET_LINK_SPEED_GEN1 0x1 +#define PCI_TARGET_LINK_WIDTH_MASK 0x3f +#define PCI_TARGET_LINK_WIDTH_OFFSET 0x4 iproc_pci_raw_config_read32(pcie, 0, IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2, 4, &link_ctrl); @@ -843,7 +845,14 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) } } - dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN"); + if (link_is_active) { + dev_info(dev, "link UP @ Speed Gen-%d and width-x%d\n", + link_status & PCI_TARGET_LINK_SPEED_MASK, + (link_status >> PCI_TARGET_LINK_WIDTH_OFFSET) & + PCI_TARGET_LINK_WIDTH_MASK); + } else { + dev_info(dev, "link DOWN\n"); + } return link_is_active ? 0 : -ENODEV; } -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] PCI: iproc: Display PCIe Link information 2020-03-26 7:07 ` Srinath Mannam @ 2020-03-26 15:12 ` Bjorn Helgaas -1 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:12 UTC (permalink / raw) To: Srinath Mannam Cc: Lorenzo Pieralisi, Florian Fainelli, Ray Jui, Andrew Murray, bcm-kernel-feedback-list, linux-pci, linux-arm-kernel, linux-kernel On Thu, Mar 26, 2020 at 12:37:27PM +0530, Srinath Mannam wrote: > Add more comprehensive information to show PCIe link speed and link > width to the console. > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index e7f0d58..ed41357 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -823,6 +823,8 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) > #define PCI_TARGET_LINK_SPEED_MASK 0xf > #define PCI_TARGET_LINK_SPEED_GEN2 0x2 > #define PCI_TARGET_LINK_SPEED_GEN1 0x1 > +#define PCI_TARGET_LINK_WIDTH_MASK 0x3f > +#define PCI_TARGET_LINK_WIDTH_OFFSET 0x4 > iproc_pci_raw_config_read32(pcie, 0, > IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2, > 4, &link_ctrl); > @@ -843,7 +845,14 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) > } > } > > - dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN"); > + if (link_is_active) { > + dev_info(dev, "link UP @ Speed Gen-%d and width-x%d\n", > + link_status & PCI_TARGET_LINK_SPEED_MASK, > + (link_status >> PCI_TARGET_LINK_WIDTH_OFFSET) & > + PCI_TARGET_LINK_WIDTH_MASK); Can you use pcie_print_link_status() or some variant here instead of rolling your own? > + } else { > + dev_info(dev, "link DOWN\n"); > + } > > return link_is_active ? 0 : -ENODEV; > } > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] PCI: iproc: Display PCIe Link information @ 2020-03-26 15:12 ` Bjorn Helgaas 0 siblings, 0 replies; 24+ messages in thread From: Bjorn Helgaas @ 2020-03-26 15:12 UTC (permalink / raw) To: Srinath Mannam Cc: Florian Fainelli, linux-pci, linux-kernel, Lorenzo Pieralisi, bcm-kernel-feedback-list, Ray Jui, Andrew Murray, linux-arm-kernel On Thu, Mar 26, 2020 at 12:37:27PM +0530, Srinath Mannam wrote: > Add more comprehensive information to show PCIe link speed and link > width to the console. > > Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com> > --- > drivers/pci/controller/pcie-iproc.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index e7f0d58..ed41357 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -823,6 +823,8 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) > #define PCI_TARGET_LINK_SPEED_MASK 0xf > #define PCI_TARGET_LINK_SPEED_GEN2 0x2 > #define PCI_TARGET_LINK_SPEED_GEN1 0x1 > +#define PCI_TARGET_LINK_WIDTH_MASK 0x3f > +#define PCI_TARGET_LINK_WIDTH_OFFSET 0x4 > iproc_pci_raw_config_read32(pcie, 0, > IPROC_PCI_EXP_CAP + PCI_EXP_LNKCTL2, > 4, &link_ctrl); > @@ -843,7 +845,14 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) > } > } > > - dev_info(dev, "link: %s\n", link_is_active ? "UP" : "DOWN"); > + if (link_is_active) { > + dev_info(dev, "link UP @ Speed Gen-%d and width-x%d\n", > + link_status & PCI_TARGET_LINK_SPEED_MASK, > + (link_status >> PCI_TARGET_LINK_WIDTH_OFFSET) & > + PCI_TARGET_LINK_WIDTH_MASK); Can you use pcie_print_link_status() or some variant here instead of rolling your own? > + } else { > + dev_info(dev, "link DOWN\n"); > + } > > return link_is_active ? 0 : -ENODEV; > } > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-03-30 17:04 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-26 7:07 [PATCH 0/3] PCI: iproc: Add fixes to pcie iproc Srinath Mannam 2020-03-26 7:07 ` Srinath Mannam 2020-03-26 7:07 ` [PATCH 1/3] PCI: iproc: fix out of bound array access Srinath Mannam 2020-03-26 7:07 ` Srinath Mannam 2020-03-26 19:48 ` Bjorn Helgaas 2020-03-26 19:48 ` Bjorn Helgaas 2020-03-26 20:27 ` Ray Jui 2020-03-26 20:27 ` Ray Jui 2020-03-26 20:48 ` Bjorn Helgaas 2020-03-26 20:48 ` Bjorn Helgaas 2020-03-30 17:04 ` Ray Jui 2020-03-30 17:04 ` Ray Jui 2020-03-26 7:07 ` [PATCH 2/3] PCI: iproc: fix invalidating PAXB address mapping Srinath Mannam 2020-03-26 7:07 ` Srinath Mannam 2020-03-26 15:28 ` Bjorn Helgaas 2020-03-26 15:28 ` Bjorn Helgaas 2020-03-26 15:33 ` Bjorn Helgaas 2020-03-26 15:33 ` Bjorn Helgaas 2020-03-26 15:38 ` Roman Bacik 2020-03-26 15:38 ` Roman Bacik 2020-03-26 7:07 ` [PATCH 3/3] PCI: iproc: Display PCIe Link information Srinath Mannam 2020-03-26 7:07 ` Srinath Mannam 2020-03-26 15:12 ` Bjorn Helgaas 2020-03-26 15:12 ` Bjorn Helgaas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.