Linux-PCI Archive on lore.kernel.org
 help / Atom feed
* [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
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ 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	[flat|nested] 16+ 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
  2019-02-06 18:02 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ 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	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PCI: dwc: allow to limit registers set length
  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-06 18:02 ` Lorenzo Pieralisi
  2019-02-07 15:08 ` [PATCH 1/2] Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> Gustavo Pimentel
  2019-02-08 11:10 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
  3 siblings, 0 replies; 16+ messages in thread
From: Lorenzo Pieralisi @ 2019-02-06 18:02 UTC (permalink / raw)
  To: Stefan Agner, gustavo.pimentel
  Cc: jingoohan1, l.stach, tpiepho, leonard.crestez, bhelgaas,
	linux-pci, linux-kernel

On Wed, Feb 06, 2019 at 10:57:31AM +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
> 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(-)

Hi Gustavo,

I need your ACK on this patch to proceed, thanks.

Lorenzo

> 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	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
  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-06 18:02 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
@ 2019-02-07 15:08 ` Gustavo Pimentel
  2019-02-08 11:10 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
  3 siblings, 0 replies; 16+ messages in thread
From: Gustavo Pimentel @ 2019-02-07 15:08 UTC (permalink / raw)
  To: Stefan Agner, lorenzo.pieralisi, jingoohan1, gustavo.pimentel,
	l.stach, tpiepho
  Cc: leonard.crestez, bhelgaas, linux-pci, linux-kernel

On 06/02/2019 09:57, 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
> 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;
> 

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PCI: dwc: allow to limit registers set length
  2019-02-06  9:57 [PATCH 1/2] PCI: dwc: allow to limit registers set length Stefan Agner
                   ` (2 preceding siblings ...)
  2019-02-07 15:08 ` [PATCH 1/2] Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> Gustavo Pimentel
@ 2019-02-08 11:10 ` Lorenzo Pieralisi
  3 siblings, 0 replies; 16+ messages in thread
From: Lorenzo Pieralisi @ 2019-02-08 11:10 UTC (permalink / raw)
  To: Stefan Agner
  Cc: jingoohan1, gustavo.pimentel, l.stach, tpiepho, leonard.crestez,
	bhelgaas, linux-pci, linux-kernel

On Wed, Feb 06, 2019 at 10:57:31AM +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
> 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(-)

Applied to pci/dwc for v5.1, thanks.

Lorenzo

PS: Remember adding a version number to the patches next time please,
thanks.

> 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	[flat|nested] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ messages in thread

* [PATCH 2/2] PCI: imx6: limit DBI register length
  2018-11-19  9:41 Stefan Agner
@ 2018-11-19  9:41 ` Stefan Agner
  2018-11-20  1:06   ` Trent Piepho
                     ` (2 more replies)
  0 siblings, 3 replies; 16+ 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	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2019-02-06 18:02 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
2019-02-07 15:08 ` [PATCH 1/2] Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> Gustavo Pimentel
2019-02-08 11:10 ` [PATCH 1/2] PCI: dwc: allow to limit registers set length Lorenzo Pieralisi
  -- strict thread matches above, loose matches on Subject: below --
2018-11-19  9:41 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

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox