RE: [PATCH] PCI: imx6: install the fault handler only if we are really running on a compatible device

[Hongxing Zhu]

> -----Original Message-----
> From: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
> Sent: 2023年2月28日 16:44
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>; Lucas Stach
> <l.stach@xxxxxxxxxxxxxx>; Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Krzysztof
> Wilczy?ski <kw@xxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Shawn
> Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-pci@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> letux-kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxxxxx; H. Nikolaus
> Schaller <hns@xxxxxxxxxxxxx>
> Subject: [PATCH] PCI: imx6: install the fault handler only if we are really running
> on a compatible device
> 
> commit bb38919ec56e ("PCI: imx6: Add support for i.MX6 PCIe controller")
> added a fault hook to this driver in the probe function. So it was only installed if
> needed.
> 
> commit bde4a5a00e76 ("PCI: imx6: Allow probe deferral by reset GPIO") moved
> it from probe to driver init which installs the hook unconditionally as soon as the
> driver is compiled into a kernel.
> 
> When this driver is compiled as a module, the hook is not registered until after
> the driver has been matched with a .compatible and loaded.
> 
> commit 415b6185c541 ("PCI: imx6: Fix config read timeout handling") extended
> the fault handling code.
> 
> commit 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ") added some
> protection for non-ARM architectures, but this does not protect non-i.MX ARM
> architectures.
> 
> Since fault handlers can be triggered on any architecture for different reasons,
> there is no guarantee that they will be triggered only for the assumed situation,
> leading to improper error handling (i.MX6-specific
> imx6q_pcie_abort_handler) on foreign systems.
> 
> I had seen strange L3 imprecise external abort messages several times on
> OMAP4 and OMAP5 devices and couldn't make sense of them until I realized
> they were related to this unused imx6q driver because I had
> CONFIG_PCI_IMX6=y.
> 
> Note that CONFIG_PCI_IMX6=y is useful for kernel binaries that are designed to
> run on different ARM SoC and be differentiated only by device tree binaries. So
> turning off CONFIG_PCI_IMX6 is not a solution.
> 
> Therefore we check the compatible in the init function before registering the
> fault handler.
> 
> Fixes: bde4a5a00e76 ("PCI: imx6: Allow probe deferral by reset GPIO")
> Fixes: 415b6185c541 ("PCI: imx6: Fix config read timeout handling")
> Fixes: 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ")
> 
> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
Hi H.Nikolaus:
I'm fine with these changes. Thanks.
Reviewed-by: Richard Zhu <hongxing.zhu@xxxxxxx>

Best Regards
Richard Zhu
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 1dde5c579edc8..89774aa187ae8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1402,6 +1402,15 @@
> DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd,
> static int __init imx6_pcie_init(void)  {  #ifdef CONFIG_ARM
> +	const struct of_device_id *reboot_id;
> +	struct device_node *np;
> +
> +	np = of_find_matching_node_and_match(NULL, imx6_pcie_of_match,
> +					     &reboot_id);
> +	if (!np)
> +		return -ENODEV;
> +	of_node_put(np);
> +
>  	/*
>  	 * Since probe() can be deferred we need to make sure that
>  	 * hook_fault_code is not called after __init memory is freed
> --
> 2.38.1