diff mbox series

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

Message ID 119211b4f2e9ada55b86041d009656e49c2b5281.1549446867.git.stefan@agner.ch
State Accepted
Delegated to: Lorenzo Pieralisi
Headers show
Series [1/2] PCI: dwc: allow to limit registers set length | expand

Commit Message

Stefan Agner Feb. 6, 2019, 9:57 a.m. UTC 
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(+)

Comments

Bjorn Helgaas Feb. 11, 2019, 9:39 p.m. UTC | #1 
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
>
Lucas Stach Feb. 12, 2019, 8:54 a.m. UTC | #2 
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
> >
Lorenzo Pieralisi Feb. 12, 2019, 11:33 a.m. UTC | #3 
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
Stefan Agner Feb. 12, 2019, 7:07 p.m. UTC | #4 
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
diff mbox series

Patch

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,