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

[PATCH 1/2] 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
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

[PATCH 2/2] 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
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

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

[Lorenzo Pieralisi]

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
> 

Re: [PATCH 1/2] Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

[Gustavo Pimentel]

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>

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

[Lorenzo Pieralisi]

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
> 

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

[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.

> 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
> 

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

[Lucas Stach]

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
> > 

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

[Lorenzo Pieralisi]

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

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

[Stefan Agner]

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

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

[Stefan Agner]

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

RE: [PATCH 2/2] PCI: imx6: limit DBI register length

[Leonard Crestez]

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

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

[Stefan Agner]

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

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

[Leonard Crestez]

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.

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

[Trent Piepho]

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], },
	...
};

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

[Trent Piepho]

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>

[PATCH 2/2] 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>
---
 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