[PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Marc Zyngier]

Geert reports that some drivers do rely on the device driver_data
field containing a pointer to the bridge structure at the point of
initialising the root port, while this has been recently changed to
contain some other data for the benefit of the Apple PCIe driver.

This small series builds on top of Geert previously posted (and
included as a prefix for reference) fix for the Microchip driver,
which breaks the Apple driver. This is basically swapping a regression
for another, which isn't a massive deal at this stage, as the
follow-up patch fixes things for the Apple driver by adding extra
tracking.

Finally, we can revert a one-liner that glued the whole thing
together, and that isn't needed anymore.

All of this is candidate for 6.16, as we have regressed the Microchip
driver in -rc1, and that fixing it breaks the Apple driver.

Geert Uytterhoeven (1):
  PCI: host-generic: Set driver_data before calling gen_pci_init()

Marc Zyngier (2):
  PCI: apple: Add tracking of probed root ports
  Revert "PCI: ecam: Allow cfg->priv to be pre-populated from the root
    port device"

 drivers/pci/controller/pci-host-common.c |  4 +-
 drivers/pci/controller/pcie-apple.c      | 53 ++++++++++++++++++++++--
 drivers/pci/ecam.c                       |  2 -
 3 files changed, 51 insertions(+), 8 deletions(-)

-- 
2.39.2

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

[Marc Zyngier]

From: Geert Uytterhoeven <geert+renesas@glider.be>

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>
Link: https://lore.kernel.org/r/774290708a6f0f683711914fda110742c18a7fb2.1750787223.git.geert+renesas@glider.be
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 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 b0992325dd65f..b370528638471 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.39.2

[PATCH 2/3] PCI: apple: Add tracking of probed root ports

[Marc Zyngier]

The apple driver relies on being able to directly find the matching
root port structure from the platform device that represents this
port.

A previous hack stashed a pointer to the root port structure in
the config window private pointer, but that ended up relying on
assumptions that break other drivers.

