[PATCH] PCI: host-generic: Set driver_data before calling gen_pci_init()

[PATCH] PCI: host-generic: Set driver_data before calling gen_pci_init()

[Geert Uytterhoeven]

On MicroChip MPFS Icicle:

    microchip-pcie 2000000000.pcie: host bridge /soc/pcie@2000000000 ranges:
    microchip-pcie 2000000000.pcie: Parsing ranges property...
    microchip-pcie 2000000000.pcie:      MEM 0x2008000000..0x2087ffffff -> 0x0008000000
    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000368
    Current swapper/0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000814f1000
    [0000000000000368] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
    Oops [#1]
    Modules linked in:
    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-rc1-icicle-00003-gafc0a570bb61 #232 NONE
    Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
    [...]
    [<ffffffff803fb8a4>] plda_pcie_setup_iomems+0xe/0x78
    [<ffffffff803fc246>] mc_platform_init+0x80/0x1d2
    [<ffffffff803f9c88>] pci_ecam_create+0x104/0x1e2
    [<ffffffff8000adbe>] pci_host_common_init+0x120/0x228
    [<ffffffff8000af42>] pci_host_common_probe+0x7c/0x8a

The initialization of driver_data was moved after the call to
gen_pci_init(), while the pci_ecam_ops.init() callback
mc_platform_init() expects it has already been initialized.

Fix this by moving the initialization of driver_data up.

Fixes: afc0a570bb613871 ("PCI: host-generic: Extract an ECAM bridge creation helper from pci_host_common_probe()")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Notes:
  1. Before, driver_data was initialized before calling
     of_pci_check_probe_only(), but the latter doesn't rely on that,
  2. drivers/pci/controller/plda/pcie-microchip-host.c seems to be the
     only driver relying on driver_data being set.
---
 drivers/pci/controller/pci-host-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index b0992325dd65f0da..b37052863847162d 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -64,13 +64,13 @@ int pci_host_common_init(struct platform_device *pdev,
 
 	of_pci_check_probe_only();
 
+	platform_set_drvdata(pdev, bridge);
+
 	/* Parse and map our Configuration Space windows */
 	cfg = gen_pci_init(dev, bridge, ops);
 	if (IS_ERR(cfg))
 		return PTR_ERR(cfg);
 
-	platform_set_drvdata(pdev, bridge);
-
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
 	bridge->enable_device = ops->enable_device;
-- 
2.43.0

Re: [PATCH] PCI: host-generic: Set driver_data before calling gen_pci_init()

[Bjorn Helgaas]

On Tue, Jun 24, 2025 at 07:50:10PM +0200, Geert Uytterhoeven wrote:
> On MicroChip MPFS Icicle:
> 
>     microchip-pcie 2000000000.pcie: host bridge /soc/pcie@2000000000 ranges:
>     microchip-pcie 2000000000.pcie: Parsing ranges property...
>     microchip-pcie 2000000000.pcie:      MEM 0x2008000000..0x2087ffffff -> 0x0008000000
>     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000368
>     Current swapper/0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000814f1000
>     [0000000000000368] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
>     Oops [#1]
>     Modules linked in:
>     CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-rc1-icicle-00003-gafc0a570bb61 #232 NONE
>     Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
>     [...]
>     [<ffffffff803fb8a4>] plda_pcie_setup_iomems+0xe/0x78
>     [<ffffffff803fc246>] mc_platform_init+0x80/0x1d2
>     [<ffffffff803f9c88>] pci_ecam_create+0x104/0x1e2
>     [<ffffffff8000adbe>] pci_host_common_init+0x120/0x228
>     [<ffffffff8000af42>] pci_host_common_probe+0x7c/0x8a
> 
> The initialization of driver_data was moved after the call to
> gen_pci_init(), while the pci_ecam_ops.init() callback
> mc_platform_init() expects it has already been initialized.
> 
> Fix this by moving the initialization of driver_data up.
> 
> Fixes: afc0a570bb613871 ("PCI: host-generic: Extract an ECAM bridge creation helper from pci_host_common_probe()")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied to pci/for-linus for v6.16, since afc0a570bb613871 appeared in
v6.16-rc1.

> ---
> Notes:
>   1. Before, driver_data was initialized before calling
>      of_pci_check_probe_only(), but the latter doesn't rely on that,
>   2. drivers/pci/controller/plda/pcie-microchip-host.c seems to be the
>      only driver relying on driver_data being set.
> ---
>  drivers/pci/controller/pci-host-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index b0992325dd65f0da..b37052863847162d 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -64,13 +64,13 @@ int pci_host_common_init(struct platform_device *pdev,
>  
>  	of_pci_check_probe_only();
>  
> +	platform_set_drvdata(pdev, bridge);
> +
>  	/* Parse and map our Configuration Space windows */
>  	cfg = gen_pci_init(dev, bridge, ops);
>  	if (IS_ERR(cfg))
>  		return PTR_ERR(cfg);
>  
> -	platform_set_drvdata(pdev, bridge);
> -
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
>  	bridge->enable_device = ops->enable_device;
> -- 
> 2.43.0
> 

Re: [PATCH] PCI: host-generic: Set driver_data before calling gen_pci_init()

[Marc Zyngier]

On Tue, 24 Jun 2025 18:50:10 +0100,
Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> On MicroChip MPFS Icicle:
> 
>     microchip-pcie 2000000000.pcie: host bridge /soc/pcie@2000000000 ranges:
>     microchip-pcie 2000000000.pcie: Parsing ranges property...
>     microchip-pcie 2000000000.pcie:      MEM 0x2008000000..0x2087ffffff -> 0x0008000000
>     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000368
>     Current swapper/0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000814f1000
>     [0000000000000368] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
>     Oops [#1]
>     Modules linked in:
>     CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-rc1-icicle-00003-gafc0a570bb61 #232 NONE
>     Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
>     [...]
>     [<ffffffff803fb8a4>] plda_pcie_setup_iomems+0xe/0x78
>     [<ffffffff803fc246>] mc_platform_init+0x80/0x1d2
>     [<ffffffff803f9c88>] pci_ecam_create+0x104/0x1e2
>     [<ffffffff8000adbe>] pci_host_common_init+0x120/0x228
>     [<ffffffff8000af42>] pci_host_common_probe+0x7c/0x8a
> 
> The initialization of driver_data was moved after the call to
> gen_pci_init(), while the pci_ecam_ops.init() callback
> mc_platform_init() expects it has already been initialized.
> 
> Fix this by moving the initialization of driver_data up.
> 
> Fixes: afc0a570bb613871 ("PCI: host-generic: Extract an ECAM bridge creation helper from pci_host_common_probe()")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Notes:
>   1. Before, driver_data was initialized before calling
>      of_pci_check_probe_only(), but the latter doesn't rely on that,
>   2. drivers/pci/controller/plda/pcie-microchip-host.c seems to be the
>      only driver relying on driver_data being set.
> ---
>  drivers/pci/controller/pci-host-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index b0992325dd65f0da..b37052863847162d 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -64,13 +64,13 @@ int pci_host_common_init(struct platform_device *pdev,
>  
>  	of_pci_check_probe_only();
>  
> +	platform_set_drvdata(pdev, bridge);
> +
>  	/* Parse and map our Configuration Space windows */
>  	cfg = gen_pci_init(dev, bridge, ops);
>  	if (IS_ERR(cfg))
>  		return PTR_ERR(cfg);
>  
> -	platform_set_drvdata(pdev, bridge);
> -
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
>  	bridge->enable_device = ops->enable_device;

This is going to break what was introduced in 4900454b4f819 ("PCI:
ecam: Allow cfg->priv to be pre-populated from the root port device").

The Apple PCIe driver uses drvdata to pass a pointer to its root port
structure from its probe routine to the .init ECAM callback. If we
overwrite drvdata with the bridge *before* calling pci_ecam_create(),
this data is lost, and drama follows.

Obviously, multi-purpose private fields are not great...

If we are going down the road of going back to the previous situation,
two things need to be done:

- the Apple PCIe driver needs to grow some form of root port
  registration in order to map a 'struct device' back to the pcie
  tracking structure

- 4900454b4f819 needs to be reverted, because it obviously doesn't
  serve any purpose anymore.

I'll look into the first point.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.