* [PATCH 1/2] PCI: dwc: allow to limit registers set length
@ 2018-11-19 9:41 Stefan Agner
2018-11-19 9:41 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Agner @ 2018-11-19 9:41 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] 13+ messages in thread* [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-19 9:41 [PATCH 1/2] PCI: dwc: allow to limit registers set length Stefan Agner
@ 2018-11-19 9:41 ` Stefan Agner
2018-11-20 1:06 ` Trent Piepho
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Stefan Agner @ 2018-11-19 9:41 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 | 51 +++++++++++++++++++--------
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 4a9a673b4777..8c96af414dac 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -39,6 +39,11 @@ enum imx6_pcie_variants {
IMX7D,
};
+struct imx6_pcie_drvdata {
+ enum imx6_pcie_variants variant;
+ int dbi_length;
+};
+
struct imx6_pcie {
struct dw_pcie *pci;
int reset_gpio;
@@ -50,7 +55,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 +62,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 +290,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 +332,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 +435,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 +473,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 +565,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 +590,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 +708,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 +752,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");
@@ -776,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");
@@ -835,11 +841,28 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
imx6_pcie_assert_core_reset(imx6_pcie);
}
+static const struct imx6_pcie_drvdata imx6q_pcie_drvdata = {
+ .variant = IMX6Q,
+ .dbi_length = 0x15c,
+};
+
+static const struct imx6_pcie_drvdata imx6sx_pcie_drvdata = {
+ .variant = IMX6SX,
+};
+
+static const struct imx6_pcie_drvdata imx6qp_pcie_drvdata = {
+ .variant = IMX6QP,
+};
+
+static const struct imx6_pcie_drvdata imx7d_pcie_drvdata = {
+ .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 = &imx6q_pcie_drvdata, },
+ { .compatible = "fsl,imx6sx-pcie", .data = &imx6sx_pcie_drvdata, },
+ { .compatible = "fsl,imx6qp-pcie", .data = &imx6qp_pcie_drvdata, },
+ { .compatible = "fsl,imx7d-pcie", .data = &imx7d_pcie_drvdata, },
{},
};
--
2.19.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-19 9:41 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
@ 2018-11-20 1:06 ` Trent Piepho
2018-11-20 1:33 ` Trent Piepho
2018-11-20 10:22 ` Leonard Crestez
2 siblings, 0 replies; 13+ messages in thread
From: Trent Piepho @ 2018-11-20 1:06 UTC (permalink / raw)
To: stefan, jingoohan1, l.stach, gustavo.pimentel
Cc: linux-kernel, linux-pci, bhelgaas
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner 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
> ...
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
After updating this for v4.20-rc2, tested on IMX 7d, config space
access works as before.
Tested-by: Trent Piepho <tpiepho@impinj.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-19 9:41 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 1:06 ` Trent Piepho
@ 2018-11-20 1:33 ` Trent Piepho
2018-11-20 10:22 ` Leonard Crestez
2 siblings, 0 replies; 13+ messages in thread
From: Trent Piepho @ 2018-11-20 1:33 UTC (permalink / raw)
To: stefan, jingoohan1, l.stach, gustavo.pimentel
Cc: linux-kernel, linux-pci, bhelgaas
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner wrote:
>
>
> +static const struct imx6_pcie_drvdata imx6q_pcie_drvdata = {
> + .variant = IMX6Q,
> + .dbi_length = 0x15c,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6sx_pcie_drvdata = {
> + .variant = IMX6SX,
> +};
> +
> +static const struct imx6_pcie_drvdata imx6qp_pcie_drvdata = {
> + .variant = IMX6QP,
> +};
> +
> +static const struct imx6_pcie_drvdata imx7d_pcie_drvdata = {
> + .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 = &imx6q_pcie_drvdata, },
> + { .compatible = "fsl,imx6sx-pcie", .data = &imx6sx_pcie_drvdata, },
> + { .compatible = "fsl,imx6qp-pcie", .data = &imx6qp_pcie_drvdata, },
> + { .compatible = "fsl,imx7d-pcie", .data = &imx7d_pcie_drvdata, },
> {},
> };
Instead of making a single drvdata struct for each type, this could use
an array:
static const struct imx6_pcie_drvdata drvdata[] = {
[IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
[IMX6SX] = { .variant = IMX6SX },
[...]
};
static const struct of_device_id imx6_pcie_of_match[] = {
{ .compatible = "fsl,imx6q-pcie", .data = &drvdata[IMX6Q], },
{ .compatible = "fsl,imx6sx-pcie", .data = &drvdata[IMX6SX], },
...
};
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-19 9:41 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 1:06 ` Trent Piepho
2018-11-20 1:33 ` Trent Piepho
@ 2018-11-20 10:22 ` Leonard Crestez
2018-11-20 10:30 ` Stefan Agner
2 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2018-11-20 10:22 UTC (permalink / raw)
To: stefan
Cc: Richard Zhu, linux-kernel, jingoohan1, gustavo.pimentel, tpiepho,
andrew.smirnov, linux-pci, l.stach, bhelgaas
On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner 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
> ...
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> +struct imx6_pcie_drvdata {
> + enum imx6_pcie_variants variant;
> + int dbi_length;
> +};
Turning imx6_pcie drvdata into a struct is very nice, maybe in the
future some of the long case statements in this driver could be split
into per-soc functions called via drvdata.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-20 10:22 ` Leonard Crestez
@ 2018-11-20 10:30 ` Stefan Agner
2018-11-20 13:03 ` Leonard Crestez
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Agner @ 2018-11-20 10:30 UTC (permalink / raw)
To: Leonard Crestez
Cc: Richard Zhu, linux-kernel, jingoohan1, gustavo.pimentel, tpiepho,
andrew.smirnov, linux-pci, l.stach, bhelgaas
On 20.11.2018 11:22, Leonard Crestez wrote:
> On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner 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
>> ...
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>
>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
>
>> +struct imx6_pcie_drvdata {
>> + enum imx6_pcie_variants variant;
>> + int dbi_length;
>> +};
>
> Turning imx6_pcie drvdata into a struct is very nice, maybe in the
> future some of the long case statements in this driver could be split
> into per-soc functions called via drvdata.
Yeah I thought that too. At a quick glance I did not saw an obvious
contender. Should certainly help for similar cases in the future.
--
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread* RE: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-20 10:30 ` Stefan Agner
@ 2018-11-20 13:03 ` Leonard Crestez
2018-11-20 13:12 ` Stefan Agner
0 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2018-11-20 13:03 UTC (permalink / raw)
To: Stefan Agner, Richard Zhu
Cc: linux-kernel, jingoohan1, gustavo.pimentel, tpiepho,
andrew.smirnov, linux-pci, l.stach, bhelgaas
From: Stefan Agner <stefan@agner.ch>
> On 20.11.2018 11:22, Leonard Crestez wrote:
> > On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner 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
> >> ...
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> >
> >> +struct imx6_pcie_drvdata {
> >> + enum imx6_pcie_variants variant;
> >> + int dbi_length;
> >> +};
> >
> > Turning imx6_pcie drvdata into a struct is very nice, maybe in the
> > future some of the long case statements in this driver could be split
> > into per-soc functions called via drvdata.
>
> Yeah I thought that too. At a quick glance I did not saw an obvious
> contender. Should certainly help for similar cases in the future.
Yes, there are other cases in which it would be useful.
But I think it makes more sense to split introducing drvdata to a separate patch.
It's nice to separate functional and code cleanup changes.
For example maybe dbi_length causes issues and has to be reverted?
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2018-11-20 13:03 ` Leonard Crestez
@ 2018-11-20 13:12 ` Stefan Agner
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Agner @ 2018-11-20 13:12 UTC (permalink / raw)
To: Leonard Crestez
Cc: Richard Zhu, linux-kernel, jingoohan1, gustavo.pimentel, tpiepho,
andrew.smirnov, linux-pci, l.stach, bhelgaas
On 20.11.2018 14:03, Leonard Crestez wrote:
> From: Stefan Agner <stefan@agner.ch>
>> On 20.11.2018 11:22, Leonard Crestez wrote:
>> > On Mon, 2018-11-19 at 10:41 +0100, Stefan Agner 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
>> >> ...
>> >>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >>
>> >> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
>> >
>> >> +struct imx6_pcie_drvdata {
>> >> + enum imx6_pcie_variants variant;
>> >> + int dbi_length;
>> >> +};
>> >
>> > Turning imx6_pcie drvdata into a struct is very nice, maybe in the
>> > future some of the long case statements in this driver could be split
>> > into per-soc functions called via drvdata.
>>
>> Yeah I thought that too. At a quick glance I did not saw an obvious
>> contender. Should certainly help for similar cases in the future.
>
> Yes, there are other cases in which it would be useful.
>
> But I think it makes more sense to split introducing drvdata to a
> separate patch.
> It's nice to separate functional and code cleanup changes.
>
> For example maybe dbi_length causes issues and has to be reverted?
Ok, makes sense, will split drvdata introduction in its own patch.
--
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] PCI: dwc: allow to limit registers set length
@ 2019-02-06 9:57 Stefan Agner
2019-02-06 9:57 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Agner @ 2019-02-06 9:57 UTC (permalink / raw)
To: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: leonard.crestez, bhelgaas, linux-pci, linux-kernel, stefan
Add length to the struct dw_pcie and check that the accessors
dw_pcie_(rd|wr)_conf() do not read/write beyond that point.
Suggested-by: Trent Piepho <tpiepho@impinj.com>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Changes in v4:
- Move length check to dw_pcie_rd_conf
Changes in v5:
- Rebased ontop of pci/dwc
.../pci/controller/dwc/pcie-designware-host.c | 16 ++++++++++++++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 45ff5e4f8af6..bad54204fb52 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -612,14 +612,20 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
int size, u32 *val)
{
struct pcie_port *pp = bus->sysdata;
+ struct dw_pcie *pci;
if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
*val = 0xffffffff;
return PCIBIOS_DEVICE_NOT_FOUND;
}
- if (bus->number == pp->root_bus_nr)
+ if (bus->number == pp->root_bus_nr) {
+ pci = to_dw_pcie_from_pp(pp);
+ if (pci->dbi_length && where + size > pci->dbi_length)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
return dw_pcie_rd_own_conf(pp, where, size, val);
+ }
return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}
@@ -628,12 +634,18 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
int where, int size, u32 val)
{
struct pcie_port *pp = bus->sysdata;
+ struct dw_pcie *pci;
if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn)))
return PCIBIOS_DEVICE_NOT_FOUND;
- if (bus->number == pp->root_bus_nr)
+ if (bus->number == pp->root_bus_nr) {
+ pci = to_dw_pcie_from_pp(pp);
+ if (pci->dbi_length && where + size > pci->dbi_length)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
return dw_pcie_wr_own_conf(pp, where, size, val);
+ }
return dw_pcie_wr_other_conf(pp, bus, devfn, where, size, val);
}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 279000255ad1..d1d95119a422 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -226,6 +226,7 @@ struct dw_pcie_ops {
struct dw_pcie {
struct device *dev;
void __iomem *dbi_base;
+ int dbi_length;
void __iomem *dbi_base2;
/* Used when iatu_unroll_enabled is true */
void __iomem *atu_base;
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/2] PCI: imx6: limit DBI register length
2019-02-06 9:57 [PATCH 1/2] PCI: dwc: allow to limit registers set length Stefan Agner
@ 2019-02-06 9:57 ` Stefan Agner
2019-02-11 21:39 ` Bjorn Helgaas
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Agner @ 2019-02-06 9:57 UTC (permalink / raw)
To: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: leonard.crestez, bhelgaas, linux-pci, linux-kernel, stefan
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>
---
Changes in v3:
- Rebase on pci/dwc
Changes in v4:
- Rebase on pci/dwc
Changes in v5:
- Rebased ontop of pci/dwc
- Use DBI length of 0x200
drivers/pci/controller/dwc/pci-imx6.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c1d434ba3642..1ef7be1232f3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -55,6 +55,7 @@ enum imx6_pcie_variants {
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
u32 flags;
+ int dbi_length;
};
struct imx6_pcie {
@@ -1087,6 +1088,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
break;
}
+ pci->dbi_length = imx6_pcie->drvdata->dbi_length;
+
/* Grab turnoff reset */
imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
if (IS_ERR(imx6_pcie->turnoff_reset)) {
@@ -1170,6 +1173,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
.variant = IMX6Q,
.flags = IMX6_PCIE_FLAG_IMX6_PHY |
IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
+ .dbi_length = 0x200,
},
[IMX6SX] = {
.variant = IMX6SX,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2019-02-06 9:57 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
@ 2019-02-11 21:39 ` Bjorn Helgaas
2019-02-12 8:54 ` Lucas Stach
0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2019-02-11 21:39 UTC (permalink / raw)
To: Stefan Agner
Cc: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach,
tpiepho, leonard.crestez, linux-pci, linux-kernel
On Wed, Feb 06, 2019 at 10:57:32AM +0100, Stefan Agner 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
> ...
I assume this problem happens when using the pci_read_config() path or
something similar?
Could this be solved using pci_dev.cfg_size instead of building a new
dwc-specific mechanism? There are some quirks that set dev->cfg_size
to keep from reading past certain points in config space, e.g.,
quirk_citrine(), quirk_nfp6000().
I'm not necessarily opposed to doing it in dwc, but maybe there's some
advantage in reducing the number of ways of doing the same thing.
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> Changes in v3:
> - Rebase on pci/dwc
> Changes in v4:
> - Rebase on pci/dwc
> Changes in v5:
> - Rebased ontop of pci/dwc
> - Use DBI length of 0x200
>
> drivers/pci/controller/dwc/pci-imx6.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index c1d434ba3642..1ef7be1232f3 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -55,6 +55,7 @@ enum imx6_pcie_variants {
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> u32 flags;
> + int dbi_length;
> };
>
> struct imx6_pcie {
> @@ -1087,6 +1088,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> break;
> }
>
> + pci->dbi_length = imx6_pcie->drvdata->dbi_length;
> +
> /* Grab turnoff reset */
> imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> if (IS_ERR(imx6_pcie->turnoff_reset)) {
> @@ -1170,6 +1173,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .variant = IMX6Q,
> .flags = IMX6_PCIE_FLAG_IMX6_PHY |
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> + .dbi_length = 0x200,
> },
> [IMX6SX] = {
> .variant = IMX6SX,
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2019-02-11 21:39 ` Bjorn Helgaas
@ 2019-02-12 8:54 ` Lucas Stach
2019-02-12 11:33 ` Lorenzo Pieralisi
0 siblings, 1 reply; 13+ messages in thread
From: Lucas Stach @ 2019-02-12 8:54 UTC (permalink / raw)
To: Bjorn Helgaas, Stefan Agner
Cc: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, tpiepho,
leonard.crestez, linux-pci, linux-kernel
Hi Bjorn,
Am Montag, den 11.02.2019, 15:39 -0600 schrieb Bjorn Helgaas:
> On Wed, Feb 06, 2019 at 10:57:32AM +0100, Stefan Agner 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
> > ...
>
> I assume this problem happens when using the pci_read_config() path or
> something similar?
>
> Could this be solved using pci_dev.cfg_size instead of building a new
> dwc-specific mechanism? There are some quirks that set dev->cfg_size
> to keep from reading past certain points in config space, e.g.,
> quirk_citrine(), quirk_nfp6000().
>
> I'm not necessarily opposed to doing it in dwc, but maybe there's some
> advantage in reducing the number of ways of doing the same thing.
This actually started out as a quirk changing the cfg size. But the
valid config space size seems to be different between root ports that
share the same (broken) device ID (Synopsys abcd), so I doubt that this
would be easier and/or any cleaner to implement as a quirk.
Regards,
Lucas
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > Changes in v3:
> > - Rebase on pci/dwc
> > Changes in v4:
> > - Rebase on pci/dwc
> > Changes in v5:
> > - Rebased ontop of pci/dwc
> > - Use DBI length of 0x200
> >
> > drivers/pci/controller/dwc/pci-imx6.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index c1d434ba3642..1ef7be1232f3 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -55,6 +55,7 @@ enum imx6_pcie_variants {
> > struct imx6_pcie_drvdata {
> > > > enum imx6_pcie_variants variant;
> > > > u32 flags;
> > > > + int dbi_length;
> > };
> >
> > struct imx6_pcie {
> > @@ -1087,6 +1088,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > > break;
> > > > }
> >
> > > > + pci->dbi_length = imx6_pcie->drvdata->dbi_length;
> > +
> > > > /* Grab turnoff reset */
> > > > imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > > > if (IS_ERR(imx6_pcie->turnoff_reset)) {
> > @@ -1170,6 +1173,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > > .variant = IMX6Q,
> > > > .flags = IMX6_PCIE_FLAG_IMX6_PHY |
> > > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> > > > + .dbi_length = 0x200,
> > > > },
> > > > [IMX6SX] = {
> > > > .variant = IMX6SX,
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2019-02-12 8:54 ` Lucas Stach
@ 2019-02-12 11:33 ` Lorenzo Pieralisi
2019-02-12 19:07 ` Stefan Agner
0 siblings, 1 reply; 13+ messages in thread
From: Lorenzo Pieralisi @ 2019-02-12 11:33 UTC (permalink / raw)
To: Lucas Stach
Cc: Bjorn Helgaas, Stefan Agner, jingoohan1, gustavo.pimentel,
tpiepho, leonard.crestez, linux-pci, linux-kernel
On Tue, Feb 12, 2019 at 09:54:54AM +0100, Lucas Stach wrote:
> Hi Bjorn,
>
> Am Montag, den 11.02.2019, 15:39 -0600 schrieb Bjorn Helgaas:
> > On Wed, Feb 06, 2019 at 10:57:32AM +0100, Stefan Agner 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
> > > ...
> >
> > I assume this problem happens when using the pci_read_config() path or
> > something similar?
> >
> > Could this be solved using pci_dev.cfg_size instead of building a new
> > dwc-specific mechanism? There are some quirks that set dev->cfg_size
> > to keep from reading past certain points in config space, e.g.,
> > quirk_citrine(), quirk_nfp6000().
> >
> > I'm not necessarily opposed to doing it in dwc, but maybe there's some
> > advantage in reducing the number of ways of doing the same thing.
>
> This actually started out as a quirk changing the cfg size. But the
> valid config space size seems to be different between root ports that
> share the same (broken) device ID (Synopsys abcd), so I doubt that this
> would be easier and/or any cleaner to implement as a quirk.
There are two things here: matching the root port and setting
the cfg size limit.
I agree with Bjorn that the cfg size limit, given that it is
implemented in core code should be leveraged instead of reinventing
the wheel to solve the same problem in driver specific code.
In the quirk code I do not think it is that complicated to retrieve
the IMX variant to apply the quirk accordingly on the pci_dev.
Please let me know if that's feasible so that I can drop the
patches from the branch and update it with a new version.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] PCI: imx6: limit DBI register length
2019-02-12 11:33 ` Lorenzo Pieralisi
@ 2019-02-12 19:07 ` Stefan Agner
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Agner @ 2019-02-12 19:07 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Lucas Stach, Bjorn Helgaas, jingoohan1, gustavo.pimentel,
tpiepho, leonard.crestez, linux-pci, linux-kernel
On 12.02.2019 12:33, Lorenzo Pieralisi wrote:
> On Tue, Feb 12, 2019 at 09:54:54AM +0100, Lucas Stach wrote:
>> Hi Bjorn,
>>
>> Am Montag, den 11.02.2019, 15:39 -0600 schrieb Bjorn Helgaas:
>> > On Wed, Feb 06, 2019 at 10:57:32AM +0100, Stefan Agner 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
>> > > ...
>> >
>> > I assume this problem happens when using the pci_read_config() path or
>> > something similar?
>> >
>> > Could this be solved using pci_dev.cfg_size instead of building a new
>> > dwc-specific mechanism? There are some quirks that set dev->cfg_size
>> > to keep from reading past certain points in config space, e.g.,
>> > quirk_citrine(), quirk_nfp6000().
>> >
>> > I'm not necessarily opposed to doing it in dwc, but maybe there's some
>> > advantage in reducing the number of ways of doing the same thing.
>>
>> This actually started out as a quirk changing the cfg size. But the
>> valid config space size seems to be different between root ports that
>> share the same (broken) device ID (Synopsys abcd), so I doubt that this
>> would be easier and/or any cleaner to implement as a quirk.
For reference, this was the initial patch using
DECLARE_PCI_FIXUP_HEADER:
https://lore.kernel.org/lkml/20181019111350.6170-1-stefan@agner.ch/T/#u
>
> There are two things here: matching the root port and setting
> the cfg size limit.
>
> I agree with Bjorn that the cfg size limit, given that it is
> implemented in core code should be leveraged instead of reinventing
> the wheel to solve the same problem in driver specific code.
Seems sensible yes.
>
> In the quirk code I do not think it is that complicated to retrieve
> the IMX variant to apply the quirk accordingly on the pci_dev.
It seems that drivers/pci/controller/pcie-iproc.c uses FIXUP functions
which access driver specific structs. I think we can get from (struct
pci_host_bridge *)->sysdata to struct pcie_port * and from there to
struct dw_pci.
I will give this a try next week.
>
> Please let me know if that's feasible so that I can drop the
> patches from the branch and update it with a new version.
>
Fine for me.
--
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-02-12 19:07 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 9:41 [PATCH 1/2] PCI: dwc: allow to limit registers set length Stefan Agner
2018-11-19 9:41 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 1:06 ` Trent Piepho
2018-11-20 1:33 ` Trent Piepho
2018-11-20 10:22 ` Leonard Crestez
2018-11-20 10:30 ` Stefan Agner
2018-11-20 13:03 ` Leonard Crestez
2018-11-20 13:12 ` Stefan Agner
2019-02-06 9:57 [PATCH 1/2] PCI: dwc: allow to limit registers set length Stefan Agner
2019-02-06 9:57 ` [PATCH 2/2] PCI: imx6: limit DBI register length Stefan Agner
2019-02-11 21:39 ` Bjorn Helgaas
2019-02-12 8:54 ` Lucas Stach
2019-02-12 11:33 ` Lorenzo Pieralisi
2019-02-12 19:07 ` Stefan Agner
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).