Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook

[Rob Herring <robh@kernel.org>]

On Mon, Oct 05, 2020 at 09:31:54AM +0200, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Oct 4, 2020 at 4:16 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> > On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
> > [...]
> > >> +static int __init rcar_pcie_init(void)
> > >> +{
> > >> +#ifdef CONFIG_ARM_LPAE
> > >> +       hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> > >> +                       "asynchronous external abort");
> > >> +#else
> > >> +       hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> > >> +                       "imprecise external abort");
> > >> +#endif
> > >
> > > As there can be only a single handler, this may interfere with a handler
> > > for another platform in a multi-platform kernel.
> > > Hence I think this should not be done unconditionally, but be moved to
> > > the driver's .probe() callback.
> >
> > Why is nobody doing this in the probe code then ? It seems all the other
> 
> drivers/pci/controller/dwc/pci-keystone.c is:
> 
>   ks_pcie_probe()
>     ks_pcie_add_pcie_port()
>       dw_pcie_host_init()
>         pp->ops->host_init(pp) = ks_pcie_host_init()
>           hook_fault_code()

Looks broken in deferred probe case as hook_fault_code is __init.

Really, hook_fault_code needs to be exported so these drivers can be 
modules. Or we split out all the abort handlers to a separate broken, 
aborting PCI hosts module.


> > drivers which hook fault code do it in init as well. I can imagine that
> 
> Probably nobody bothered exercising the external abort handler on
> multi-platform kernels?
> 
> > something might trip the fault handler even before probe is called, e.g.
> > some PM handling or simply user accessing that PCIe area using setpci.

I don't see how that's possible. You'd first hit translation faults as 
nothing is mapped.


> If that is the case, it must indeed by done earlier, but still
> conditional on the presence of the actual PCIe controller.

imx6 should be conditional too.

Rob