Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH 0/3] PCI: Remove device links to Generic PHY
@ 2026-09-12 16:14 vladimir.oltean
  2026-09-12 16:14 ` [PATCH 1/3] PCI: cadence: Remove device links to PHY vladimir.oltean
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: vladimir.oltean @ 2026-09-12 16:14 UTC (permalink / raw)
  To: linux-pci
  Cc: Aksh Garg, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
	Vignesh Raghavendra, Siddharth Vadapalli, Manikandan K Pillai,
	Felix Gu, linux-kernel, linux-omap, linux-arm-kernel

This is a request from Bjorn Helgaas to replace commit 9a9d18547ddc
("PCI: Remove device links to PHY") from the pci/next branch with 3
distinct changes, per PCI controller, to handle conflicts with bug fixes
in the same area on dra7xx in a better way:
https://lore.kernel.org/linux-pci/20260911221209.GA587622@bhelgaas/

The patches are formatted against the v7.3-rc1 tag. They do *not* apply
to the current pci/next, as the change is already present there.

Previous submission was:
https://lore.kernel.org/linux-pci/20260810124315.433448-1-vladimir.oltean@nxp.com/

There is no other change than splitting up into individual commits and
adjusting each one's commit message. All review/ack tags were dropped in
that process.

Vladimir Oltean (3):
  PCI: cadence: Remove device links to PHY
  PCI: dra7xx: Remove device links to PHY
  PCI: keystone: Remove device links to PHY

 .../controller/cadence/pcie-cadence-plat.c    |  4 ---
 drivers/pci/controller/cadence/pcie-cadence.c | 16 +---------
 drivers/pci/controller/cadence/pcie-cadence.h |  2 --
 drivers/pci/controller/dwc/pci-dra7xx.c       | 16 ----------
 drivers/pci/controller/dwc/pci-keystone.c     | 31 +++----------------
 5 files changed, 5 insertions(+), 64 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] PCI: cadence: Remove device links to PHY
  2026-09-12 16:14 [PATCH 0/3] PCI: Remove device links to Generic PHY vladimir.oltean
@ 2026-09-12 16:14 ` vladimir.oltean
  2026-09-12 16:24   ` sashiko-bot
  2026-09-12 16:14 ` [PATCH 2/3] PCI: dra7xx: " vladimir.oltean
  2026-09-12 16:14 ` [PATCH 3/3] PCI: keystone: " vladimir.oltean
  2 siblings, 1 reply; 8+ messages in thread
From: vladimir.oltean @ 2026-09-12 16:14 UTC (permalink / raw)
  To: linux-pci
  Cc: Aksh Garg, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
	Vignesh Raghavendra, Siddharth Vadapalli, Manikandan K Pillai,
	Felix Gu, linux-kernel, linux-omap, linux-arm-kernel

This is a partial revert of the device link pieces from commit
dfb80534692d ("PCI: cadence: Add generic PHY support to host and EP
drivers").

The trouble with this is that a PHY consumer driver dereferences fields
from struct phy, which will become no longer possible.

Since commit 987351e1ea77 ("phy: core: Add consumer device link
support") from 2019, the PHY core also adds a device link to order
PHY provider and consumer suspend/resume operations. The reverted
functionality is from 2018, and is redundant with the PHY core now.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../pci/controller/cadence/pcie-cadence-plat.c   |  4 ----
 drivers/pci/controller/cadence/pcie-cadence.c    | 16 +---------------
 drivers/pci/controller/cadence/pcie-cadence.h    |  2 --
 3 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
index a1ea24fc3b63..5900d68c6e83 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
@@ -41,7 +41,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
 	struct pci_host_bridge *bridge;
 	struct cdns_pcie_ep *ep;
 	struct cdns_pcie_rc *rc;
-	int phy_count;
 	bool is_rc;
 	int ret;
 
@@ -122,9 +121,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 	cdns_pcie_disable_phy(cdns_plat_pcie->pcie);
-	phy_count = cdns_plat_pcie->pcie->phy_count;
-	while (phy_count--)
-		device_link_del(cdns_plat_pcie->pcie->link[phy_count]);
 
 	return 0;
 }
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index a1eada56edba..0ac980249941 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -222,7 +222,6 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 	struct device_node *np = dev->of_node;
 	int phy_count;
 	struct phy **phy;
-	struct device_link **link;
 	int i;
 	int ret;
 	const char *name;
@@ -238,10 +237,6 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 	if (!phy)
 		return -ENOMEM;
 
-	link = devm_kcalloc(dev, phy_count, sizeof(*link), GFP_KERNEL);
-	if (!link)
-		return -ENOMEM;
-
 	for (i = 0; i < phy_count; i++) {
 		of_property_read_string_index(np, "phy-names", i, &name);
 		phy[i] = devm_phy_get(dev, name);
@@ -249,17 +244,10 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 			ret = PTR_ERR(phy[i]);
 			goto err_phy;
 		}
-		link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
-		if (!link[i]) {
-			devm_phy_put(dev, phy[i]);
-			ret = -EINVAL;
-			goto err_phy;
-		}
 	}
 
 	pcie->phy_count = phy_count;
 	pcie->phy = phy;
-	pcie->link = link;
 
 	ret =  cdns_pcie_enable_phy(pcie);
 	if (ret)
@@ -268,10 +256,8 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 	return 0;
 
 err_phy:
-	while (--i >= 0) {
-		device_link_del(link[i]);
+	while (--i >= 0)
 		devm_phy_put(dev, phy[i]);
-	}
 
 	return ret;
 }
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index b4d51adefea5..4a96e1869e53 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -260,7 +260,6 @@ struct cdns_plat_pcie_of_data {
  * @is_hpa: indicates if the architecture is HPA
  * @phy_count: number of supported PHY devices
  * @phy: list of pointers to specific PHY control blocks
- * @link: list of pointers to corresponding device link representations
  * @ops: Platform-specific ops to control various inputs from Cadence PCIe
  *       wrapper
  * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
@@ -276,7 +275,6 @@ struct cdns_pcie {
 	bool				     is_hpa;
 	int			             phy_count;
 	struct phy		             **phy;
-	struct device_link	             **link;
 	const  struct cdns_pcie_ops          *ops;
 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
 	int				     max_link_speed;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] PCI: dra7xx: Remove device links to PHY
  2026-09-12 16:14 [PATCH 0/3] PCI: Remove device links to Generic PHY vladimir.oltean
  2026-09-12 16:14 ` [PATCH 1/3] PCI: cadence: Remove device links to PHY vladimir.oltean