Instead, bite the bullet and track the association as part of the
driver itself as a list of probed root ports.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/controller/pcie-apple.c | 53 ++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 77fe739766548..0380d300adca6 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -187,6 +187,7 @@ struct apple_pcie {
 	const struct hw_info	*hw;
 	unsigned long		*bitmap;
 	struct list_head	ports;
+	struct list_head	entry;
 	struct completion	event;
 	struct irq_fwspec	fwspec;
 	u32			nvecs;
@@ -205,6 +206,9 @@ struct apple_pcie_port {
 	int			idx;
 };
 
+static LIST_HEAD(pcie_list);
+static DEFINE_MUTEX(pcie_list_lock);
+
 static void rmw_set(u32 set, void __iomem *addr)
 {
 	writel_relaxed(readl_relaxed(addr) | set, addr);
@@ -720,13 +724,45 @@ static int apple_msi_init(struct apple_pcie *pcie)
 	return 0;
 }
 
+static void apple_pcie_register(struct apple_pcie *pcie)
+{
+	guard(mutex)(&pcie_list_lock);
+
+	list_add_tail(&pcie->entry, &pcie_list);
+}
+
+static void apple_pcie_unregister(struct apple_pcie *pcie)
+{
+	guard(mutex)(&pcie_list_lock);
+
+	list_del(&pcie->entry);
+}
+
+static struct apple_pcie *apple_pcie_lookup(struct device *dev)
+{
+	struct apple_pcie *pcie;
+
+	guard(mutex)(&pcie_list_lock);
+
+	list_for_each_entry(pcie, &pcie_list, entry) {
+		if (pcie->dev == dev)
+			return pcie;
+	}
+
+	return NULL;
+}
+
 static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
 {
 	struct pci_config_window *cfg = pdev->sysdata;
-	struct apple_pcie *pcie = cfg->priv;
+	struct apple_pcie *pcie;
 	struct pci_dev *port_pdev;
 	struct apple_pcie_port *port;
 
+	pcie = apple_pcie_lookup(cfg->parent);
+	if (WARN_ON(!pcie))
+		return NULL;
+
 	/* Find the root port this device is on */
 	port_pdev = pcie_find_root_port(pdev);
 
@@ -806,10 +842,14 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
 
 static int apple_pcie_init(struct pci_config_window *cfg)
 {
-	struct apple_pcie *pcie = cfg->priv;
 	struct device *dev = cfg->parent;
+	struct apple_pcie *pcie;
 	int ret;
 
+	pcie = apple_pcie_lookup(dev);
+	if (WARN_ON(!pcie))
+		return -ENOENT;
+
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = apple_pcie_setup_port(pcie, of_port);
 		if (ret) {
@@ -852,13 +892,18 @@ static int apple_pcie_probe(struct platform_device *pdev)
 
 	mutex_init(&pcie->lock);
 	INIT_LIST_HEAD(&pcie->ports);
-	dev_set_drvdata(dev, pcie);
 
 	ret = apple_msi_init(pcie);
 	if (ret)
 		return ret;
 
-	return pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
+	apple_pcie_register(pcie);
+
+	ret = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
+	if (ret)
+		apple_pcie_unregister(pcie);
+
+	return ret;
 }
 
 static const struct of_device_id apple_pcie_of_match[] = {
-- 
2.39.2

Re: [PATCH 2/3] PCI: apple: Add tracking of probed root ports

[Geert Uytterhoeven]

On Wed, 25 Jun 2025 at 13:18, Marc Zyngier <maz@kernel.org> wrote:
> The apple driver relies on being able to directly find the matching
> root port structure from the platform device that represents this
> port.
>
> A previous hack stashed a pointer to the root port structure in
> the config window private pointer, but that ended up relying on
> assumptions that break other drivers.
>
> Instead, bite the bullet and track the association as part of the
> driver itself as a list of probed root ports.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 3/3] Revert "PCI: ecam: Allow cfg->priv to be pre-populated from the root port device"

[Marc Zyngier]

This reverts commit 4900454b4f819e88e9c57ed93542bf9325d7e161.

Now that nobody relies of cfg->priv containing anything useful before
the .init() callback is used, restore the previous behaviour.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/ecam.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 2c5e6446e00ee..260b7de2dbd57 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -84,8 +84,6 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 			goto err_exit_iomap;
 	}
 
-	cfg->priv = dev_get_drvdata(dev);
-
 	if (ops->init) {
 		err = ops->init(cfg);
 		if (err)
-- 
2.39.2

Re: [PATCH 3/3] Revert "PCI: ecam: Allow cfg->priv to be pre-populated from the root port device"

[Geert Uytterhoeven]

On Wed, 25 Jun 2025 at 13:18, Marc Zyngier <maz@kernel.org> wrote:
> This reverts commit 4900454b4f819e88e9c57ed93542bf9325d7e161.
>
> Now that nobody relies of cfg->priv containing anything useful before
> the .init() callback is used, restore the previous behaviour.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Geert Uytterhoeven]

Hi Marc,

On Wed, 25 Jun 2025 at 13:18, Marc Zyngier <maz@kernel.org> wrote:
> Geert reports that some drivers do rely on the device driver_data
> field containing a pointer to the bridge structure at the point of
> initialising the root port, while this has been recently changed to
> contain some other data for the benefit of the Apple PCIe driver.
>
> This small series builds on top of Geert previously posted (and
> included as a prefix for reference) fix for the Microchip driver,
> which breaks the Apple driver. This is basically swapping a regression
> for another, which isn't a massive deal at this stage, as the
> follow-up patch fixes things for the Apple driver by adding extra
> tracking.
>
> Finally, we can revert a one-liner that glued the whole thing
> together, and that isn't needed anymore.

Thanks, works fine on Icicle.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Bjorn Helgaas]

On Wed, Jun 25, 2025 at 12:18:03PM +0100, Marc Zyngier wrote:
> Geert reports that some drivers do rely on the device driver_data
> field containing a pointer to the bridge structure at the point of
> initialising the root port, while this has been recently changed to
> contain some other data for the benefit of the Apple PCIe driver.
> 
> This small series builds on top of Geert previously posted (and
> included as a prefix for reference) fix for the Microchip driver,
> which breaks the Apple driver. This is basically swapping a regression
> for another, which isn't a massive deal at this stage, as the
> follow-up patch fixes things for the Apple driver by adding extra
> tracking.

Is there a bisection hole between patches 1 and 2?

  1: PCI: host-generic: Set driver_data before calling gen_pci_init()
  2: PCI: apple: Add tracking of probed root ports

If so, would it be practical to avoid the hole by reordering those
patches?

> Finally, we can revert a one-liner that glued the whole thing
> together, and that isn't needed anymore.
> 
> All of this is candidate for 6.16, as we have regressed the Microchip
> driver in -rc1, and that fixing it breaks the Apple driver.
> 
> Geert Uytterhoeven (1):
>   PCI: host-generic: Set driver_data before calling gen_pci_init()
> 
> Marc Zyngier (2):
>   PCI: apple: Add tracking of probed root ports
>   Revert "PCI: ecam: Allow cfg->priv to be pre-populated from the root
>     port device"
> 
>  drivers/pci/controller/pci-host-common.c |  4 +-
>  drivers/pci/controller/pcie-apple.c      | 53 ++++++++++++++++++++++--
>  drivers/pci/ecam.c                       |  2 -
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Marc Zyngier]

On Mon, 30 Jun 2025 18:06:01 +0100,
Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Wed, Jun 25, 2025 at 12:18:03PM +0100, Marc Zyngier wrote:
> > Geert reports that some drivers do rely on the device driver_data
> > field containing a pointer to the bridge structure at the point of
> > initialising the root port, while this has been recently changed to
> > contain some other data for the benefit of the Apple PCIe driver.
> > 
> > This small series builds on top of Geert previously posted (and
> > included as a prefix for reference) fix for the Microchip driver,
> > which breaks the Apple driver. This is basically swapping a regression
> > for another, which isn't a massive deal at this stage, as the
> > follow-up patch fixes things for the Apple driver by adding extra
> > tracking.
> 
> Is there a bisection hole between patches 1 and 2?
> 
>   1: PCI: host-generic: Set driver_data before calling gen_pci_init()
>   2: PCI: apple: Add tracking of probed root ports
> 
> If so, would it be practical to avoid the hole by reordering those
> patches?

