[PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[Stefan Agner]

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

 .../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 692dd1b264fb..9fc0f7bd99f0 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -606,14 +606,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);
 }
@@ -622,12 +628,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 9943d8c68335..9cd7bdc94200 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -229,6 +229,7 @@ struct dw_pcie {
 	void __iomem		*dbi_base2;
 	/* Used when iatu_unroll_enabled is true */
 	void __iomem		*atu_base;
+	int			dbi_length;
 	u32			num_viewport;
 	u8			iatu_unroll_enabled;
 	struct pcie_port	pp;
-- 
2.19.1

[PATCH v4 2/3] PCI: imx6: introduce drvdata

[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
Changes in v3:
- Rebase on pci/dwc
Changes in v4:
- Rebase on pci/dwc

 drivers/pci/controller/dwc/pci-imx6.c | 48 ++++++++++++++++-----------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 26087b3da590..45cbdf2ccf80 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -41,6 +41,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;
@@ -53,7 +57,6 @@ struct imx6_pcie {
 	struct reset_control	*pciephy_reset;
 	struct reset_control	*apps_reset;
 	struct reset_control	*turnoff_reset;
-	enum imx6_pcie_variants variant;
 	u32			tx_deemph_gen1;
 	u32			tx_deemph_gen2_3p5db;
 	u32			tx_deemph_gen2_6db;
@@ -66,6 +69,7 @@ struct imx6_pcie {
 	struct device		*pd_pcie;
 	/* power domain for pcie phy */
 	struct device		*pd_pcie_phy;
+	const struct imx6_pcie_drvdata *drvdata;
 };
 
 /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
@@ -340,7 +344,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);
@@ -382,7 +386,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) {
@@ -485,7 +489,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);
@@ -523,7 +527,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);
@@ -645,7 +649,7 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6Q:
 	case IMX6SX:
 	case IMX6QP:
@@ -698,7 +702,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
@@ -801,7 +805,7 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 	case IMX6QP:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
@@ -827,7 +831,7 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
 	}
 
 	/* Others poke directly at IOMUXC registers */
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				IMX6SX_GPR12_PCIE_PM_TURN_OFF,
@@ -857,7 +861,7 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_phy);
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 		clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
 		break;
@@ -873,8 +877,8 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 
 static inline bool imx6_pcie_supports_suspend(struct imx6_pcie *imx6_pcie)
 {
-	return (imx6_pcie->variant == IMX7D ||
-		imx6_pcie->variant == IMX6SX);
+	return (imx6_pcie->drvdata->variant == IMX7D ||
+		imx6_pcie->drvdata->variant == IMX6SX);
 }
 
 static int imx6_pcie_suspend_noirq(struct device *dev)
@@ -939,8 +943,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);
@@ -984,7 +987,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");
@@ -1082,11 +1085,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

[PATCH v4 3/3] PCI: imx6: limit DBI register length