@ 2026-09-12 16:14 ` vladimir.oltean
  2026-09-12 16:25   ` sashiko-bot
  2026-09-12 16:14 ` [PATCH 3/3] PCI: keystone: " vladimir.oltean
  2 siblings, 1 reply; 8+ messages in thread
From: vladimir.oltean @ 2026-09-12 16:14 UTC (permalink / raw)
  To: linux-pci
  Cc: Aksh Garg, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
	Vignesh Raghavendra, Siddharth Vadapalli, Manikandan K Pillai,
	Felix Gu, linux-kernel, linux-omap, linux-arm-kernel

This is a full revert of commit 7a4db656a635 ("PCI: dra7xx: Create
functional dependency between PCIe and PHY").

The trouble with this is that a PHY consumer driver dereferences fields
from struct phy, which will become no longer possible.

Since commit 987351e1ea77 ("phy: core: Add consumer device link
support") from 2019, the PHY core also adds a device link to order
PHY provider and consumer suspend/resume operations. The reverted
functionality is from 2017, and is redundant with the PHY core now.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/pci/controller/dwc/pci-dra7xx.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index 6ae5b27e27b3..6c9e88177600 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -9,7 +9,6 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
-#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -680,7 +679,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
 	int i;
 	int phy_count;
 	struct phy **phy;
-	struct device_link **link;
 	void __iomem *base;
 	struct dw_pcie *pci;
 	struct dra7xx_pcie *dra7xx;
@@ -728,10 +726,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
 	if (!phy)
 		return -ENOMEM;
 
-	link = devm_kcalloc(dev, phy_count, sizeof(*link), GFP_KERNEL);
-	if (!link)
-		return -ENOMEM;
-
 	dra7xx->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(dra7xx->clk))
 		return dev_err_probe(dev, PTR_ERR(dra7xx->clk),
@@ -746,12 +740,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
 		phy[i] = devm_phy_get(dev, name);
 		if (IS_ERR(phy[i]))
 			return PTR_ERR(phy[i]);
-
-		link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
-		if (!link[i]) {
-			ret = -EINVAL;
-			goto err_link;
-		}
 	}
 
 	dra7xx->base = base;
@@ -853,10 +841,6 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 	dra7xx_pcie_disable_phy(dra7xx);
 
-err_link:
-	while (--i >= 0)
-		device_link_del(link[i]);
-
 	return ret;
 }
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] PCI: keystone: Remove device links to PHY
  2026-09-12 16:14 [PATCH 0/3] PCI: Remove device links to Generic PHY vladimir.oltean
  2026-09-12 16:14 ` [PATCH 1/3] PCI: cadence: Remove device links to PHY vladimir.oltean
  2026-09-12 16:14 ` [PATCH 2/3] PCI: dra7xx: " vladimir.oltean
@ 2026-09-12 16:14 ` vladimir.oltean
  2026-09-12 16:27   ` sashiko-bot
  2 siblings, 1 reply; 8+ messages in thread
From: vladimir.oltean @ 2026-09-12 16:14 UTC (permalink / raw)
  To: linux-pci
  Cc: Aksh Garg, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
	Vignesh Raghavendra, Siddharth Vadapalli, Manikandan K Pillai,
	Felix Gu, linux-kernel, linux-omap, linux-arm-kernel

This is a partial revert of the device link pieces from commit
49229238ab47 ("PCI: keystone: Cleanup PHY handling").

The trouble with this is that a PHY consumer driver dereferences fields
from struct phy, which will become no longer possible.

Since commit 987351e1ea77 ("phy: core: Add consumer device link
support") from 2019, the PHY core also adds a device link to order
PHY provider and consumer suspend/resume operations. The reverted
functionality is from 2018, and is redundant with the PHY core now.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 31 +++--------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 602516239a57..bd736a1624bc 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -129,7 +129,6 @@ struct keystone_pcie {
 	int			num_lanes;
 	u32			num_viewport;
 	struct phy		**phy;
-	struct device_link	**link;
 	struct			device_node *msi_intc_np;
 	struct irq_domain	*intx_irq_domain;
 	struct device_node	*np;
@@ -1131,7 +1130,6 @@ static int ks_pcie_probe(struct platform_device *pdev)
 	enum dw_pcie_device_mode mode;
 	struct dw_pcie *pci;
 	struct keystone_pcie *ks_pcie;
-	struct device_link **link;
 	struct gpio_desc *gpiod;
 	struct resource *res;
 	void __iomem *base;
@@ -1202,31 +1200,17 @@ static int ks_pcie_probe(struct platform_device *pdev)
 	if (!phy)
 		return -ENOMEM;
 
-	link = devm_kcalloc(dev, num_lanes, sizeof(*link), GFP_KERNEL);
-	if (!link)
-		return -ENOMEM;
-
 	for (i = 0; i < num_lanes; i++) {
 		snprintf(name, sizeof(name), "pcie-phy%d", i);
 		phy[i] = devm_phy_optional_get(dev, name);
 		if (IS_ERR(phy[i])) {
 			ret = PTR_ERR(phy[i]);
-			goto err_link;
-		}
-
-		if (!phy[i])
-			continue;
-
-		link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
-		if (!link[i]) {
-			ret = -EINVAL;
-			goto err_link;
+			goto err;
 		}
 	}
 
 	ks_pcie->np = np;
 	ks_pcie->pci = pci;
-	ks_pcie->link = link;
 	ks_pcie->num_lanes = num_lanes;
 	ks_pcie->phy = phy;
 
@@ -1236,7 +1220,7 @@ static int ks_pcie_probe(struct platform_device *pdev)
 		ret = PTR_ERR(gpiod);
 		if (ret != -EPROBE_DEFER)
 			dev_err(dev, "Failed to get reset GPIO\n");
-		goto err_link;
+		goto err;
 	}
 
 	/* Obtain references to the PHYs */
@@ -1251,7 +1235,7 @@ static int ks_pcie_probe(struct platform_device *pdev)
 
 	if (ret) {
 		dev_err(dev, "failed to enable phy\n");
-		goto err_link;
+		goto err;
 	}
 
 	platform_set_drvdata(pdev, ks_pcie);
@@ -1338,25 +1322,18 @@ static int ks_pcie_probe(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 	ks_pcie_disable_phy(ks_pcie);
 
-err_link:
-	while (--i >= 0 && link[i])
-		device_link_del(link[i]);
-
+err:
 	return ret;
 }
 
 static void ks_pcie_remove(struct platform_device *pdev)
 {
 	struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
-	struct device_link **link = ks_pcie->link;
-	int num_lanes = ks_pcie->num_lanes;
 	struct device *dev = &pdev->dev;
 
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 	ks_pcie_disable_phy(ks_pcie);
-	while (num_lanes--)
-		device_link_del(link[num_lanes]);
 }
 
 static struct platform_driver ks_pcie_driver = {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] PCI: cadence: Remove device links to PHY
  2026-09-12 16:14 ` [PATCH 1/3] PCI: cadence: Remove device links to PHY vladimir.oltean
