* [PATCH v7] PCI: imx6: limit DBI register length
@ 2019-02-25 16:02 Stefan Agner
2019-02-25 16:15 ` Leonard Crestez
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Agner @ 2019-02-25 16:02 UTC (permalink / raw)
To: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach, tpiepho
Cc: leonard.crestez, bhelgaas, linux-pci, linux-kernel, Stefan Agner
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 <stefan@agner.ch>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7] PCI: imx6: limit DBI register length
2019-02-25 16:02 [PATCH v7] PCI: imx6: limit DBI register length Stefan Agner
@ 2019-02-25 16:15 ` Leonard Crestez
2019-02-25 16:52 ` Trent Piepho
0 siblings, 1 reply; 6+ messages in thread
From: Leonard Crestez @ 2019-02-25 16:15 UTC (permalink / raw)
To: stefan
Cc: Richard Zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
gustavo.pimentel, tpiepho, linux-pci, bhelgaas, l.stach
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] PCI: imx6: limit DBI register length
2019-02-25 16:15 ` Leonard Crestez
@ 2019-02-25 16:52 ` Trent Piepho
2019-02-25 20:19 ` Bjorn Helgaas
2019-02-26 11:36 ` Stefan Agner
0 siblings, 2 replies; 6+ messages in thread
From: Trent Piepho @ 2019-02-25 16:52 UTC (permalink / raw)
To: stefan, leonard.crestez
Cc: hongxing.zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
gustavo.pimentel, linux-pci, l.stach, bhelgaas
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] PCI: imx6: limit DBI register length
2019-02-25 16:52 ` Trent Piepho
@ 2019-02-25 20:19 ` Bjorn Helgaas
2019-02-26 9:55 ` Stefan Agner
2019-02-26 11:36 ` Stefan Agner
1 sibling, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2019-02-25 20:19 UTC (permalink / raw)
To: Trent Piepho
Cc: stefan, leonard.crestez, hongxing.zhu, linux-kernel, jingoohan1,
lorenzo.pieralisi, gustavo.pimentel, linux-pci, l.stach,
Thinh Nguyen
[+cc Thinh]
On Mon, Feb 25, 2019 at 10:52 AM Trent Piepho <tpiepho@impinj.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] PCI: imx6: limit DBI register length
2019-02-25 20:19 ` Bjorn Helgaas
@ 2019-02-26 9:55 ` Stefan Agner
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2019-02-26 9:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Trent Piepho, leonard.crestez, hongxing.zhu, linux-kernel,
jingoohan1, lorenzo.pieralisi, gustavo.pimentel, linux-pci,
l.stach, Thinh Nguyen
On 25.02.2019 21:19, Bjorn Helgaas wrote:
> [+cc Thinh]
>
> On Mon, Feb 25, 2019 at 10:52 AM Trent Piepho <tpiepho@impinj.com> 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 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] PCI: imx6: limit DBI register length
2019-02-25 16:52 ` Trent Piepho
2019-02-25 20:19 ` Bjorn Helgaas
@ 2019-02-26 11:36 ` Stefan Agner
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Agner @ 2019-02-26 11:36 UTC (permalink / raw)
To: Trent Piepho
Cc: leonard.crestez, hongxing.zhu, linux-kernel, jingoohan1,
lorenzo.pieralisi, gustavo.pimentel, linux-pci, l.stach,
bhelgaas
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-26 11:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-25 16:02 [PATCH v7] PCI: imx6: limit DBI register length Stefan Agner
2019-02-25 16:15 ` Leonard Crestez
2019-02-25 16:52 ` Trent Piepho
2019-02-25 20:19 ` Bjorn Helgaas
2019-02-26 9:55 ` Stefan Agner
2019-02-26 11:36 ` Stefan Agner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.