Define the length of the DBI registers and limit config space to its
length. 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 <[email protected]>
---
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
Changes in v6:
- Use pci_dev.cfg_size mechanism to limit config space (this made patch 1
of previous versions of this patchset obsolete).
Changes in v7:
- Restrict fixup to Synopsys/0xabcd
- Apply cfg_size limitation only if dbi_length is specified
drivers/pci/controller/dwc/pci-imx6.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index aaa9489e2140..4317d2fffb67 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -56,6 +56,7 @@ enum imx6_pcie_variants {
struct imx6_pcie_drvdata {
enum imx6_pcie_variants variant;
u32 flags;
+ int dbi_length;
};
struct imx6_pcie {
@@ -912,6 +913,25 @@ static const struct dw_pcie_ops dw_pcie_ops = {
/* No special ops needed, but pcie-designware still expects this struct */
};
+static void imx6_pcie_quirk(struct pci_dev *dev)
+{
+ struct pci_bus *bus = dev->bus;
+ struct pcie_port *pp = bus->sysdata;
+
+ if (bus->number == pp->root_bus_nr) {
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+ /*
+ * Limit config length to avoid the kernel reading beyond
+ * the register set and causing an abort on i.MX 6Quad
+ */
+ if (imx6_pcie->drvdata->dbi_length)
+ dev->cfg_size = imx6_pcie->drvdata->dbi_length;
+ }
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
+
#ifdef CONFIG_PM_SLEEP
static void imx6_pcie_ltssm_disable(struct device *dev)
{
@@ -1242,6 +1262,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 Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers
> beyond that point, avoiding the following abort on a i.MX 6Quad:
>
> +static void imx6_pcie_quirk(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pcie_port *pp = bus->sysdata;
> +
> + if (bus->number == pp->root_bus_nr) {
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> + /*
> + * Limit config length to avoid the kernel reading beyond
> + * the register set and causing an abort on i.MX 6Quad
> + */
> + if (imx6_pcie->drvdata->dbi_length)
> + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> + }
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
This looks like a default from SYNOPSYS so it likely run on other SOCs
using the DesignWare PCI IP and crash because of those unchecked casts.
There might also be some way to configure pci ids before boot (not
sure, didn't check).
Maybe you should check the device driver name in the quirk function?
--
Regards,
Leonard
On Mon, 2019-02-25 at 16:15 +0000, Leonard Crestez wrote:
> On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
> > Define the length of the DBI registers and limit config space to its
> > length. This makes sure that the kernel does not access registers
> > beyond that point, avoiding the following abort on a i.MX 6Quad:
> >
> > +static void imx6_pcie_quirk(struct pci_dev *dev)
> > +{
> > + struct pci_bus *bus = dev->bus;
> > + struct pcie_port *pp = bus->sysdata;
> > +
> > + if (bus->number == pp->root_bus_nr) {
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > +
> > + /*
> > + * Limit config length to avoid the kernel reading beyond
> > + * the register set and causing an abort on i.MX 6Quad
> > + */
> > + if (imx6_pcie->drvdata->dbi_length)
> > + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> > + }
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
>
> This looks like a default from SYNOPSYS so it likely run on other SOCs
> using the DesignWare PCI IP and crash because of those unchecked casts.
Yes, it's used on IMX7d too. But it's worse than that, there's a USB
controller core that uses the same vendor and device id,
PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3. The quirk for that one uses class ==
PCI_CLASS_SERIAL_USB_DEVICE to avoid matching this PCI-e IP. See
thread "PCI: Check for USB xHCI class for HAPS platform"
[+cc Thinh]
On Mon, Feb 25, 2019 at 10:52 AM Trent Piepho <[email protected]> wrote:
> On Mon, 2019-02-25 at 16:15 +0000, Leonard Crestez wrote:
> > On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
> > > Define the length of the DBI registers and limit config space to its
> > > length. This makes sure that the kernel does not access registers
> > > beyond that point, avoiding the following abort on a i.MX 6Quad:
> > >
> > > +static void imx6_pcie_quirk(struct pci_dev *dev)
> > > +{
> > > + struct pci_bus *bus = dev->bus;
> > > + struct pcie_port *pp = bus->sysdata;
> > > +
> > > + if (bus->number == pp->root_bus_nr) {
> > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> > > +
> > > + /*
> > > + * Limit config length to avoid the kernel reading beyond
> > > + * the register set and causing an abort on i.MX 6Quad
> > > + */
> > > + if (imx6_pcie->drvdata->dbi_length)
> > > + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> > > + }
> > > +}
> > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
> >
> > This looks like a default from SYNOPSYS so it likely run on other SOCs
> > using the DesignWare PCI IP and crash because of those unchecked casts.
>
> Yes, it's used on IMX7d too. But it's worse than that, there's a USB
> controller core that uses the same vendor and device id,
> PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3. The quirk for that one uses class ==
> PCI_CLASS_SERIAL_USB_DEVICE to avoid matching this PCI-e IP. See
> thread "PCI: Check for USB xHCI class for HAPS platform"
If we could get these vendors to allocate their own Vendor/Device IDs,
maybe we could consider a DECLARE_PCI_FIXUP_EARLY quirk that would fix
up pdev->vendor and pdev->device? That might be cleaner than
cluttering all these quirks with details of this screwup.
Bjorn
On 25.02.2019 21:19, Bjorn Helgaas wrote:
> [+cc Thinh]
>
> On Mon, Feb 25, 2019 at 10:52 AM Trent Piepho <[email protected]> wrote:
>> On Mon, 2019-02-25 at 16:15 +0000, Leonard Crestez wrote:
>> > On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
>> > > Define the length of the DBI registers and limit config space to its
>> > > length. This makes sure that the kernel does not access registers
>> > > beyond that point, avoiding the following abort on a i.MX 6Quad:
>> > >
>> > > +static void imx6_pcie_quirk(struct pci_dev *dev)
>> > > +{
>> > > + struct pci_bus *bus = dev->bus;
>> > > + struct pcie_port *pp = bus->sysdata;
>> > > +
>> > > + if (bus->number == pp->root_bus_nr) {
>> > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> > > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
>> > > +
>> > > + /*
>> > > + * Limit config length to avoid the kernel reading beyond
>> > > + * the register set and causing an abort on i.MX 6Quad
>> > > + */
>> > > + if (imx6_pcie->drvdata->dbi_length)
>> > > + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
>> > > + }
>> > > +}
>> > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
>> >
>> > This looks like a default from SYNOPSYS so it likely run on other SOCs
>> > using the DesignWare PCI IP and crash because of those unchecked casts.
>>
>> Yes, it's used on IMX7d too. But it's worse than that, there's a USB
>> controller core that uses the same vendor and device id,
>> PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3. The quirk for that one uses class ==
>> PCI_CLASS_SERIAL_USB_DEVICE to avoid matching this PCI-e IP. See
>> thread "PCI: Check for USB xHCI class for HAPS platform"
>
> If we could get these vendors to allocate their own Vendor/Device IDs,
> maybe we could consider a DECLARE_PCI_FIXUP_EARLY quirk that would fix
> up pdev->vendor and pdev->device? That might be cleaner than
> cluttering all these quirks with details of this screwup.
According to http://www.pcilookup.com there is a vendor/product id allocated
for recovery mode (Freescale i.MX 6, 15a2:0054). Is this a real PCI id?
The same Vendor/Device ID is used for USB recovery...
--
Stefan
On 25.02.2019 17:52, Trent Piepho wrote:
> On Mon, 2019-02-25 at 16:15 +0000, Leonard Crestez wrote:
>> On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
>> > Define the length of the DBI registers and limit config space to its
>> > length. This makes sure that the kernel does not access registers
>> > beyond that point, avoiding the following abort on a i.MX 6Quad:
>> >
>> > +static void imx6_pcie_quirk(struct pci_dev *dev)
>> > +{
>> > + struct pci_bus *bus = dev->bus;
>> > + struct pcie_port *pp = bus->sysdata;
>> > +
>> > + if (bus->number == pp->root_bus_nr) {
>> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
>> > +
>> > + /*
>> > + * Limit config length to avoid the kernel reading beyond
>> > + * the register set and causing an abort on i.MX 6Quad
>> > + */
>> > + if (imx6_pcie->drvdata->dbi_length)
>> > + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
>> > + }
>> > +}
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
>>
>> This looks like a default from SYNOPSYS so it likely run on other SOCs
>> using the DesignWare PCI IP and crash because of those unchecked casts.
>
> Yes, it's used on IMX7d too. But it's worse than that, there's a USB
> controller core that uses the same vendor and device id,
> PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3. The quirk for that one uses class ==
> PCI_CLASS_SERIAL_USB_DEVICE to avoid matching this PCI-e IP. See
> thread "PCI: Check for USB xHCI class for HAPS platform"
Hm, I see, something like this should fix this:
DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd,
PCI_CLASS_BRIDGE_PCI, 8, imx6_pcie_quirk);
(this needs "PCI: Work around Synopsys duplicate Device ID (HAPS USB3,
NXP i.MX)" applied, which is in v5.0-rc8 already).
--
Stefan