@ 2026-09-12 16:24   ` sashiko-bot
  2026-09-12 16:27     ` Vladimir Oltean
  0 siblings, 1 reply; 8+ messages in thread
From: sashiko-bot @ 2026-09-12 16:24 UTC (permalink / raw)
  To: vladimir.oltean; +Cc: linux-pci

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The probe function `cdns_plat_pcie_probe` unconditionally returns 0 on failure paths, masking initialization errors from the driver core.
- [High] Shutdown and PM callbacks erroneously cast `struct cdns_plat_pcie *` to `struct cdns_pcie *`, causing severe out-of-bounds memory accesses.
--

commit 93f901fd65f6099ca141c0baa86f19be3649b0d0
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

PCI: cadence: Remove device links to PHY

This patch partially reverts device link pieces from a previous commit
because a PHY consumer driver dereferences fields from struct phy, which
will become impossible. It relies on the PHY core's own device links to
order provider and consumer suspend/resume operations.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
> index a1ea24fc3b63f..5900d68c6e83a 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
[ ... ]
> @@ -122,9 +121,6 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);
>  	cdns_pcie_disable_phy(cdns_plat_pcie->pcie);
> -	phy_count = cdns_plat_pcie->pcie->phy_count;
> -	while (phy_count--)
> -		device_link_del(cdns_plat_pcie->pcie->link[phy_count]);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path intentionally return 0?

