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.
