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

[Leonard Crestez]

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.

> > 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 assigning to pci_dev->cfg_size from probe code?
Unfortunately there doesn't seem to be any straight-forward way to do
that from imx6_pcie or even dw_pcie, but maybe I'm missing something?

--
Regards,
Leonard