If a setup failure occurs during probe (such as cdns_pcie_host_setup()
failing), the code jumps to this error path, performs partial cleanup,
but explicitly returns 0 instead of propagating the error code.

Since the driver core sees a successful probe, it will keep the device
bound. Will subsequent accesses to the device crash the kernel because
the internal structures and resources are partially freed or disabled?

[Severity: High]
This is another pre-existing issue, but could the platform drvdata setup
cause severe out-of-bounds memory accesses during power management?

The cdns_plat_pcie_probe() function allocates an 8-byte struct cdns_plat_pcie
and assigns it as the platform drvdata. However, cdns_plat_pcie_shutdown(),
cdns_pcie_suspend_noirq(), and cdns_pcie_resume_noirq() retrieve the
drvdata and cast it to the much larger struct cdns_pcie *:

cdns_plat_pcie_shutdown() {
    ...
    struct cdns_pcie *pcie = dev_get_drvdata(dev);
    ...
    cdns_pcie_disable_phy(pcie);
}

Does accessing fields at large offsets, like pcie->phy_count inside
cdns_pcie_disable_phy(), read memory completely out of bounds and
potentially cause an immediate kernel panic during suspend, resume,
or shutdown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912161440.1032188-1-vladimir.oltean@nxp.com?part=1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] PCI: dra7xx: Remove device links to PHY
  2026-09-12 16:14 ` [PATCH 2/3] PCI: dra7xx: " vladimir.oltean
@ 2026-09-12 16:25   ` sashiko-bot
  0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-09-12 16:25 UTC (permalink / raw)
  To: vladimir.oltean; +Cc: linux-pci

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Shared IRQ handler accesses hardware registers without checking if the device is runtime active.
--

