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

[Trent Piepho <tpiepho@impinj.com>]

On Mon, 2018-11-26 at 10:16 +0000, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > On 20.11.2018 20:13, Trent Piepho wrote:
> > > On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
> > > > On Tue, 2018-11-20 at 17:56 +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 don't know exactly where this limitation comes from, I can indeed
> > > > reproduce a stack dump when dumping pci config from /sys/
> > > > 
> > > > Unfortunately this seems to block access to registers used for
> > > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > > 
> > > > 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > > 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > > 			    4, &val);
> > > > 
> > > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > > 
> > > On IMX7d, there are significant blocks of 00s in the config space, and
> > > all 0xff at 0xb50 on up.
> > > 
> > > I.e., significant portions are empty, in the middle of the config
> > > space, not just at the end.
> > > 
> > > But they can be read without problem.
> > > 
> > > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > > returning zeros like imx7d.  In that case it really needs something
> > > more complex to prevent abort than just a length.
> > 
> > Yeah it seems those SoCs behave differently.
> > 
> > Describing a register set with holes will get complicated, I guess it
> > would ask for a regmap...
> 
> The PortLogic area seems to be always at 0x700-0xA00, there are defines
> for it. So this would only require one additional range, no regmap
> stuff.
> 
> I don't know if making portlogic accessible to userspace is useful. In
> theory somebody could read debug DWC registers via /sys/blah/config,
> but there are better ways to read HW registers from userspace.
> 
> Browsing through the docs I can't find stuff like reads with side-
> effects so I can't say that it's harmful either.

I doubt those register are used much from userspace, since AFAIK there
are no public docs for them.  I have dumped the config regs from
userspace in imx7d to debug a hang problem, but those registers weren't
useful to me.  Maybe they would have been, no docs...

> > > It also seems to me that this doesn't need to be in the internal pci
> > > config access functions.  The driver shouldn't be reading registers
> > > that don't exist anyway.  It's really about trying to fix sysfs access
> > > to registers that don't exist.  So maybe it should be done there.
> > 
> > That was my first approach
> 
> Doing it on a per-soc basis is better, this seems to affect both 6q and
> 6qp (those are distinct!) but not others.
> 
> The pci config area from userspace is accessed through pci_ops.read =
> dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
> So moving the dbi_length check up one level would work easily.

What about something like:

static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
                           int size, u32 *val)
{
	struct pcie_port *pp = bus->sysdata;

	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 (pp->ops->cfg_valid && !pp->ops_cfg_valid(pp, where, size)) {
+			*val = 0xffffffff;
+			return PCIBIOS_SOMETHING_GOES_HERE;
+		}
+
                return dw_pcie_rd_own_conf(pp, where, size, val);
+	}
	return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}

imx6q and imx6qp can provide cfg_valid that checks their range.