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