commit 5052482ced4c68a6a0a92320a0f14365dce06957
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

PCI: dra7xx: Remove device links to PHY

This is a full revert of commit 7a4db656a635 ("PCI: dra7xx: Create
functional dependency between PCIe and PHY"). The reverted functionality
is redundant since the PHY core now adds a device link to order PHY
provider and consumer suspend/resume operations.

> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index 6ae5b27e27b32..6c9e881776002 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the shared IRQ handler access hardware
registers without checking if the device is runtime active?

If another device sharing the same IRQ line triggers an interrupt during or
after shutdown, the handler unconditionally accesses the powered-off registers:

drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_irq_handler() {
	...
	reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);
	...
}

In the driver's shutdown callback, the device is powered off
(pm_runtime_put_sync, clk_disable_unprepare) but the devm-managed IRQ is never
freed.

Could this lead to a kernel panic (precise data abort on ARM) due to reading
from unclocked hardware registers, or spurious interrupt storms? Should the
handler use pm_runtime_get_if_active() before accessing registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912161440.1032188-1-vladimir.oltean@nxp.com?part=2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] PCI: cadence: Remove device links to PHY
  2026-09-12 16:24   ` sashiko-bot
@ 2026-09-12 16:27     ` Vladimir Oltean
  0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2026-09-12 16:27 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: linux-pci, helgaas

On Sat, Sep 12, 2026 at 04:24:52PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The probe function `cdns_plat_pcie_probe` unconditionally returns 0 on failure paths, masking initialization errors from the driver core.
> - [High] Shutdown and PM callbacks erroneously cast `struct cdns_plat_pcie *` to `struct cdns_pcie *`, causing severe out-of-bounds memory accesses.
> --

There is no way I'm touching the pre-existing issues, sorry.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] PCI: keystone: Remove device links to PHY
  2026-09-12 16:14 ` [PATCH 3/3] PCI: keystone: " vladimir.oltean
@ 2026-09-12 16:27   ` sashiko-bot
  0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-09-12 16:27 UTC (permalink / raw)
  To: vladimir.oltean; +Cc: linux-pci

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Asynchronous `pm_runtime_put()` immediately followed by `pm_runtime_disable()` risks canceling pending suspend work, leaving hardware in an incorrect power state.
- [High] Missing cleanup of DWC host and endpoint structures in `ks_pcie_remove()` leads to a Use-After-Free vulnerability.
- [High] Hardware registers are unconditionally accessed in a shared IRQ handler without checking runtime PM state, risking a kernel panic.
- [High] Early registration of a shared IRQ before state structures are fully initialized introduces a race condition leading to a NULL pointer dereference.
--

commit cee8876507fd3272f25089c961cbca85b16e29eb
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

PCI: keystone: Remove device links to PHY

This commit partially reverts earlier device link handling to clean up
PHY provider and consumer suspend/resume operations. The PHY core now
handles the device link addition natively, making the local implementation
redundant.

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c

