Messages in this thread | | | Date | Tue, 12 Feb 2019 20:07:32 +0100 | From | Stefan Agner <> | Subject | Re: [PATCH 2/2] PCI: imx6: limit DBI register length |
| |
On 12.02.2019 12:33, Lorenzo Pieralisi wrote: > On Tue, Feb 12, 2019 at 09:54:54AM +0100, Lucas Stach wrote: >> Hi Bjorn, >> >> Am Montag, den 11.02.2019, 15:39 -0600 schrieb Bjorn Helgaas: >> > On Wed, Feb 06, 2019 at 10:57:32AM +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 assume this problem happens when using the pci_read_config() path or >> > something similar? >> > >> > Could this be solved using pci_dev.cfg_size instead of building a new >> > dwc-specific mechanism? There are some quirks that set dev->cfg_size >> > to keep from reading past certain points in config space, e.g., >> > quirk_citrine(), quirk_nfp6000(). >> > >> > I'm not necessarily opposed to doing it in dwc, but maybe there's some >> > advantage in reducing the number of ways of doing the same thing. >> >> This actually started out as a quirk changing the cfg size. But the >> valid config space size seems to be different between root ports that >> share the same (broken) device ID (Synopsys abcd), so I doubt that this >> would be easier and/or any cleaner to implement as a quirk.
For reference, this was the initial patch using DECLARE_PCI_FIXUP_HEADER: https://lore.kernel.org/lkml/20181019111350.6170-1-stefan@agner.ch/T/#u
> > There are two things here: matching the root port and setting > the cfg size limit. > > I agree with Bjorn that the cfg size limit, given that it is > implemented in core code should be leveraged instead of reinventing > the wheel to solve the same problem in driver specific code.
Seems sensible yes.
> > In the quirk code I do not think it is that complicated to retrieve > the IMX variant to apply the quirk accordingly on the pci_dev.
It seems that drivers/pci/controller/pcie-iproc.c uses FIXUP functions which access driver specific structs. I think we can get from (struct pci_host_bridge *)->sysdata to struct pcie_port * and from there to struct dw_pci.
I will give this a try next week.
> > Please let me know if that's feasible so that I can drop the > patches from the branch and update it with a new version. >
Fine for me.
-- Stefan
| |