Sure, but you said you already had queued patch #1, and what is in
-rc1 already breaks Geert's box. So no matter the order, we break
something at some point.

If you want to only break one thing, then yes, swapping these two
patches is the correct thing to do.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Bjorn Helgaas]

On Mon, Jun 30, 2025 at 06:23:00PM +0100, Marc Zyngier wrote:
> On Mon, 30 Jun 2025 18:06:01 +0100,
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 
> > On Wed, Jun 25, 2025 at 12:18:03PM +0100, Marc Zyngier wrote:
> > > Geert reports that some drivers do rely on the device driver_data
> > > field containing a pointer to the bridge structure at the point of
> > > initialising the root port, while this has been recently changed to
> > > contain some other data for the benefit of the Apple PCIe driver.
> > > 
> > > This small series builds on top of Geert previously posted (and
> > > included as a prefix for reference) fix for the Microchip driver,
> > > which breaks the Apple driver. This is basically swapping a regression
> > > for another, which isn't a massive deal at this stage, as the
> > > follow-up patch fixes things for the Apple driver by adding extra
> > > tracking.
> > 
> > Is there a bisection hole between patches 1 and 2?
> > 
> >   1: PCI: host-generic: Set driver_data before calling gen_pci_init()
> >   2: PCI: apple: Add tracking of probed root ports
> > 
> > If so, would it be practical to avoid the hole by reordering those
> > patches?
> 
> Sure, but you said you already had queued patch #1, and what is in
> -rc1 already breaks Geert's box. So no matter the order, we break
> something at some point.

I did, but when I saw your problem report and subsequent updates, I
put Geert's patch on hold.

> If you want to only break one thing, then yes, swapping these two
> patches is the correct thing to do.

I swapped them and put them back on pci/for-linus for v6.16:

  https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git/log/?h=for-linus&id=ba74278c638d

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[Marc Zyngier]

On Mon, 30 Jun 2025 18:34:15 +0100,
Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Mon, Jun 30, 2025 at 06:23:00PM +0100, Marc Zyngier wrote:
> > On Mon, 30 Jun 2025 18:06:01 +0100,
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > 
> > > On Wed, Jun 25, 2025 at 12:18:03PM +0100, Marc Zyngier wrote:
> > > > Geert reports that some drivers do rely on the device driver_data
> > > > field containing a pointer to the bridge structure at the point of
> > > > initialising the root port, while this has been recently changed to
> > > > contain some other data for the benefit of the Apple PCIe driver.
> > > > 
> > > > This small series builds on top of Geert previously posted (and
> > > > included as a prefix for reference) fix for the Microchip driver,
> > > > which breaks the Apple driver. This is basically swapping a regression
> > > > for another, which isn't a massive deal at this stage, as the
> > > > follow-up patch fixes things for the Apple driver by adding extra
> > > > tracking.
> > > 
> > > Is there a bisection hole between patches 1 and 2?
> > > 
> > >   1: PCI: host-generic: Set driver_data before calling gen_pci_init()
> > >   2: PCI: apple: Add tracking of probed root ports
> > > 
> > > If so, would it be practical to avoid the hole by reordering those
> > > patches?
> > 
> > Sure, but you said you already had queued patch #1, and what is in
> > -rc1 already breaks Geert's box. So no matter the order, we break
> > something at some point.
> 
> I did, but when I saw your problem report and subsequent updates, I
> put Geert's patch on hold.
> 
> > If you want to only break one thing, then yes, swapping these two
> > patches is the correct thing to do.
> 
> I swapped them and put them back on pci/for-linus for v6.16:
> 
>   https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git/log/?h=for-linus&id=ba74278c638d
> 

LGTM, thanks for picking these up!

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] PCI: host-generic: Fix driver_data overwriting bugs

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Bjorn Helgaas <bhelgaas@google.com>:

On Wed, 25 Jun 2025 12:18:03 +0100 you wrote:
> Geert reports that some drivers do rely on the device driver_data
> field containing a pointer to the bridge structure at the point of
> initialising the root port, while this has been recently changed to
> contain some other data for the benefit of the Apple PCIe driver.
> 
> This small series builds on top of Geert previously posted (and
> included as a prefix for reference) fix for the Microchip driver,
> which breaks the Apple driver. This is basically swapping a regression
> for another, which isn't a massive deal at this stage, as the
> follow-up patch fixes things for the Apple driver by adding extra
> tracking.
> 
> [...]

Here is the summary with links:
  - [1/3] PCI: host-generic: Set driver_data before calling gen_pci_init()
    https://git.kernel.org/riscv/c/bdb32a0f6780
  - [2/3] PCI: apple: Add tracking of probed root ports
    https://git.kernel.org/riscv/c/643c0c9d0496
  - [3/3] Revert "PCI: ecam: Allow cfg->priv to be pre-populated from the root port device"
    https://git.kernel.org/riscv/c/ba74278c638d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html