[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>
---
Changes in v3:
- Rebase on pci/dwc
Changes in v4:
- Rebase on pci/dwc

 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 45cbdf2ccf80..a69eb625ed18 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -43,6 +43,7 @@ enum imx6_pcie_variants {
 
 struct imx6_pcie_drvdata {
 	enum imx6_pcie_variants variant;
+	int			dbi_length;
 };
 
 struct imx6_pcie {
@@ -1015,6 +1016,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)) {
@@ -1086,7 +1089,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

Re: [PATCH v4 3/3] PCI: imx6: limit DBI register length

[Leonard Crestez]

On Tue, 2018-12-04 at 17:55 +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
> 
>  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 },

Also seems to affect IMX6QP variant (but not others).

Lucas suggested 0x15c because that's the last register documented in
the datasheet but the real HW limit is 0x200, wouldn't it make more
sense to use that?

--
Regards,
Leonard

Re: [PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[Lorenzo Pieralisi]

On Tue, Dec 04, 2018 at 05:55:26PM +0100, Stefan Agner wrote:
> 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
> 
>  .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
>  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)

Hi Stefan,

I wanted to ask you if this series should be considered for v5.1
inclusion, it is in the PCI backlog. If it is, let me have a look
and if it is OK to go I will likely ask you to rebase it.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 692dd1b264fb..9fc0f7bd99f0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -606,14 +606,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);
>  }
> @@ -622,12 +628,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 9943d8c68335..9cd7bdc94200 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -229,6 +229,7 @@ struct dw_pcie {
>  	void __iomem		*dbi_base2;
>  	/* Used when iatu_unroll_enabled is true */
>  	void __iomem		*atu_base;
> +	int			dbi_length;
>  	u32			num_viewport;
>  	u8			iatu_unroll_enabled;
>  	struct pcie_port	pp;
> -- 
> 2.19.1
> 

Re: [PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[Stefan Agner]

On 30.01.2019 18:54, Lorenzo Pieralisi wrote:
> On Tue, Dec 04, 2018 at 05:55:26PM +0100, Stefan Agner wrote:
>> 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
>>
>>  .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
>>  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
>>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> Hi Stefan,
> 
> I wanted to ask you if this series should be considered for v5.1
> inclusion, it is in the PCI backlog. If it is, let me have a look
> and if it is OK to go I will likely ask you to rebase it.

Yes please. With this last change I did not see any regression anymore
so far.

Andrey Smirnov picked up the second patch: "PCI: imx6: introduce
drvdata". Not sure what the plan is with his patchset, if it gets merged
into v5.1 too then I probably better drop this patch and rebase ontop of
his series.

--
Stefan

> 
> Thanks,
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 692dd1b264fb..9fc0f7bd99f0 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -606,14 +606,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);
>>  }
>> @@ -622,12 +628,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 9943d8c68335..9cd7bdc94200 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -229,6 +229,7 @@ struct dw_pcie {
>>  	void __iomem		*dbi_base2;
>>  	/* Used when iatu_unroll_enabled is true */
>>  	void __iomem		*atu_base;
>> +	int			dbi_length;
>>  	u32			num_viewport;
>>  	u8			iatu_unroll_enabled;
>>  	struct pcie_port	pp;
>> --
>> 2.19.1
>>

Re: [PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[Lorenzo Pieralisi]

On Thu, Jan 31, 2019 at 10:08:11AM +0100, Stefan Agner wrote:
> On 30.01.2019 18:54, Lorenzo Pieralisi wrote:
> > On Tue, Dec 04, 2018 at 05:55:26PM +0100, Stefan Agner wrote:
> >> 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
> >>
> >>  .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
> >>  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > Hi Stefan,
> > 
> > I wanted to ask you if this series should be considered for v5.1
> > inclusion, it is in the PCI backlog. If it is, let me have a look
> > and if it is OK to go I will likely ask you to rebase it.
> 
> Yes please. With this last change I did not see any regression anymore
> so far.
> 
> Andrey Smirnov picked up the second patch: "PCI: imx6: introduce
> drvdata". Not sure what the plan is with his patchset, if it gets merged
> into v5.1 too then I probably better drop this patch and rebase ontop of
> his series.

Ok, I will get back to you when I merged Andrey's series so that you
can rebase on top of my pci/dwc branch with Andrey's patches applied.

Lorenzo

Re: [PATCH v4 1/3] PCI: dwc: allow to limit registers set length

[Lorenzo Pieralisi]

On Thu, Jan 31, 2019 at 10:08:11AM +0100, Stefan Agner wrote:
> On 30.01.2019 18:54, Lorenzo Pieralisi wrote:
> > On Tue, Dec 04, 2018 at 05:55:26PM +0100, Stefan Agner wrote:
> >> 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
> >>
> >>  .../pci/controller/dwc/pcie-designware-host.c    | 16 ++++++++++++++--
> >>  drivers/pci/controller/dwc/pcie-designware.h     |  1 +
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > Hi Stefan,
> > 
> > I wanted to ask you if this series should be considered for v5.1
> > inclusion, it is in the PCI backlog. If it is, let me have a look
> > and if it is OK to go I will likely ask you to rebase it.
> 
> Yes please. With this last change I did not see any regression anymore
> so far.
> 
> Andrey Smirnov picked up the second patch: "PCI: imx6: introduce
> drvdata". Not sure what the plan is with his patchset, if it gets merged
> into v5.1 too then I probably better drop this patch and rebase ontop of
> his series.

You can now rebase it on top of my pci/dwc branch and send a v5, I
will mark this series as superseded in the PCI patch queue.

Lorenzo

> Stefan
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> index 692dd1b264fb..9fc0f7bd99f0 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> @@ -606,14 +606,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);
> >>  }
> >> @@ -622,12 +628,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 9943d8c68335..9cd7bdc94200 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >> @@ -229,6 +229,7 @@ struct dw_pcie {
> >>  	void __iomem		*dbi_base2;
> >>  	/* Used when iatu_unroll_enabled is true */
> >>  	void __iomem		*atu_base;
> >> +	int			dbi_length;
> >>  	u32			num_viewport;
> >>  	u8			iatu_unroll_enabled;
> >>  	struct pcie_port	pp;
> >> --
> >> 2.19.1
> >>