[ ... ]

> @@ -1338,25 +1322,18 @@ static int ks_pcie_probe(struct platform_device *pdev)
>  	pm_runtime_disable(dev);
>  	ks_pcie_disable_phy(ks_pcie);
>  
> -err_link:
> -	while (--i >= 0 && link[i])
> -		device_link_del(link[i]);
> -
> +err:
>  	return ret;
>  }
>  
>  static void ks_pcie_remove(struct platform_device *pdev)
>  {
>  	struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
> -	struct device_link **link = ks_pcie->link;
> -	int num_lanes = ks_pcie->num_lanes;
>  	struct device *dev = &pdev->dev;
>  
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);

[Severity: High]
This is a pre-existing issue, but does using the asynchronous
pm_runtime_put() right before pm_runtime_disable() risk leaving the
hardware active?

According to PM subsystem guidelines, pm_runtime_put() queues asynchronous
suspend work which is immediately flushed and canceled by
pm_runtime_disable(). Should this use the synchronous pm_runtime_put_sync()
instead to ensure the device is properly suspended in both this remove path
and the probe error path?

[Severity: High]
This is also a pre-existing issue, but does ks_pcie_remove() miss the
required DWC core cleanup routines?

It looks like dw_pcie_host_init() or dw_pcie_ep_init() are called during
probe, but there are no symmetrical calls to dw_pcie_host_deinit() or
dw_pcie_ep_deinit() here. Since the struct dw_pcie is allocated with
devm_kzalloc(), wouldn't the active PCI core retain dangling references
after unbinding, leading to a Use-After-Free?

>  	ks_pcie_disable_phy(ks_pcie);
> -	while (num_lanes--)
> -		device_link_del(link[num_lanes]);
>  }

While reviewing the PM handling in this driver, I noticed a couple of
other pre-existing issues regarding the error IRQ handler.

[Severity: High]
This is a pre-existing issue, but could this early IRQ registration lead to
a NULL pointer dereference?

In ks_pcie_probe(), the shared IRQ is requested before ks_pcie->pci is
assigned:

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_probe() {
	...
	ret = devm_request_irq(dev, irq, ks_pcie_err_irq_handler, IRQF_SHARED,
			       "ks-pcie-error-irq", ks_pcie);
	...
	ks_pcie->pci = pci;
	...
}

If an interrupt from another device on the same shared line fires
immediately, the handler will run and dereference the uninitialized pointer:

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_handle_error_irq() {
	struct device *dev = ks_pcie->pci->dev;
	...
}

[Severity: High]
This is another pre-existing issue, but can the shared error IRQ handler
safely read hardware registers without checking the runtime PM state?

If another device sharing the IRQ line fires an interrupt while the
keystone controller is suspended and its clocks are gated, will reading
ERR_IRQ_STATUS cause a kernel panic (synchronous external abort) or return
invalid states?

drivers/pci/controller/dwc/pci-keystone.c:ks_pcie_handle_error_irq() {
	...
	reg = ks_pcie_app_readl(ks_pcie, ERR_IRQ_STATUS);
	...
}

Should this use pm_runtime_get_if_active() to ensure the device is clocked
before accessing hardware registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912161440.1032188-1-vladimir.oltean@nxp.com?part=3

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-09-12 16:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-12 16:14 [PATCH 0/3] PCI: Remove device links to Generic PHY vladimir.oltean
2026-09-12 16:14 ` [PATCH 1/3] PCI: cadence: Remove device links to PHY vladimir.oltean
2026-09-12 16:24   ` sashiko-bot
2026-09-12 16:27     ` Vladimir Oltean
2026-09-12 16:14 ` [PATCH 2/3] PCI: dra7xx: " vladimir.oltean
2026-09-12 16:25   ` sashiko-bot
2026-09-12 16:14 ` [PATCH 3/3] PCI: keystone: " vladimir.oltean
2026-09-12 16:27   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox