* [PATCH v2 1/3] PCI: dwc: allow to limit registers set length
@ 2018-11-20 13:27 Stefan Agner
2018-11-20 13:27 ` [PATCH v2 2/3] PCI: imx6: introduce drvdata Stefan Agner
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Stefan Agner @ 2018-11-20 13:27 UTC (permalink / raw)
To: jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel, Stefan Agner
Add length to the struct dw_pcie and check that the accessors
dw_pcie_(rd|wr)_own_conf() do not read/write beyond that point.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/pci/controller/dwc/pcie-designware-host.c | 4 ++++
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 29a05759a294..b422538ee0bb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -29,6 +29,8 @@ static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
return pp->ops->rd_own_conf(pp, where, size, val);
pci = to_dw_pcie_from_pp(pp);
+ if (pci->dbi_length && where + size > pci->dbi_length)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
return dw_pcie_read(pci->dbi_base + where, size, val);
}
@@ -41,6 +43,8 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
return pp->ops->wr_own_conf(pp, where, size, val);
pci = to_dw_pcie_from_pp(pp);
+ if (pci->dbi_length && where + size > pci->dbi_length)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
return dw_pcie_write(pci->dbi_base + where, size, val);
}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 9f1a5e399b70..5be5f369abf2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -215,6 +215,7 @@ struct dw_pcie {
struct device *dev;
void __iomem *dbi_base;
void __iomem *dbi_base2;
+ int dbi_length;
u32 num_viewport;
u8 iatu_unroll_enabled;
struct pcie_port pp;
--
2.19.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] PCI: imx6: introduce drvdata
2018-11-20 13:27 [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Stefan Agner
@ 2018-11-20 13:27 ` Stefan Agner
2018-11-20 14:36 ` Lucas Stach
2018-11-20 13:27 ` [PATCH v2 3/3] PCI: imx6: limit DBI register length Stefan Agner
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2018-11-20 13:27 UTC (permalink / raw)
To: jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel, Stefan Agner
Introduce driver data struct. This will simplify handling of device
specific differences.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Changes in v2:
- Split drvdata introduction in a separate patch
- Use an array instead of individual struct imx6_pcie_drvdata declarations
drivers/pci/controller/dwc/pci-imx6.c | 38 +++++++++++++++++----------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 4a9a673b4777..7ac1a639fe91 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -39,6 +39,10 @@ enum imx6_pcie_variants {
IMX7D,
};
+struct imx6_pcie_drvdata {
+ enum imx6_pcie_variants variant;
+};
+
struct imx6_pcie {
struct dw_pcie *pci;
int reset_gpio;
@@ -50,7 +54,6 @@ struct imx6_pcie {
struct regmap *iomuxc_gpr;
struct reset_control *pciephy_reset;
struct reset_control *apps_reset;
- enum imx6_pcie_variants variant;
u32 tx_deemph_gen1;
u32 tx_deemph_gen2_3p5db;
u32 tx_deemph_gen2_6db;
@@ -58,6 +61,7 @@ struct imx6_pcie {
u32 tx_swing_low;
int link_gen;
struct regulator *vpcie;
+ const struct imx6_pcie_drvdata *drvdata;
};
/* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
@@ -285,7 +289,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
{
struct device *dev = imx6_pcie->pci->dev;
- switch (imx6_pcie->variant) {
+ switch (imx6_pcie->drvdata->variant) {
case IMX7D:
reset_control_assert(imx6_pcie->pciephy_reset);
reset_control_assert(imx6_pcie->apps_reset);
@@ -327,7 +331,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
struct device *dev = pci->dev;
int ret = 0;
- switch (imx6_pcie->variant) {
+ switch (imx6_pcie->drvdata->variant) {
case IMX6SX:
ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
if (ret) {
@@ -430,7 +434,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
!imx6_pcie->gpio_active_high);
}
- switch (imx6_pcie->variant) {
+ switch (imx6_pcie->drvdata->variant) {
case IMX7D:
reset_control_deassert(imx6_pcie->pciephy_reset);
imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
@@ -468,7 +472,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
{
- switch (imx6_pcie->variant) {
+ switch (imx6_pcie->drvdata->variant) {
case IMX7D:
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
@@ -560,7 +564,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
/* Start LTSSM. */
- if (imx6_pcie->variant == IMX7D)
+ if (imx6_pcie->drvdata->variant == IMX7D)
reset_control_deassert(imx6_pcie->apps_reset);
else
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
@@ -585,7 +589,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
tmp |= PORT_LOGIC_SPEED_CHANGE;
dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
- if (imx6_pcie->variant != IMX7D) {
+ if (imx6_pcie->drvdata->variant != IMX7D) {
/*
* On i.MX7, DIRECT_SPEED_CHANGE behaves differently
* from i.MX6 family when no link speed transition
@@ -703,8 +707,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
pci->ops = &dw_pcie_ops;
imx6_pcie->pci = pci;
- imx6_pcie->variant =
- (enum imx6_pcie_variants)of_device_get_match_data(dev);
+ imx6_pcie->drvdata = of_device_get_match_data(dev);
dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
@@ -748,7 +751,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
return PTR_ERR(imx6_pcie->pcie);
}
- switch (imx6_pcie->variant) {
+ switch (imx6_pcie->drvdata->variant) {
case IMX6SX:
imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
"pcie_inbound_axi");
@@ -835,11 +838,18 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
imx6_pcie_assert_core_reset(imx6_pcie);
}
+static const struct imx6_pcie_drvdata drvdata[] = {
+ [IMX6Q] = { .variant = IMX6Q },
+ [IMX6SX] = { .variant = IMX6SX },
+ [IMX6QP] = { .variant = IMX6QP },
+ [IMX7D] = { .variant = IMX7D },
+};
+
static const struct of_device_id imx6_pcie_of_match[] = {
- { .compatible = "fsl,imx6q-pcie", .data = (void *)IMX6Q, },
- { .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
- { .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
- { .compatible = "fsl,imx7d-pcie", .data = (void *)IMX7D, },
+ { .compatible = "fsl,imx6q-pcie", .data = &drvdata[IMX6Q], },
+ { .compatible = "fsl,imx6sx-pcie", .data = &drvdata[IMX6SX], },
+ { .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
+ { .compatible = "fsl,imx7d-pcie", .data = &drvdata[IMX7D], },
{},
};
--
2.19.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-20 13:27 [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Stefan Agner
2018-11-20 13:27 ` [PATCH v2 2/3] PCI: imx6: introduce drvdata Stefan Agner
@ 2018-11-20 13:27 ` Stefan Agner
2018-11-20 14:37 ` Lucas Stach
2018-11-28 0:56 ` Andrey Smirnov
2018-11-20 14:35 ` [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Lucas Stach
2018-11-20 15:14 ` Lorenzo Pieralisi
3 siblings, 2 replies; 11+ messages in thread
From: Stefan Agner @ 2018-11-20 13:27 UTC (permalink / raw)
To: jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel, Stefan Agner
Define the length of the DBI registers. This makes sure that
the kernel does not access registers beyond that point, avoiding
the following abort on a i.MX 6Quad:
# cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
[ 100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
...
[ 100.056423] PC is at dw_pcie_read+0x50/0x84
[ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
...
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
drivers/pci/controller/dwc/pci-imx6.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 7ac1a639fe91..41d6127b40ad 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -41,6 +41,7 @@ enum imx6_pcie_variants {
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
+ int dbi_length;
};
struct imx6_pcie {
@@ -779,6 +780,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
break;
}
+ pci->dbi_length = imx6_pcie->drvdata->dbi_length;
+
/* Grab GPR config register range */
imx6_pcie->iomuxc_gpr =
syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
@@ -839,7 +842,7 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
}
static const struct imx6_pcie_drvdata drvdata[] = {
- [IMX6Q] = { .variant = IMX6Q },
+ [IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
[IMX6SX] = { .variant = IMX6SX },
[IMX6QP] = { .variant = IMX6QP },
[IMX7D] = { .variant = IMX7D },
--
2.19.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] PCI: dwc: allow to limit registers set length
2018-11-20 13:27 [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Stefan Agner
2018-11-20 13:27 ` [PATCH v2 2/3] PCI: imx6: introduce drvdata Stefan Agner
2018-11-20 13:27 ` [PATCH v2 3/3] PCI: imx6: limit DBI register length Stefan Agner
@ 2018-11-20 14:35 ` Lucas Stach
2018-11-20 15:14 ` Lorenzo Pieralisi
3 siblings, 0 replies; 11+ messages in thread
From: Lucas Stach @ 2018-11-20 14:35 UTC (permalink / raw)
To: Stefan Agner, jingoohan1, gustavo.pimentel, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel
Am Dienstag, den 20.11.2018, 14:27 +0100 schrieb Stefan Agner:
> Add length to the struct dw_pcie and check that the accessors
> dw_pcie_(rd|wr)_own_conf() do not read/write beyond that point.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
FWIW:
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 4 ++++
> drivers/pci/controller/dwc/pcie-designware.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 29a05759a294..b422538ee0bb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,6 +29,8 @@ static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> > return pp->ops->rd_own_conf(pp, where, size, val);
>
> > pci = to_dw_pcie_from_pp(pp);
> > + if (pci->dbi_length && where + size > pci->dbi_length)
> > + return PCIBIOS_BAD_REGISTER_NUMBER;
> > return dw_pcie_read(pci->dbi_base + where, size, val);
> }
>
> @@ -41,6 +43,8 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
> > return pp->ops->wr_own_conf(pp, where, size, val);
>
> > pci = to_dw_pcie_from_pp(pp);
> > + if (pci->dbi_length && where + size > pci->dbi_length)
> > + return PCIBIOS_BAD_REGISTER_NUMBER;
> > return dw_pcie_write(pci->dbi_base + where, size, val);
> }
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 9f1a5e399b70..5be5f369abf2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -215,6 +215,7 @@ struct dw_pcie {
> > > struct device *dev;
> > > void __iomem *dbi_base;
> > > void __iomem *dbi_base2;
> > > + int dbi_length;
> > > u32 num_viewport;
> > > u8 iatu_unroll_enabled;
> > > struct pcie_port pp;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] PCI: imx6: introduce drvdata
2018-11-20 13:27 ` [PATCH v2 2/3] PCI: imx6: introduce drvdata Stefan Agner
@ 2018-11-20 14:36 ` Lucas Stach
0 siblings, 0 replies; 11+ messages in thread
From: Lucas Stach @ 2018-11-20 14:36 UTC (permalink / raw)
To: Stefan Agner, jingoohan1, gustavo.pimentel, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel
Am Dienstag, den 20.11.2018, 14:27 +0100 schrieb Stefan Agner:
> Introduce driver data struct. This will simplify handling of device
> specific differences.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> Changes in v2:
> - Split drvdata introduction in a separate patch
> - Use an array instead of individual struct imx6_pcie_drvdata declarations
>
> drivers/pci/controller/dwc/pci-imx6.c | 38 +++++++++++++++++----------
> 1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 4a9a673b4777..7ac1a639fe91 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -39,6 +39,10 @@ enum imx6_pcie_variants {
> > IMX7D,
> };
>
> +struct imx6_pcie_drvdata {
> > + enum imx6_pcie_variants variant;
> +};
> +
> struct imx6_pcie {
> > > struct dw_pcie *pci;
> > > int reset_gpio;
> @@ -50,7 +54,6 @@ struct imx6_pcie {
> > > struct regmap *iomuxc_gpr;
> > > struct reset_control *pciephy_reset;
> > > struct reset_control *apps_reset;
> > - enum imx6_pcie_variants variant;
> > > u32 tx_deemph_gen1;
> > > u32 tx_deemph_gen2_3p5db;
> > > u32 tx_deemph_gen2_6db;
> @@ -58,6 +61,7 @@ struct imx6_pcie {
> > > u32 tx_swing_low;
> > > int link_gen;
> > > struct regulator *vpcie;
> > + const struct imx6_pcie_drvdata *drvdata;
> };
>
> /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> @@ -285,7 +289,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> {
> > struct device *dev = imx6_pcie->pci->dev;
>
> > - switch (imx6_pcie->variant) {
> > + switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > reset_control_assert(imx6_pcie->pciephy_reset);
> > reset_control_assert(imx6_pcie->apps_reset);
> @@ -327,7 +331,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > struct device *dev = pci->dev;
> > int ret = 0;
>
> > - switch (imx6_pcie->variant) {
> > + switch (imx6_pcie->drvdata->variant) {
> > case IMX6SX:
> > ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> > if (ret) {
> @@ -430,7 +434,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > !imx6_pcie->gpio_active_high);
> > }
>
> > - switch (imx6_pcie->variant) {
> > + switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > reset_control_deassert(imx6_pcie->pciephy_reset);
> > imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> @@ -468,7 +472,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> {
> > - switch (imx6_pcie->variant) {
> > + switch (imx6_pcie->drvdata->variant) {
> > case IMX7D:
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
> @@ -560,7 +564,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
> > dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);
>
> > /* Start LTSSM. */
> > - if (imx6_pcie->variant == IMX7D)
> > + if (imx6_pcie->drvdata->variant == IMX7D)
> > reset_control_deassert(imx6_pcie->apps_reset);
> > else
> > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> @@ -585,7 +589,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
> > tmp |= PORT_LOGIC_SPEED_CHANGE;
> > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>
> > - if (imx6_pcie->variant != IMX7D) {
> > + if (imx6_pcie->drvdata->variant != IMX7D) {
> > /*
> > * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
> > * from i.MX6 family when no link speed transition
> @@ -703,8 +707,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > pci->ops = &dw_pcie_ops;
>
> > imx6_pcie->pci = pci;
> > - imx6_pcie->variant =
> > - (enum imx6_pcie_variants)of_device_get_match_data(dev);
> > + imx6_pcie->drvdata = of_device_get_match_data(dev);
>
> > dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
> @@ -748,7 +751,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > return PTR_ERR(imx6_pcie->pcie);
> > }
>
> > - switch (imx6_pcie->variant) {
> > + switch (imx6_pcie->drvdata->variant) {
> > case IMX6SX:
> > imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> > "pcie_inbound_axi");
> @@ -835,11 +838,18 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
> > imx6_pcie_assert_core_reset(imx6_pcie);
> }
>
> +static const struct imx6_pcie_drvdata drvdata[] = {
> > + [IMX6Q] = { .variant = IMX6Q },
> > + [IMX6SX] = { .variant = IMX6SX },
> > + [IMX6QP] = { .variant = IMX6QP },
> > + [IMX7D] = { .variant = IMX7D },
> +};
> +
> static const struct of_device_id imx6_pcie_of_match[] = {
> > - { .compatible = "fsl,imx6q-pcie", .data = (void *)IMX6Q, },
> > - { .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
> > - { .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
> > - { .compatible = "fsl,imx7d-pcie", .data = (void *)IMX7D, },
> > + { .compatible = "fsl,imx6q-pcie", .data = &drvdata[IMX6Q], },
> > + { .compatible = "fsl,imx6sx-pcie", .data = &drvdata[IMX6SX], },
> > + { .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
> > + { .compatible = "fsl,imx7d-pcie", .data = &drvdata[IMX7D], },
> > {},
> };
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-20 13:27 ` [PATCH v2 3/3] PCI: imx6: limit DBI register length Stefan Agner
@ 2018-11-20 14:37 ` Lucas Stach
2018-11-28 0:56 ` Andrey Smirnov
1 sibling, 0 replies; 11+ messages in thread
From: Lucas Stach @ 2018-11-20 14:37 UTC (permalink / raw)
To: Stefan Agner, jingoohan1, gustavo.pimentel, tpiepho
Cc: bhelgaas, linux-pci, linux-kernel
Am Dienstag, den 20.11.2018, 14:27 +0100 schrieb Stefan Agner:
> Define the length of the DBI registers. This makes sure that
> the kernel does not access registers beyond that point, avoiding
> the following abort on a i.MX 6Quad:
> # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> [ 100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
> ...
> [ 100.056423] PC is at dw_pcie_read+0x50/0x84
> [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> ...
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 7ac1a639fe91..41d6127b40ad 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -41,6 +41,7 @@ enum imx6_pcie_variants {
>
> struct imx6_pcie_drvdata {
> > enum imx6_pcie_variants variant;
> > > + int dbi_length;
> };
>
> struct imx6_pcie {
> @@ -779,6 +780,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > break;
> > }
>
> > + pci->dbi_length = imx6_pcie->drvdata->dbi_length;
> +
> > /* Grab GPR config register range */
> > imx6_pcie->iomuxc_gpr =
> > syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> @@ -839,7 +842,7 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
> }
>
> static const struct imx6_pcie_drvdata drvdata[] = {
> > - [IMX6Q] = { .variant = IMX6Q },
> > + [IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
> > [IMX6SX] = { .variant = IMX6SX },
> > [IMX6QP] = { .variant = IMX6QP },
> > [IMX7D] = { .variant = IMX7D },
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] PCI: dwc: allow to limit registers set length
2018-11-20 13:27 [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Stefan Agner
` (2 preceding siblings ...)
2018-11-20 14:35 ` [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Lucas Stach
@ 2018-11-20 15:14 ` Lorenzo Pieralisi
3 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-20 15:14 UTC (permalink / raw)
To: Stefan Agner
Cc: jingoohan1, gustavo.pimentel, l.stach, tpiepho, bhelgaas,
linux-pci, linux-kernel
On Tue, Nov 20, 2018 at 02:27:03PM +0100, Stefan Agner wrote:
> Add length to the struct dw_pcie and check that the accessors
> dw_pcie_(rd|wr)_own_conf() do not read/write beyond that point.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 4 ++++
> drivers/pci/controller/dwc/pcie-designware.h | 1 +
> 2 files changed, 5 insertions(+)
Hi Stefan,
may I kindly ask you please to rebase this series against my pci/dwc
branch ? I will apply it with Lucas tags then.
Please CC me on the patches.
Thanks,
Lorenzo
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 29a05759a294..b422538ee0bb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,6 +29,8 @@ static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> return pp->ops->rd_own_conf(pp, where, size, val);
>
> pci = to_dw_pcie_from_pp(pp);
> + if (pci->dbi_length && where + size > pci->dbi_length)
> + return PCIBIOS_BAD_REGISTER_NUMBER;
> return dw_pcie_read(pci->dbi_base + where, size, val);
> }
>
> @@ -41,6 +43,8 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
> return pp->ops->wr_own_conf(pp, where, size, val);
>
> pci = to_dw_pcie_from_pp(pp);
> + if (pci->dbi_length && where + size > pci->dbi_length)
> + return PCIBIOS_BAD_REGISTER_NUMBER;
> return dw_pcie_write(pci->dbi_base + where, size, val);
> }
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 9f1a5e399b70..5be5f369abf2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -215,6 +215,7 @@ struct dw_pcie {
> struct device *dev;
> void __iomem *dbi_base;
> void __iomem *dbi_base2;
> + int dbi_length;
> u32 num_viewport;
> u8 iatu_unroll_enabled;
> struct pcie_port pp;
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-20 13:27 ` [PATCH v2 3/3] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 14:37 ` Lucas Stach
@ 2018-11-28 0:56 ` Andrey Smirnov
2018-11-28 1:12 ` Fabio Estevam
1 sibling, 1 reply; 11+ messages in thread
From: Andrey Smirnov @ 2018-11-28 0:56 UTC (permalink / raw)
To: Stefan Agner
Cc: Jingoo Han, gustavo.pimentel, Lucas Stach, Trent Piepho,
Bjorn Helgaas, linux-pci, linux-kernel, Leonard Crestez
On Tue, Nov 20, 2018 at 9:43 AM Stefan Agner <stefan@agner.ch> wrote:
>
> Define the length of the DBI registers. This makes sure that
> the kernel does not access registers beyond that point, avoiding
> the following abort on a i.MX 6Quad:
> # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> [ 100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
> ...
> [ 100.056423] PC is at dw_pcie_read+0x50/0x84
> [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> ...
Could this be a regression? Prior to 415b6185c541 ("PCI: imx6: Fix
config read timeout handling") all of the imprecise aborts were caught
and handled via no-op handler. I did an experiment on i.MX6Q board
that I have (ZII RDU2) and adding a simple no-op for imprecise aborts
via
hook_fault_code(16 + 6, imx6q_pcie_no_op_handler, SIGBUS, 0,
"imprecise external abort");
seems to "resolve" this problem:
hexdump /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
0000000 16c3 abcd 0547 0010 0001 0604 0010 0001
0000010 0000 01c0 0000 0000 0100 00ff 1010 0000
0000020 0100 01b0 fff0 0000 0000 0000 0000 0000
0000030 0000 0000 0040 0000 0000 0000 012a 0001
0000040 5001 dbc3 0000 0000 0000 0000 0000 0000
0000050 7005 0181 c000 7e27 0000 0000 0000 0000
0000060 0000 0000 0000 0000 0000 0000 0000 0000
0000070 0010 0042 8000 0000 201f 0010 cc11 0013
0000080 0040 3011 0000 0000 03c0 0040 0008 0000
0000090 0000 0000 001f 0000 0000 0000 0002 0000
00000a0 0002 0001 0000 0000 0000 0000 0000 0000
00000b0 0000 0000 0000 0000 0000 0000 0000 0000
*
0000100 0001 1401 0000 0000 0000 0000 2030 0006
0000110 0000 0000 2000 0000 00a0 0000 0000 0000
0000120 0000 0000 0000 0000 0000 0000 0007 0000
0000130 0000 0000 0000 0000 0000 0000 0000 0000
0000140 0002 0001 0000 0000 0000 0000 0000 0000
0000150 0000 0000 00ff 8000 0000 0000 0000 0000
0000160 0000 0000 0000 0000 0000 0000 0000 0000
*
0000700 0076 0163 ffff ffff 0004 0700 2c00 1b2c
0000710 0120 0001 0000 0000 8000 0000 0280 0000
0000720 0000 0000 0001 0000 b611 03d5 0410 0800
0000730 4020 0000 4004 0000 ffff 000f 0000 0000
0000740 000f 0000 0000 0000 c019 0020 c003 0020
0000750 0000 0080 0000 0000 0000 0000 0000 0000
0000760 0000 0000 0000 0000 0000 0000 0000 0000
*
0000800 0000 0000 0000 0000 0000 0000 012c 0000
0000810 0000 0000 0000 0000 0302 0000 0000 0000
0000820 c000 7e27 0000 0000 0001 0000 0000 0000
0000830 0000 0000 0000 0000 0000 0000 0000 0000
*
0000900 0001 0000 0002 0000 0000 8000 0000 01f8
0000910 0000 0000 ffff 01f8 0000 0000 0000 0000
0000920 0000 0000 0000 0000 0000 0000 0000 0000
*
0001000
Maybe a simple fix would be to install such a handler when running on i.MX6Q?
Thanks,
Andrey Smirnov
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 7ac1a639fe91..41d6127b40ad 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -41,6 +41,7 @@ enum imx6_pcie_variants {
>
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> + int dbi_length;
> };
>
> struct imx6_pcie {
> @@ -779,6 +780,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> break;
> }
>
> + pci->dbi_length = imx6_pcie->drvdata->dbi_length;
> +
> /* Grab GPR config register range */
> imx6_pcie->iomuxc_gpr =
> syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> @@ -839,7 +842,7 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
> }
>
> static const struct imx6_pcie_drvdata drvdata[] = {
> - [IMX6Q] = { .variant = IMX6Q },
> + [IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
> [IMX6SX] = { .variant = IMX6SX },
> [IMX6QP] = { .variant = IMX6QP },
> [IMX7D] = { .variant = IMX7D },
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-28 0:56 ` Andrey Smirnov
@ 2018-11-28 1:12 ` Fabio Estevam
2018-11-28 1:28 ` Andrey Smirnov
0 siblings, 1 reply; 11+ messages in thread
From: Fabio Estevam @ 2018-11-28 1:12 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Stefan Agner, Jingoo Han, Gustavo Pimentel, Lucas Stach,
Trent Piepho, Bjorn Helgaas, linux-pci, linux-kernel,
Leonard Crestez
Hi Andrey,
On Tue, Nov 27, 2018 at 10:57 PM Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Could this be a regression? Prior to 415b6185c541 ("PCI: imx6: Fix
> config read timeout handling") all of the imprecise aborts were caught
> and handled via no-op handler. I did an experiment on i.MX6Q board
> that I have (ZII RDU2) and adding a simple no-op for imprecise aborts
> via
>
> hook_fault_code(16 + 6, imx6q_pcie_no_op_handler, SIGBUS, 0,
> "imprecise external abort");
>
> seems to "resolve" this problem:
Please check https://patchwork.kernel.org/patch/9720313/
This commit fixed a kernel crash on mx6q boards with a PCI switch.
So we can't go back to the simple no-op.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-28 1:12 ` Fabio Estevam
@ 2018-11-28 1:28 ` Andrey Smirnov
2018-11-28 12:45 ` Stefan Agner
0 siblings, 1 reply; 11+ messages in thread
From: Andrey Smirnov @ 2018-11-28 1:28 UTC (permalink / raw)
To: Fabio Estevam
Cc: Stefan Agner, Jingoo Han, gustavo.pimentel, Lucas Stach,
Trent Piepho, Bjorn Helgaas, linux-pci, linux-kernel,
Leonard Crestez
On Tue, Nov 27, 2018 at 5:12 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Andrey,
>
> On Tue, Nov 27, 2018 at 10:57 PM Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>
> > Could this be a regression? Prior to 415b6185c541 ("PCI: imx6: Fix
> > config read timeout handling") all of the imprecise aborts were caught
> > and handled via no-op handler. I did an experiment on i.MX6Q board
> > that I have (ZII RDU2) and adding a simple no-op for imprecise aborts
> > via
> >
> > hook_fault_code(16 + 6, imx6q_pcie_no_op_handler, SIGBUS, 0,
> > "imprecise external abort");
> >
> > seems to "resolve" this problem:
>
> Please check https://patchwork.kernel.org/patch/9720313/
>
> This commit fixed a kernel crash on mx6q boards with a PCI switch.
>
> So we can't go back to the simple no-op.
It's probably not exactly clear form my message, but I wasn't
proposing to go back to a no-op. What I had in mind is having a no-op
handler for imprecise aborts _alongside_ the non-linefetch handlers
that is already there when running against i.MX6Q type of the IP
block.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] PCI: imx6: limit DBI register length
2018-11-28 1:28 ` Andrey Smirnov
@ 2018-11-28 12:45 ` Stefan Agner
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Agner @ 2018-11-28 12:45 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Fabio Estevam, Jingoo Han, gustavo.pimentel, Lucas Stach,
Trent Piepho, Bjorn Helgaas, linux-pci, linux-kernel,
Leonard Crestez
On 28.11.2018 02:28, Andrey Smirnov wrote:
> On Tue, Nov 27, 2018 at 5:12 PM Fabio Estevam <festevam@gmail.com> wrote:
>>
>> Hi Andrey,
>>
>> On Tue, Nov 27, 2018 at 10:57 PM Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>>
>> > Could this be a regression? Prior to 415b6185c541 ("PCI: imx6: Fix
>> > config read timeout handling") all of the imprecise aborts were caught
>> > and handled via no-op handler. I did an experiment on i.MX6Q board
>> > that I have (ZII RDU2) and adding a simple no-op for imprecise aborts
>> > via
>> >
>> > hook_fault_code(16 + 6, imx6q_pcie_no_op_handler, SIGBUS, 0,
>> > "imprecise external abort");
Unsurprisingly, introducing this handler also "fixes" the issue in my
setup.
FWIW, during my investigation with the Thumb2 issue, I was looking at
the abort handler and its history a bit more closely too. I was about to
suggest readding this handler too, you just beat me by some hours :-)
The current 4.9 downstream BSP still has the old fault handler, and
hence this issue does not happen in the downstream BSP.
>> >
>> > seems to "resolve" this problem:
>>
>> Please check https://patchwork.kernel.org/patch/9720313/
>>
>> This commit fixed a kernel crash on mx6q boards with a PCI switch.
>>
>> So we can't go back to the simple no-op.
>
> It's probably not exactly clear form my message, but I wasn't
> proposing to go back to a no-op. What I had in mind is having a no-op
> handler for imprecise aborts _alongside_ the non-linefetch handlers
> that is already there when running against i.MX6Q type of the IP
> block.
>
Agreed, it should be alongside the "external abort on non-linefetch"
handler.
I actually encountered another issue when I had a Intel e1000e running
yesterday. Unfortunately I wasn't able to reproduce the issue, so maybe
it was just a fluke. It probably would be solved by the additional
"imprecise external abort" too:
[ 37.644300] fec 2188000.ethernet eth0: Link is Down
[ 38.077383] Unhandled fault: imprecise external abort (0x1406) at
0xb64e8000
[ 38.084638] pgd = ac4709d6
[ 38.087434] [b64e8000] *pgd=00000000
[ 38.091129] Internal error: : 1406 [#1] PREEMPT SMP ARM
[ 38.096508] CPU: 0 PID: 468 Comm: kworker/0:2 Not tainted
4.19.4-00044-ged7a0cc2ef01-dirty #479
[ 38.105428] Hardware name: Freescale i.MX6 Quad/DualLite (Device
Tree)
[ 38.112143] Workqueue: events e1000_watchdog_task
[ 38.116993] PC is at e1000e_update_stats+0x68/0xa7c
[ 38.122008] LR is at e1000_watchdog_task+0xe8/0x71c
[ 38.127021] pc : [<c0621238>] lr : [<c0628d0c>] psr: 60010013
[ 38.133449] sp : ed185ea0 ip : 00007374 fp : ec83ece4
[ 38.138814] r10: ec71f700 r9 : ec83c500 r8 : ec83c000
[ 38.144180] r7 : ec83c924 r6 : ec83c000 r5 : c1104cc8 r4 :
ec83c500
[ 38.150875] r3 : f14c4000 r2 : 000003e8 r1 : 00000000 r0 :
ec83c500
[ 38.157573] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM
Segment none
[ 38.164890] Control: 10c5387d Table: 3d1c004a DAC: 00000051
[ 38.170789] Process kworker/0:2 (pid: 468, stack limit = 0xbc71b316)
[ 38.177306] Stack: (0xed185ea0 to 0xed186000)
[ 38.181794] 5ea0: ef7a9100 c0619ba3 ec0870e0 ec087068 ec0870e0
00000000 60010013 c0619ba3
[ 38.190184] 5ec0: ef7a8d00 ec83c54c c1104cc8 ec83e54c ec83c924
ec83c000 ec83c500 ec71f700
[ 38.198573] 5ee0: ec83ece4 c0628d0c ecbd16c0 c0c0237c ed185f3c
c0b26de0 ec131c04 c0619ba3
[ 38.206966] 5f00: c1153fe4 ec83c54c ecdcc100 ef7a8d00 ef7a9e00
00000000 ec83c550 00000000
[ 38.221770] 5f20: ef7a8d00 c0136aec c1103d00 ef7a8d18 ecdcc100
ef7a8d00 ecdcc114 c1103d00
[ 38.236801] 5f40: ef7a8d18 ffffe000 00000008 c0136d40 ec521c70
c1176068 c0e4213c ed184000
[ 38.251874] 5f60: ecfecfdc ecfecfc0 ed0db1c0 00000000 ed184000
ecdcc100 c0136cfc ec0a3ea4
[ 38.267199] 5f80: ecfecfdc c013c810 00000000 ed0db1c0 c013c6c8
00000000 00000000 00000000
[ 38.282612] 5fa0: 00000000 00000000 00000000 c01010e8 00000000
00000000 00000000 00000000
[ 38.298080] 5fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 38.313792] 5fe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[ 38.329716] [<c0621238>] (e1000e_update_stats) from [<c0628d0c>]
(e1000_watchdog_task+0xe8/0x71c)
[ 38.346597] [<c0628d0c>] (e1000_watchdog_task) from [<c0136aec>]
(process_one_work+0x1f0/0x400)
[ 38.363493] [<c0136aec>] (process_one_work) from [<c0136d40>]
(worker_thread+0x44/0x584)
[ 38.379844] [<c0136d40>] (worker_thread) from [<c013c810>]
(kthread+0x148/0x150)
[ 38.395575] [<c013c810>] (kthread) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[ 38.411119] Exception stack(0xed185fb0 to 0xed185ff8)
[ 38.420331] 5fa0: 00000000
00000000 00000000 00000000
[ 38.436662] 5fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 38.452933] 5fe0: 00000000 00000000 00000000 00000000 00000013
00000000
[ 38.463661] Code: e590641c e2833901 e5931000 f57ff04f (e280ad9f)
--
Stefan
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-28 12:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 13:27 [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Stefan Agner
2018-11-20 13:27 ` [PATCH v2 2/3] PCI: imx6: introduce drvdata Stefan Agner
2018-11-20 14:36 ` Lucas Stach
2018-11-20 13:27 ` [PATCH v2 3/3] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 14:37 ` Lucas Stach
2018-11-28 0:56 ` Andrey Smirnov
2018-11-28 1:12 ` Fabio Estevam
2018-11-28 1:28 ` Andrey Smirnov
2018-11-28 12:45 ` Stefan Agner
2018-11-20 14:35 ` [PATCH v2 1/3] PCI: dwc: allow to limit registers set length Lucas Stach
2018-11-20 15:14 ` Lorenzo Pieralisi
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).