* [PATCH v6 1/9] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:48 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase Claudiu Beznea
` (7 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, stable, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
The RZ/G3S PCIe hardware manual specifies that the clocks must be enabled
before the reset signals are deasserted during initialization.
Follow this sequence in the probe(), suspend(), and resume() paths to
match the hardware requirements and avoid potential issues.
Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
Changes in v4:
- none
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 40 +++++++++++++-----------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 077cfb0834b3..e1105b1f2652 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1888,10 +1888,6 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
if (ret)
goto sysc_signal_restore;
- ret = rzg3s_pcie_power_resets_deassert(host);
- if (ret)
- goto sysc_signal_restore;
-
pm_runtime_enable(dev);
/*
@@ -1902,12 +1898,16 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
if (ret)
goto rpm_disable;
+ ret = rzg3s_pcie_power_resets_deassert(host);
+ if (ret)
+ goto rpm_put;
+
raw_spin_lock_init(&host->hw_lock);
ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_init_irqdomain,
rzg3s_pcie_teardown_irqdomain);
if (ret)
- goto rpm_put;
+ goto power_resets_assert;
bridge->sysdata = host;
bridge->ops = &rzg3s_pcie_root_ops;
@@ -1922,12 +1922,13 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
clk_disable_unprepare(host->port.refclk);
rzg3s_pcie_teardown_irqdomain(host);
host->data->config_deinit(host);
+power_resets_assert:
+ reset_control_bulk_assert(host->data->num_power_resets,
+ host->power_resets);
rpm_put:
pm_runtime_put_sync(dev);
rpm_disable:
pm_runtime_disable(dev);
- reset_control_bulk_assert(host->data->num_power_resets,
- host->power_resets);
sysc_signal_restore:
/*
* SYSC RST_RSM_B signal need to be asserted before turning off the
@@ -1948,10 +1949,6 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
struct rzg3s_sysc *sysc = host->sysc;
int ret;
- ret = pm_runtime_put_sync(dev);
- if (ret)
- return ret;
-
clk_disable_unprepare(port->refclk);
/* SoC-specific de-initialization */
@@ -1964,13 +1961,19 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
if (ret)
goto config_reinit;
- ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+ ret = pm_runtime_put_sync(dev);
if (ret)
goto power_resets_restore;
+ ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+ if (ret)
+ goto rpm_resume;
+
return 0;
/* Restore the previous state if any error happens */
+rpm_resume:
+ pm_runtime_resume_and_get(dev);
power_resets_restore:
reset_control_bulk_deassert(data->num_power_resets,
host->power_resets);
@@ -1980,7 +1983,6 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
data->config_post_init(host);
refclk_restore:
clk_prepare_enable(port->refclk);
- pm_runtime_resume_and_get(dev);
return ret;
}
@@ -2009,18 +2011,18 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
goto assert_rst_rsm_b;
}
- ret = rzg3s_pcie_power_resets_deassert(host);
+ ret = pm_runtime_resume_and_get(dev);
if (ret)
goto assert_rst_rsm_b;
- ret = pm_runtime_resume_and_get(dev);
+ ret = rzg3s_pcie_power_resets_deassert(host);
if (ret)
- goto assert_power_resets;
+ goto rpm_put;
ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
rzg3s_pcie_msi_hw_teardown);
if (ret)
- goto rpm_put;
+ goto assert_power_resets;
return 0;
@@ -2028,11 +2030,11 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
* If any error happens there is no way to recover the IP. Put it in the
* lowest possible power state.
*/
-rpm_put:
- pm_runtime_put_sync(dev);
assert_power_resets:
reset_control_bulk_assert(data->num_power_resets,
host->power_resets);
+rpm_put:
+ pm_runtime_put_sync(dev);
assert_rst_rsm_b:
rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v6 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
2026-10-07 5:36 ` [PATCH v6 1/9] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:53 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 3/9] PCI: rzg3s-host: Drop nop instructions Claudiu Beznea
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, stable, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
The runtime PM documentation states the following:
- During system suspend, pm_runtime_get_noresume() is called for every
device right before executing the subsystem-level .prepare() callback
(in device_prepare()). In addition, the PM core disables runtime PM for
every device right before executing the subsystem-level .suspend_late()
callback (in device_suspend_late()).
- During system resume, pm_runtime_enable() is called for every device
right after executing the subsystem-level .resume_early() callback (in
device_resume_early()), and pm_runtime_put() is called right after
executing the subsystem-level .complete() callback (in
device_complete()).
The driver's .suspend_noirq() callback is invoked after .suspend_late(),
while .resume_noirq() is invoked before .resume_early().
If:
- the device is not part of the wake-up path, and
- its runtime PM status is not RPM_SUSPENDED,
the generic power domain .suspend_noirq()/.resume_noirq() callbacks
(genpd_suspend_noirq()/genpd_resume_noirq()) invoke the driver's
.suspend_noirq()/.resume_noirq() callbacks and call
genpd_stop_dev()/genpd_start_dev() before and after them, respectively.
Calling genpd_stop_dev()/genpd_start_dev() allows devices whose power is
controlled by generic power domains to be powered off and on during
system suspend and resume, even though their runtime PM usage count does
not reach zero.
Since the runtime PM usage count is incremented in device_prepare() and
decremented in device_complete(), runtime PM operations performed from
the driver's .suspend_noirq()/.resume_noirq() callbacks are no-ops. The
actual power transitions are handled by the generic power domain
.suspend_noirq()/.resume_noirq() callbacks.
Moreover, attempting to runtime resume a device while runtime PM is
disabled may return -EACCES. This may cause system resume to fail when
resuming after a failed Root Port reset, as described in a subsequent
patch adding hot-plug support.
Remove the runtime PM calls from the driver's
.suspend_noirq()/.resume_noirq() callbacks and rely on the generic power
domain callbacks to power the device off and on.
Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
Changes in v4:
- simplified the comments in rzg3s_pcie_suspend_noirq()/rzg3s_pcie_resume_noirq()
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index e1105b1f2652..0ef49bb5ab1a 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1961,19 +1961,18 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
if (ret)
goto config_reinit;
- ret = pm_runtime_put_sync(dev);
- if (ret)
- goto power_resets_restore;
+ /*
+ * Since the power domain's genpd_suspend_noirq() will disable clocks,
+ * there is no need to manually invoke runtime PM API here.
+ */
ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
if (ret)
- goto rpm_resume;
+ goto power_resets_restore;
return 0;
/* Restore the previous state if any error happens */
-rpm_resume:
- pm_runtime_resume_and_get(dev);
power_resets_restore:
reset_control_bulk_deassert(data->num_power_resets,
host->power_resets);
@@ -2011,13 +2010,14 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
goto assert_rst_rsm_b;
}
- ret = pm_runtime_resume_and_get(dev);
- if (ret)
- goto assert_rst_rsm_b;
+ /*
+ * Since the power domain's genpd_resume_noirq() will enable clocks,
+ * there is no need to manually invoke runtime PM API here.
+ */
ret = rzg3s_pcie_power_resets_deassert(host);
if (ret)
- goto rpm_put;
+ goto assert_rst_rsm_b;
ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
rzg3s_pcie_msi_hw_teardown);
@@ -2033,8 +2033,6 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
assert_power_resets:
reset_control_bulk_assert(data->num_power_resets,
host->power_resets);
-rpm_put:
- pm_runtime_put_sync(dev);
assert_rst_rsm_b:
rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase
2026-10-07 5:36 ` [PATCH v6 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase Claudiu Beznea
@ 2026-10-07 5:53 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:53 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: wsa+renesas, linux-pci
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The runtime PM documentation states the following:
> - During system suspend, pm_runtime_get_noresume() is called for every
> device right before executing the subsystem-level .prepare() callback
> (in device_prepare()). In addition, the PM core disables runtime PM for
> every device right before executing the subsystem-level .suspend_late()
> callback (in device_suspend_late()).
> - During system resume, pm_runtime_enable() is called for every device
> right after executing the subsystem-level .resume_early() callback (in
> device_resume_early()), and pm_runtime_put() is called right after
> executing the subsystem-level .complete() callback (in
> device_complete()).
>
> The driver's .suspend_noirq() callback is invoked after .suspend_late(),
> [ ... ]
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 3/9] PCI: rzg3s-host: Drop nop instructions
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
2026-10-07 5:36 ` [PATCH v6 1/9] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order Claudiu Beznea
2026-10-07 5:36 ` [PATCH v6 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:46 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 4/9] PCI: rzg3s-host: Move host configuration code together Claudiu Beznea
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
On the RZ/G3S PCIe IP variant (which is similar to those used on the
RZ/G3E, RZ/V2H, and RZ/V2N SoCs), access to the PCIe Type 1 registers
requires setting the PCI_PERM.CFG_HWINIT_EN bit. Since this bit is not
set on the code paths where rzg3s_pcie_set_max_link_speed() is called,
the writes to PCI_EXP_LNKCTL2.TLS are nops. Drop them.
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
Changes in v4:
- none
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 0ef49bb5ab1a..362aa923978f 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1144,11 +1144,6 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
remote_supported_link_speeds != max_supported_link_speeds)
return 0;
- /* Set target Link speed */
- rzg3s_pcie_update_bits(host->pcie, pcie_cap + PCI_EXP_LNKCTL2,
- PCI_EXP_LNKCTL2_TLS,
- FIELD_PREP(PCI_EXP_LNKCTL2_TLS, link_speed));
-
/* Request link speed change */
rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PCCTRL2,
RZG3S_PCI_PCCTRL2_LS_CHG_REQ |
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 3/9] PCI: rzg3s-host: Drop nop instructions
2026-10-07 5:36 ` [PATCH v6 3/9] PCI: rzg3s-host: Drop nop instructions Claudiu Beznea
@ 2026-10-07 5:46 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:46 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: wsa+renesas, linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: rzg3s: dropped LNKCTL2 write leaves TLS out of sync and breaks retraining
--
commit 82580733205489082c60a7965e3e79b44492b4e4
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
PCI: rzg3s-host: Drop nop instructions
On the RZ/G3S PCIe IP variant, access to the PCIe Type 1 registers requires
setting the PCI_PERM.CFG_HWINIT_EN bit. Since this bit is not set on the
code paths where rzg3s_pcie_set_max_link_speed() is called, the writes to
PCI_EXP_LNKCTL2.TLS are nops and are dropped by this commit.
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 0ef49bb5ab1aa..362aa923978f7 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -1144,11 +1144,6 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
> remote_supported_link_speeds != max_supported_link_speeds)
> return 0;
>
> - /* Set target Link speed */
> - rzg3s_pcie_update_bits(host->pcie, pcie_cap + PCI_EXP_LNKCTL2,
> - PCI_EXP_LNKCTL2_TLS,
> - FIELD_PREP(PCI_EXP_LNKCTL2_TLS, link_speed));
> -
[Severity: Medium]
Does dropping this LNKCTL2 update leave the standard capability register out
of sync with the hardware link state configured via RZG3S_PCI_PCCTRL2?
If LNKCTL2 is not updated, lspci and generic bandwidth control logic might
read incorrect values.
Furthermore, if LNKCTL2 and potentially other standard capability registers
are locked behind CFG_HWINIT_EN, does this mean that standard PCI core
functions (like pcie_set_target_speed() or ASPM configuration) will silently
fail when the PCI core attempts to write to the Root Port's LNKCTL2 via
pci_generic_config_write?
drivers/pci/controller/pcie-rzg3s-host.c:
static struct pci_ops rzg3s_pcie_root_ops = {
.read = pci_generic_config_read,
.write = pci_generic_config_write,
.map_bus = rzg3s_pcie_root_map_bus,
};
Should the driver temporarily assert CFG_HWINIT_EN to perform this update and
provide a custom config write wrapper instead of dropping the write entirely?
> /* Request link speed change */
> rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PCCTRL2,
> RZG3S_PCI_PCCTRL2_LS_CHG_REQ |
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 4/9] PCI: rzg3s-host: Move host configuration code together
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (2 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 3/9] PCI: rzg3s-host: Drop nop instructions Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:47 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 5/9] PCI: rzg3s-host: Move suspend/resume code into dedicated functions Claudiu Beznea
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Move host configuration code together to have it grouped. This prepares for
the addition of hotplug support.
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
Changes in v4:
- none
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 230 +++++++++++------------
1 file changed, 115 insertions(+), 115 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 362aa923978f..3ecada238402 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1356,121 +1356,6 @@ static int rzg3s_pcie_resets_prepare_and_get(struct rzg3s_pcie_host *host)
host->cfg_resets);
}
-static int rzg3s_pcie_host_parse_port(struct rzg3s_pcie_host *host)
-{
- struct device_node *of_port __free(device_node) =
- of_get_next_child(host->dev->of_node, NULL);
- struct rzg3s_pcie_port *port = &host->port;
- int ret;
-
- ret = of_property_read_u32(of_port, "vendor-id", &port->vendor_id);
- if (ret)
- return ret;
-
- ret = of_property_read_u32(of_port, "device-id", &port->device_id);
- if (ret)
- return ret;
-
- port->refclk = of_clk_get_by_name(of_port, "ref");
- if (IS_ERR(port->refclk))
- return PTR_ERR(port->refclk);
-
- return 0;
-}
-
-static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
-{
- struct rzg3s_pcie_port *port = &host->port;
- struct device *dev = host->dev;
- int ret;
-
- /* Enable access control to the CFGU */
- writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
- host->axi + RZG3S_PCI_PERM);
-
- /* Update vendor ID and device ID */
- writew_relaxed(port->vendor_id, host->pcie + PCI_VENDOR_ID);
- writew_relaxed(port->device_id, host->pcie + PCI_DEVICE_ID);
-
- /* Disable access control to the CFGU */
- writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
-
- ret = clk_prepare_enable(port->refclk);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to enable refclk!\n");
-
- /* Set the PHY, if any */
- if (host->data->init_phy) {
- ret = host->data->init_phy(host);
- if (ret) {
- dev_err_probe(dev, ret, "Failed to set the PHY!\n");
- goto refclk_disable;
- }
- }
-
- return 0;
-
-refclk_disable:
- clk_disable_unprepare(port->refclk);
- return ret;
-}
-
-static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
-{
- u32 val;
- int ret;
-
- /* SoC-specific pre-configuration */
- if (host->data->config_pre_init)
- host->data->config_pre_init(host);
-
- /* Initialize the PCIe related registers */
- ret = rzg3s_pcie_config_init(host);
- if (ret)
- goto config_deinit;
-
- ret = rzg3s_pcie_host_init_port(host);
- if (ret)
- goto config_deinit;
-
- /* Enable ASPM L1 transition for SoCs that use it */
- ret = rzg3s_sysc_config_func(host->sysc,
- RZG3S_SYSC_FUNC_ID_L1_ALLOW, 1);
- if (ret)
- goto config_deinit_and_refclk;
-
- /* Initialize the interrupts */
- rzg3s_pcie_irq_init(host);
-
- /* SoC-specific post-configuration */
- ret = host->data->config_post_init(host);
- if (ret)
- goto config_deinit_and_refclk;
-
- /* Wait for link up */
- ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
- !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
- PCIE_LINK_WAIT_SLEEP_MS * MILLI,
- PCIE_LINK_WAIT_SLEEP_MS * MILLI *
- PCIE_LINK_WAIT_MAX_RETRIES);
- if (ret)
- goto config_deinit_post;
-
- val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
- dev_info(host->dev, "PCIe link status [0x%x]\n", val);
-
- return 0;
-
-config_deinit_post:
- host->data->config_deinit(host);
-config_deinit_and_refclk:
- clk_disable_unprepare(host->port.refclk);
-config_deinit:
- if (host->data->config_pre_init)
- host->data->config_deinit(host);
- return ret;
-}
-
static void rzg3s_pcie_set_inbound_window(struct rzg3s_pcie_host *host,
u64 cpu_addr, u64 pci_addr, u64 size,
int id)
@@ -1698,6 +1583,121 @@ static int rzg3s_soc_pcie_init_phy(struct rzg3s_pcie_host *host)
return 0;
}
+static int rzg3s_pcie_host_parse_port(struct rzg3s_pcie_host *host)
+{
+ struct device_node *of_port __free(device_node) =
+ of_get_next_child(host->dev->of_node, NULL);
+ struct rzg3s_pcie_port *port = &host->port;
+ int ret;
+
+ ret = of_property_read_u32(of_port, "vendor-id", &port->vendor_id);
+ if (ret)
+ return ret;
+
+ ret = of_property_read_u32(of_port, "device-id", &port->device_id);
+ if (ret)
+ return ret;
+
+ port->refclk = of_clk_get_by_name(of_port, "ref");
+ if (IS_ERR(port->refclk))
+ return PTR_ERR(port->refclk);
+
+ return 0;
+}
+
+static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
+{
+ struct rzg3s_pcie_port *port = &host->port;
+ struct device *dev = host->dev;
+ int ret;
+
+ /* Enable access control to the CFGU */
+ writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
+ host->axi + RZG3S_PCI_PERM);
+
+ /* Update vendor ID and device ID */
+ writew_relaxed(port->vendor_id, host->pcie + PCI_VENDOR_ID);
+ writew_relaxed(port->device_id, host->pcie + PCI_DEVICE_ID);
+
+ /* Disable access control to the CFGU */
+ writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
+
+ ret = clk_prepare_enable(port->refclk);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable refclk!\n");
+
+ /* Set the PHY, if any */
+ if (host->data->init_phy) {
+ ret = host->data->init_phy(host);
+ if (ret) {
+ dev_err_probe(dev, ret, "Failed to set the PHY!\n");
+ goto refclk_disable;
+ }
+ }
+
+ return 0;
+
+refclk_disable:
+ clk_disable_unprepare(port->refclk);
+ return ret;
+}
+
+static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
+{
+ u32 val;
+ int ret;
+
+ /* SoC-specific pre-configuration */
+ if (host->data->config_pre_init)
+ host->data->config_pre_init(host);
+
+ /* Initialize the PCIe related registers */
+ ret = rzg3s_pcie_config_init(host);
+ if (ret)
+ goto config_deinit;
+
+ ret = rzg3s_pcie_host_init_port(host);
+ if (ret)
+ goto config_deinit;
+
+ /* Enable ASPM L1 transition for SoCs that use it */
+ ret = rzg3s_sysc_config_func(host->sysc,
+ RZG3S_SYSC_FUNC_ID_L1_ALLOW, 1);
+ if (ret)
+ goto config_deinit_and_refclk;
+
+ /* Initialize the interrupts */
+ rzg3s_pcie_irq_init(host);
+
+ /* SoC-specific post-configuration */
+ ret = host->data->config_post_init(host);
+ if (ret)
+ goto config_deinit_and_refclk;
+
+ /* Wait for link up */
+ ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
+ !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
+ PCIE_LINK_WAIT_SLEEP_MS * MILLI,
+ PCIE_LINK_WAIT_SLEEP_MS * MILLI *
+ PCIE_LINK_WAIT_MAX_RETRIES);
+ if (ret)
+ goto config_deinit_post;
+
+ val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
+ dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+
+ return 0;
+
+config_deinit_post:
+ host->data->config_deinit(host);
+config_deinit_and_refclk:
+ clk_disable_unprepare(host->port.refclk);
+config_deinit:
+ if (host->data->config_pre_init)
+ host->data->config_deinit(host);
+ return ret;
+}
+
static int
rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
int (*init_irqdomain)(struct rzg3s_pcie_host *host),
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 4/9] PCI: rzg3s-host: Move host configuration code together
2026-10-07 5:36 ` [PATCH v6 4/9] PCI: rzg3s-host: Move host configuration code together Claudiu Beznea
@ 2026-10-07 5:47 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:47 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-pci, wsa+renesas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cleanup: mixing goto and __free() in rzg3s_pcie_probe
--
commit 58c1ad45841aacf58a73f4caada04798bba4e7e2
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
PCI: rzg3s-host: Move host configuration code together
This commit rearranges the host configuration functions together to prepare for
hotplug support. It also adjusts the error handling paths and labels in the
probe and suspend/resume routines.
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 077cfb0834b37..3ecada2384023 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1902,12 +1893,16 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
> if (ret)
> goto rpm_disable;
>
> + ret = rzg3s_pcie_power_resets_deassert(host);
> + if (ret)
> + goto rpm_put;
> +
[Severity: Medium]
Does this error handling violate the cleanup subsystem requirements?
The rzg3s_pcie_probe() function declares sysc_np using __free(device_node):
struct device_node *sysc_np __free(device_node) =
of_parse_phandle(np, "renesas,sysc", 0);
However, the function also heavily uses manual goto rollback labels, and
this patch modifies those paths. According to include/linux/cleanup.h,
mixing goto-based cleanup labels and scope-based __free() in the same
function is not permitted due to the risk of confusing ownership semantics.
Should this function either be converted entirely to scope-based cleanups
or avoid __free() entirely?
> raw_spin_lock_init(&host->hw_lock);
>
> ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_init_irqdomain,
> rzg3s_pcie_teardown_irqdomain);
> if (ret)
> - goto rpm_put;
> + goto power_resets_assert;
>
> bridge->sysdata = host;
> bridge->ops = &rzg3s_pcie_root_ops;
> @@ -1922,12 +1917,13 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
> clk_disable_unprepare(host->port.refclk);
> rzg3s_pcie_teardown_irqdomain(host);
> host->data->config_deinit(host);
> +power_resets_assert:
> + reset_control_bulk_assert(host->data->num_power_resets,
> + host->power_resets);
> rpm_put:
> pm_runtime_put_sync(dev);
> rpm_disable:
> pm_runtime_disable(dev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=4
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 5/9] PCI: rzg3s-host: Move suspend/resume code into dedicated functions
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (3 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 4/9] PCI: rzg3s-host: Move host configuration code together Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:52 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 6/9] PCI: rzg3s-host: Move IRQ domain setup code Claudiu Beznea
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
In preparation for implementing hotplug using
pci_host_bridge::reset_root_port(), move the suspend/resume code into
rzg3s_pcie_host_stop() and rzg3s_pcie_host_start(). These functions
will later be reused by the hotplug implementation through
pci_host_bridge::reset_root_port().
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
Changes in v4:
- none
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 181 ++++++++++++-----------
1 file changed, 96 insertions(+), 85 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 3ecada238402..40d5ef3e347e 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1742,6 +1742,100 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
return ret;
}
+static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
+{
+ const struct rzg3s_pcie_soc_data *data = host->data;
+ struct rzg3s_pcie_port *port = &host->port;
+ struct rzg3s_sysc *sysc = host->sysc;
+ int ret;
+
+ clk_disable_unprepare(port->refclk);
+
+ /* SoC-specific de-initialization */
+ ret = data->config_deinit(host);
+ if (ret)
+ goto refclk_restore;
+
+ ret = reset_control_bulk_assert(data->num_power_resets,
+ host->power_resets);
+ if (ret)
+ goto config_reinit;
+
+ /*
+ * Since the power domain's genpd_suspend_noirq() will disable clocks,
+ * there is no need to manually invoke runtime PM API here.
+ */
+
+ ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+ if (ret)
+ goto power_resets_restore;
+
+ return 0;
+
+ /* Restore the previous state if any error happens */
+power_resets_restore:
+ reset_control_bulk_deassert(data->num_power_resets,
+ host->power_resets);
+config_reinit:
+ if (data->config_pre_init)
+ data->config_pre_init(host);
+ data->config_post_init(host);
+refclk_restore:
+ clk_prepare_enable(port->refclk);
+ return ret;
+}
+
+static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
+{
+ const struct rzg3s_pcie_soc_data *data = host->data;
+ struct rzg3s_sysc *sysc = host->sysc;
+ int ret;
+
+ ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
+ if (ret)
+ return ret;
+
+ ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);
+ if (ret)
+ return ret;
+
+ if (host->num_lanes) {
+ ret = rzg3s_sysc_config_func(host->sysc,
+ RZG3S_SYSC_FUNC_ID_LINK_MASTER,
+ host->num_lanes == 2 ?
+ RZG3S_SYSC_LINK_MODE_DUAL_X2 :
+ RZG3S_SYSC_LINK_MODE_SINGLE_X4);
+ if (ret)
+ goto assert_rst_rsm_b;
+ }
+
+ /*
+ * Since the power domain's genpd_resume_noirq() will enable clocks,
+ * there is no need to manually invoke runtime PM API here.
+ */
+
+ ret = rzg3s_pcie_power_resets_deassert(host);
+ if (ret)
+ goto assert_rst_rsm_b;
+
+ ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
+ rzg3s_pcie_msi_hw_teardown);
+ if (ret)
+ goto assert_power_resets;
+
+ return 0;
+
+ /*
+ * If any error happens there is no way to recover the IP. Put it in the
+ * lowest possible power state.
+ */
+assert_power_resets:
+ reset_control_bulk_assert(data->num_power_resets, host->power_resets);
+assert_rst_rsm_b:
+ rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+ return ret;
+}
+
static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
{
struct device_node *np = host->dev->of_node;
@@ -1939,98 +2033,15 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
static int rzg3s_pcie_suspend_noirq(struct device *dev)
{
struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
- const struct rzg3s_pcie_soc_data *data = host->data;
- struct rzg3s_pcie_port *port = &host->port;
- struct rzg3s_sysc *sysc = host->sysc;
- int ret;
-
- clk_disable_unprepare(port->refclk);
-
- /* SoC-specific de-initialization */
- ret = data->config_deinit(host);
- if (ret)
- goto refclk_restore;
-
- ret = reset_control_bulk_assert(data->num_power_resets,
- host->power_resets);
- if (ret)
- goto config_reinit;
-
- /*
- * Since the power domain's genpd_suspend_noirq() will disable clocks,
- * there is no need to manually invoke runtime PM API here.
- */
-
- ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
- if (ret)
- goto power_resets_restore;
- return 0;
-
- /* Restore the previous state if any error happens */
-power_resets_restore:
- reset_control_bulk_deassert(data->num_power_resets,
- host->power_resets);
-config_reinit:
- if (data->config_pre_init)
- data->config_pre_init(host);
- data->config_post_init(host);
-refclk_restore:
- clk_prepare_enable(port->refclk);
- return ret;
+ return rzg3s_pcie_host_stop(host);
}
static int rzg3s_pcie_resume_noirq(struct device *dev)
{
struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
- const struct rzg3s_pcie_soc_data *data = host->data;
- struct rzg3s_sysc *sysc = host->sysc;
- int ret;
- ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
- if (ret)
- return ret;
-
- ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);
- if (ret)
- return ret;
-
- if (host->num_lanes) {
- ret = rzg3s_sysc_config_func(host->sysc,
- RZG3S_SYSC_FUNC_ID_LINK_MASTER,
- host->num_lanes == 2 ?
- RZG3S_SYSC_LINK_MODE_DUAL_X2 :
- RZG3S_SYSC_LINK_MODE_SINGLE_X4);
- if (ret)
- goto assert_rst_rsm_b;
- }
-
- /*
- * Since the power domain's genpd_resume_noirq() will enable clocks,
- * there is no need to manually invoke runtime PM API here.
- */
-
- ret = rzg3s_pcie_power_resets_deassert(host);
- if (ret)
- goto assert_rst_rsm_b;
-
- ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
- rzg3s_pcie_msi_hw_teardown);
- if (ret)
- goto assert_power_resets;
-
- return 0;
-
- /*
- * If any error happens there is no way to recover the IP. Put it in the
- * lowest possible power state.
- */
-assert_power_resets:
- reset_control_bulk_assert(data->num_power_resets,
- host->power_resets);
-assert_rst_rsm_b:
- rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
- return ret;
+ return rzg3s_pcie_host_start(host);
}
static const struct dev_pm_ops rzg3s_pcie_pm_ops = {
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v6 6/9] PCI: rzg3s-host: Move IRQ domain setup code
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (4 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 5/9] PCI: rzg3s-host: Move suspend/resume code into dedicated functions Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:47 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 7/9] PCI: rzg3s-host: Add helper to wait for link up Claudiu Beznea
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea, Lad Prabhakar
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subsequent patches add support for the event IRQ to handle link
up/down events. The event IRQ handler will use
rzg3s_pcie_set_max_link_speed(). In preparation for adding event IRQ
support, move the IRQ domain initialization code after
rzg3s_pcie_set_max_link_speed().
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none
Changes in v5:
- collected tags
- restored the previous order in rzg3s_pcie_teardown_intx()
Changes in v4:
- none
Changes in v3:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 147 +++++++++++------------
1 file changed, 73 insertions(+), 74 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 40d5ef3e347e..3b62b1be5b2a 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1006,80 +1006,6 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
.xlate = irq_domain_xlate_onetwocell,
};
-static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host,
- int count)
-{
- while (--count >= 0) {
- irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
- NULL);
- }
-
- if (host->intx_domain)
- irq_domain_remove(host->intx_domain);
-}
-
-static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
-{
- struct device *dev = host->dev;
- struct platform_device *pdev = to_platform_device(dev);
- int i, ret;
-
- for (i = 0; i < PCI_NUM_INTX; i++) {
- char irq_name[5] = {0};
- int irq;
-
- scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
-
- irq = platform_get_irq_byname(pdev, irq_name);
- if (irq < 0) {
- ret = irq;
- dev_err_probe(dev, ret,
- "Failed to parse and map INT%c IRQ\n",
- 'A' + i);
- goto teardown_intx;
- }
-
- host->intx_irqs[i] = irq;
- irq_set_chained_handler_and_data(irq,
- rzg3s_pcie_intx_irq_handler,
- host);
- }
-
- host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
- PCI_NUM_INTX,
- &rzg3s_pcie_intx_domain_ops,
- host);
- if (!host->intx_domain) {
- ret = -EINVAL;
- dev_err_probe(dev, ret,
- "Failed to add irq domain for INTx IRQs\n");
- goto teardown_intx;
- }
- irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
-
- if (IS_ENABLED(CONFIG_PCI_MSI)) {
- ret = rzg3s_pcie_init_msi(host);
-
- if (ret)
- goto teardown_intx;
- }
-
- return 0;
-
-teardown_intx:
- rzg3s_pcie_teardown_intx(host, i);
-
- return ret;
-}
-
-static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
-{
- if (IS_ENABLED(CONFIG_PCI_MSI))
- rzg3s_pcie_teardown_msi(host);
-
- rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
-}
-
static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
{
u32 remote_supported_link_speeds, max_supported_link_speeds;
@@ -1169,6 +1095,79 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
return ret;
}
+static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
+{
+ while (--count >= 0) {
+ irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
+ NULL);
+ }
+
+ if (host->intx_domain)
+ irq_domain_remove(host->intx_domain);
+}
+
+static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
+{
+ struct device *dev = host->dev;
+ struct platform_device *pdev = to_platform_device(dev);
+ int i, ret;
+
+ for (i = 0; i < PCI_NUM_INTX; i++) {
+ char irq_name[5] = {0};
+ int irq;
+
+ scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
+
+ irq = platform_get_irq_byname(pdev, irq_name);
+ if (irq < 0) {
+ ret = irq;
+ dev_err_probe(dev, ret,
+ "Failed to parse and map INT%c IRQ\n",
+ 'A' + i);
+ goto teardown_intx;
+ }
+
+ host->intx_irqs[i] = irq;
+ irq_set_chained_handler_and_data(irq,
+ rzg3s_pcie_intx_irq_handler,
+ host);
+ }
+
+ host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
+ PCI_NUM_INTX,
+ &rzg3s_pcie_intx_domain_ops,
+ host);
+ if (!host->intx_domain) {
+ ret = -EINVAL;
+ dev_err_probe(dev, ret,
+ "Failed to add irq domain for INTx IRQs\n");
+ goto teardown_intx;
+ }
+ irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ ret = rzg3s_pcie_init_msi(host);
+
+ if (ret)
+ goto teardown_intx;
+ }
+
+ return 0;
+
+teardown_intx:
+ rzg3s_pcie_teardown_intx(host, i);
+
+ return ret;
+}
+
+static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
+{
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ rzg3s_pcie_teardown_msi(host);
+
+ rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
+}
+
static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
{
struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 6/9] PCI: rzg3s-host: Move IRQ domain setup code
2026-10-07 5:36 ` [PATCH v6 6/9] PCI: rzg3s-host: Move IRQ domain setup code Claudiu Beznea
@ 2026-10-07 5:47 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:47 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-pci, wsa+renesas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] irq: race in INTx chained handler setup before domain creation
--
commit 3d8eb4132e9198313898445e6472b136f82e2b3d
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
PCI: rzg3s-host: Move IRQ domain setup code
Subsequent patches add support for the event IRQ to handle link
up/down events. The event IRQ handler will use
rzg3s_pcie_set_max_link_speed(). In preparation for adding event IRQ
support, move the IRQ domain initialization code after
rzg3s_pcie_set_max_link_speed().
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 40d5ef3e347e4..3b62b1be5b2a1 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1169,6 +1095,79 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
[ ... ]
> +static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
> +{
> + struct device *dev = host->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + int i, ret;
> +
> + for (i = 0; i < PCI_NUM_INTX; i++) {
> + char irq_name[5] = {0};
> + int irq;
> +
> + scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
> +
> + irq = platform_get_irq_byname(pdev, irq_name);
> + if (irq < 0) {
> + ret = irq;
> + dev_err_probe(dev, ret,
> + "Failed to parse and map INT%c IRQ\n",
> + 'A' + i);
> + goto teardown_intx;
> + }
> +
> + host->intx_irqs[i] = irq;
> + irq_set_chained_handler_and_data(irq,
> + rzg3s_pcie_intx_irq_handler,
> + host);
> + }
> +
> + host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
> + PCI_NUM_INTX,
[Severity: High]
Does registering the chained handler here open a race window before the domain
is created?
If the interrupt fires after irq_set_chained_handler_and_data() but before
irq_domain_create_linear(), rzg3s_pcie_intx_irq_handler() will execute with
host->intx_domain still being NULL.
This would result in a call to:
generic_handle_domain_irq(NULL, intx);
Since the domain is NULL, __irq_resolve_mapping() will fall back to
irq_default_domain, which could incorrectly map the hardware IRQ (usually 0-3)
to an unrelated system interrupt.
Can this lead to unpredictable panics or memory corruption if an unintended
handler is executed?
> + &rzg3s_pcie_intx_domain_ops,
> + host);
> + if (!host->intx_domain) {
> + ret = -EINVAL;
> + dev_err_probe(dev, ret,
> + "Failed to add irq domain for INTx IRQs\n");
> + goto teardown_intx;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=6
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 7/9] PCI: rzg3s-host: Add helper to wait for link up
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (5 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 6/9] PCI: rzg3s-host: Move IRQ domain setup code Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:44 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes Claudiu Beznea
2026-10-07 5:36 ` [PATCH v6 9/9] PCI: rzg3s-host: Add bridge::reset_root_port() Claudiu Beznea
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Factor out the link up polling from rzg3s_pcie_host_init() into a new
helper. With the link up/down event handling added by a subsequent
commit, the probe will no longer fail when the link is down, but
continue and rely on later link up events to detect the link becoming
active. Having the polling in a dedicated helper allows the probe path
to treat its failure as non-fatal.
No functional change.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- none, this patch is new
drivers/pci/controller/pcie-rzg3s-host.c | 29 ++++++++++++++++--------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 3b62b1be5b2a..ebe89e6a0796 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1095,6 +1095,25 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
return ret;
}
+static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)
+{
+ u32 val;
+ int ret;
+
+ ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
+ !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
+ PCIE_LINK_WAIT_SLEEP_MS * MILLI,
+ PCIE_LINK_WAIT_SLEEP_MS * MILLI *
+ PCIE_LINK_WAIT_MAX_RETRIES);
+ if (ret)
+ return ret;
+
+ val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
+ dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+
+ return 0;
+}
+
static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
{
while (--count >= 0) {
@@ -1673,18 +1692,10 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
if (ret)
goto config_deinit_and_refclk;
- /* Wait for link up */
- ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
- !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
- PCIE_LINK_WAIT_SLEEP_MS * MILLI,
- PCIE_LINK_WAIT_SLEEP_MS * MILLI *
- PCIE_LINK_WAIT_MAX_RETRIES);
+ ret = rzg3s_pcie_wait_for_link_up(host);
if (ret)
goto config_deinit_post;
- val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
- dev_info(host->dev, "PCIe link status [0x%x]\n", val);
-
return 0;
config_deinit_post:
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v6 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (6 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 7/9] PCI: rzg3s-host: Add helper to wait for link up Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:53 ` sashiko-bot
2026-10-07 5:36 ` [PATCH v6 9/9] PCI: rzg3s-host: Add bridge::reset_root_port() Claudiu Beznea
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
John Madieu, Claudiu Beznea
From: John Madieu <john.madieu.xa@bp.renesas.com>
The RZ/G3{E, S}, RZ/V2{H(P), N} PCIe controllers do not expose the
standard PCIe Slot Capability registers, so the generic pciehp driver
cannot be used. The only link-state signal the hardware provides is the
DL_UpDown bit in the PEIS0 event status register, which is raised on every
Data Link layer up/down transition.
Enable DL_UpDown in PEIE0 and hook up an interrupt handler so the driver
can react to link-state changes: a device that trains after boot gets
enumerated, and a device that disappears on link loss is removed. This
provides hotplug-like behavior without the PCI hotplug core, which is
unavailable for the reason above.
On a DL_UpDown event the handler acks the W1C status bit and inspects
PCSTAT1.DL_DOWN_STS:
- Link up: re-run max link speed negotiation, wait for the link to settle
and rescan the root bus. To cover the case where the device is replaced
or removed while the system is suspended add the
rzg3s_pcie_device_replaced(). It reads the vendor ID, device ID,
revision ID and class code of the device behind the root port and
compares them against the values cached in the corresponding
struct pci_dev, along with the Device Serial Number cached in
struct rzg3s_pcie_host::con_dev_dsn, mirroring pciehp_device_replaced().
If the device is detected as replaced (or no longer present),
pci_dev_set_disconnected() is called for all the devices on the bus
below the root port, so that the teardown of the old devices does not
end up accessing the newly connected device, and the devices are then
removed. If a link up/down cycle occurred while devices are on the bus
(fast link transitions coalesced into a single DL_UpDown event), the
devices are removed and re-enumerated even if the connected device did
not change.
- Link down: walk the bus in reverse and pci_stop_and_remove_bus_device()
each child.
Both paths take pci_lock_rescan_remove() to serialize against the PCI
core.
Link events are processed only after the controller has been fully
initialized.
Add rzg3s_pcie_pm_notifier() to avoid the link event interrupt interfering
with the suspend/resume of the other PCIe devices in the topology, and to
avoid deadlocks caused by pci_dev_lock() being taken from the link event
handler while suspend/resume is in progress. The notifier disables the link
event interrupt before suspend and enables it and rescans the bus, on
resume, to pick up devices that may have been plugged or unplugged while
the system was suspended. struct rzg3s_pcie_host::started was added to
avoid touching the controller registers in case the resume failed.
While at it, make probe tolerant of an absent device. Previously, if the
link failed to come up during rzg3s_pcie_host_setup(), probe tore the
controller back down and failed. Ignore the timeout returned by
rzg3s_pcie_wait_for_link_up(), leave the controller and refclk running,
and let the link-up path enumerate the device once it appears.
Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
Co-developed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- moved the struct rzg3s_pcie_host::started in this patch to avoid
setting the controller in failure cases (started = false)
- adopted the approch from pciehp driver with regards to checking the
DSN of the connected device and decide, based on this, if the bus
devices need to be removed + rescanned; the DSN of the connected
device is cached in probe and link up events
- in case of link up events with !list_empty(&bus->devices) remove
first the devices from the bus and re-scan (adopted from pciehp driver)
- in rzg3s_pcie_pm_notifier() touch the HW registers only if host->started
- in rzg3s_pcie_resume_noirq() call pci_dev_set_disconnected() for the
devices on the bus if the connected device is changed at the resume
similar to what the pciehp driver is doing
- made adjustments to rzg3s_pcie_wait_for_link_up()
- dropped the tags
- adjusted patch description
Changes in v5:
- in rzg3s_pcie_link_event(), on link up path, added pcie_bus_configure_settings();
and dropped pci_rescan_bus() and inlined its instructions
- pm_runtime_get_sync(&bridge->dev)/pm_runtime_put_sync(&bridge->dev) to avoid
"pci 0000:00:00.0: runtime PM trying to activate child device 0000:00:00.0 but
parent (pci0000:00) is not active"
- added a PM notifier and disable/enable the link interrupt
before suspend/after resume to avoid deadlocks b/w PM core and any PCI
core code calling pci_dev_lock(); the next patch uses pci_host_handle_link_down()
which calls pci_dev_lock() on the following path:
pci_host_handle_link_down() ->
pci_host_reset_root_port() ->
pci_bus_error_reset() ->
pci_reset_bridge() ->
pci_slot_reset() ->
pci_slot_lock() ->
__pci_bus_lock() ->
pci_dev_lock()
- added struct rzg3s_pcie_host::link_rescan to force a link rescan on return
from resume and handle scenarios where devices dissaper while in suspend
- updated the commit message to reflect this
- didn't collect the tags from v4 dues to these changes
Changes in v4:
- dropped .reset_root_port() changes
Changes in v3:
- added RZG3S_PCI_PEIE0_DL_UPDOWN
- re-worked the support by implemeting
struct pci_host_bridge::reset_root_port()
- introduced the struct rzg3s_pcie_host::state to:
-- avoid touching the controller while a reset root port is in progress
-- and avoid touching the controller in case a reset root port failed
-- and to be able to re-use the already existing code in the reset
root port function
-- and added CLASS() constructs helpers for it to keep the state handling
code simpler
- updated the patch description to reflect the updates
drivers/pci/controller/pcie-rzg3s-host.c | 320 +++++++++++++++++++++--
1 file changed, 301 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index ebe89e6a0796..c2fbacfe6704 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -33,6 +33,7 @@
#include <linux/reset.h>
#include <linux/sizes.h>
#include <linux/slab.h>
+#include <linux/suspend.h>
#include <linux/units.h>
#include "pci-host-common.h"
@@ -86,6 +87,7 @@
#define RZG3S_PCI_MSGRCVIS_MRI BIT(24)
#define RZG3S_PCI_PEIE0 0x200
+#define RZG3S_PCI_PEIE0_DL_UPDOWN BIT(9)
#define RZG3S_PCI_PEIS0 0x204
#define RZG3S_PCI_PEIS0_RX_DLLP_PM_ENTER BIT(12)
@@ -323,9 +325,16 @@ struct rzg3s_pcie_port {
* @msi: MSI data structure
* @port: PCIe Root Port
* @hw_lock: lock for access to the HW resources
+ * @pm_nb: PM notifier block (for link events)
+ * @con_dev_dsn: Device Serial Number for the connected device, used to
+ * determine whether a hotplugged device was replaced with a different one
+ * during system sleep
+ * @event_irq: PCIe event interrupt for DL_UpDown detection
* @intx_irqs: INTx interrupts
* @max_link_speed: maximum supported link speed
* @controller_id: PCIe controller identifier, used for System Controller access
+ * @link_rescan: The PCIe link rescan state
+ * @started: The PCIe controller state (started or not)
* @num_lanes: The number of lanes
*/
struct rzg3s_pcie_host {
@@ -340,9 +349,14 @@ struct rzg3s_pcie_host {
struct rzg3s_pcie_msi msi;
struct rzg3s_pcie_port port;
raw_spinlock_t hw_lock;
+ struct notifier_block pm_nb;
+ u64 con_dev_dsn;
+ int event_irq;
int intx_irqs[PCI_NUM_INTX];
int max_link_speed;
enum rzg3s_pcie_controller_id controller_id;
+ bool link_rescan;
+ bool started;
u8 num_lanes;
};
@@ -1105,12 +1119,188 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)
PCIE_LINK_WAIT_SLEEP_MS * MILLI,
PCIE_LINK_WAIT_SLEEP_MS * MILLI *
PCIE_LINK_WAIT_MAX_RETRIES);
- if (ret)
- return ret;
+ if (ret) {
+ dev_info(host->dev,
+ "PCIe link down, waiting for DL_UpDown\n");
+ }
val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+ return ret;
+}
+
+static struct pci_dev
+*rzg3s_pcie_get_connected_dev(struct rzg3s_pcie_host *host)
+{
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
+ struct pci_dev *port __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+
+ if (!port || !port->subordinate)
+ return NULL;
+
+ return pci_get_slot(port->subordinate, PCI_DEVFN(0, 0));
+}
+
+static void rzg3s_pcie_cache_connected_dev_id(struct rzg3s_pcie_host *host)
+{
+ struct pci_dev *con_dev __free(pci_dev_put) =
+ rzg3s_pcie_get_connected_dev(host);
+
+ host->con_dev_dsn = con_dev ? pci_get_dsn(con_dev) : 0;
+}
+
+static bool rzg3s_pcie_device_replaced(struct rzg3s_pcie_host *host)
+{
+ struct pci_dev *con_dev __free(pci_dev_put) =
+ rzg3s_pcie_get_connected_dev(host);
+ u32 reg;
+
+ if (!con_dev)
+ return true;
+
+ if (pci_read_config_dword(con_dev, PCI_VENDOR_ID, ®) ||
+ reg != (con_dev->vendor | (con_dev->device << 16)) ||
+ pci_read_config_dword(con_dev, PCI_CLASS_REVISION, ®) ||
+ reg != (con_dev->revision | (con_dev->class << 8)))
+ return true;
+
+ if (con_dev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
+ (pci_read_config_dword(con_dev, PCI_SUBSYSTEM_VENDOR_ID, ®) ||
+ reg != (con_dev->subsystem_vendor |
+ (con_dev->subsystem_device << 16))))
+ return true;
+
+ if (pci_get_dsn(con_dev) != host->con_dev_dsn)
+ return true;
+
+ return false;
+}
+
+static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
+{
+ struct pci_dev *dev, *tmp;
+
+ list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list)
+ pci_stop_and_remove_bus_device(dev);
+}
+
+static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
+{
+ struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
+ struct pci_bus *bus = bridge->bus, *child;
+ u32 val;
+ int ret;
+
+ pci_lock_rescan_remove();
+
+ val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
+ if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
+ if (!list_empty(&bus->devices)) {
+ dev_info(host->dev,
+ "PCIe link down, removing devices\n");
+ rzg3s_pcie_remove_devices(bus);
+ }
+ goto unlock;
+ }
+
+ if (!list_empty(&bus->devices)) {
+ bool replaced = rzg3s_pcie_device_replaced(host);
+
+ /*
+ * If bounced = false it means link rescanning (after probe
+ * or system resume), no link transition was recorded. The
+ * enumerated devices are intact, so remove them only if the
+ * connected device was replaced while link events were not
+ * delivered.
+ *
+ * If bounced = true it means at least one real link down/up
+ * cycle occurred.
+ */
+ if (!bounced && !replaced)
+ goto unlock;
+
+ if (replaced) {
+ struct pci_dev *port __free(pci_dev_put) =
+ pci_get_slot(bus, PCI_DEVFN(0, 0));
+
+ if (port && port->subordinate)
+ pci_walk_bus(port->subordinate,
+ pci_dev_set_disconnected, NULL);
+ }
+
+ rzg3s_pcie_remove_devices(bus);
+ }
+
+ dev_info(host->dev, "PCIe link up, rescanning bus\n");
+
+ ret = rzg3s_pcie_set_max_link_speed(host);
+ if (ret)
+ dev_info(host->dev, "Failed to set max link speed\n");
+
+ pci_host_common_link_train_delay(host->max_link_speed);
+
+ pm_runtime_get_sync(&bridge->dev);
+
+ pci_scan_child_bus(bus);
+ pci_assign_unassigned_bus_resources(bus);
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ pci_bus_add_devices(bus);
+
+ rzg3s_pcie_cache_connected_dev_id(host);
+
+ pm_runtime_put_sync(&bridge->dev);
+unlock:
+ pci_unlock_rescan_remove();
+}
+
+static irqreturn_t rzg3s_pcie_event_irq_thread(int irq, void *data)
+{
+ struct rzg3s_pcie_host *host = data;
+ bool bounced;
+ u32 status;
+
+ status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
+ bounced = status & RZG3S_PCI_PEIS0_DL_UPDOWN;
+
+ if (!bounced && !READ_ONCE(host->link_rescan))
+ return IRQ_NONE;
+
+ /* Clear the DL_UpDown status (W1C) */
+ if (bounced)
+ writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+ WRITE_ONCE(host->link_rescan, false);
+
+ rzg3s_pcie_link_event(host, bounced);
+
+ return IRQ_HANDLED;
+}
+
+static int rzg3s_pcie_request_event_irq(struct rzg3s_pcie_host *host)
+{
+ struct device *dev = host->dev;
+ struct platform_device *pdev = to_platform_device(dev);
+ const char *evt_name;
+ int ret, irq;
+
+ evt_name = devm_kasprintf(dev, GFP_KERNEL, "%s-evt", dev_name(dev));
+ if (!evt_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname(pdev, "pcie_evt");
+ if (irq < 0)
+ return irq;
+
+ ret = request_threaded_irq(irq, NULL, rzg3s_pcie_event_irq_thread,
+ IRQF_ONESHOT, evt_name, host);
+ if (ret) {
+ return dev_err_probe(dev, ret,
+ "Failed to request pcie_evt IRQ\n");
+ }
+ host->event_irq = irq;
+
return 0;
}
@@ -1125,6 +1315,17 @@ static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
irq_domain_remove(host->intx_domain);
}
+static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
+{
+ if (host->event_irq > 0)
+ free_irq(host->event_irq, host);
+
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ rzg3s_pcie_teardown_msi(host);
+
+ rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
+}
+
static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
{
struct device *dev = host->dev;
@@ -1171,22 +1372,21 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
goto teardown_intx;
}
+ ret = rzg3s_pcie_request_event_irq(host);
+ if (ret)
+ goto teardown_msi;
+
return 0;
+teardown_msi:
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ rzg3s_pcie_teardown_msi(host);
teardown_intx:
rzg3s_pcie_teardown_intx(host, i);
return ret;
}
-static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
-{
- if (IS_ENABLED(CONFIG_PCI_MSI))
- rzg3s_pcie_teardown_msi(host);
-
- rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
-}
-
static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
{
struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
@@ -1662,7 +1862,6 @@ static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
{
- u32 val;
int ret;
/* SoC-specific pre-configuration */
@@ -1692,14 +1891,8 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
if (ret)
goto config_deinit_and_refclk;
- ret = rzg3s_pcie_wait_for_link_up(host);
- if (ret)
- goto config_deinit_post;
-
return 0;
-config_deinit_post:
- host->data->config_deinit(host);
config_deinit_and_refclk:
clk_disable_unprepare(host->port.refclk);
config_deinit:
@@ -1738,6 +1931,10 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
goto teardown_irqdomain;
}
+ ret = rzg3s_pcie_wait_for_link_up(host);
+ if (ret)
+ return 0;
+
ret = rzg3s_pcie_set_max_link_speed(host);
if (ret)
dev_info(dev, "Failed to set max link speed\n");
@@ -1759,6 +1956,9 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
struct rzg3s_sysc *sysc = host->sysc;
int ret;
+ if (!host->started)
+ return 0;
+
clk_disable_unprepare(port->refclk);
/* SoC-specific de-initialization */
@@ -1780,6 +1980,8 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
if (ret)
goto power_resets_restore;
+ host->started = false;
+
return 0;
/* Restore the previous state if any error happens */
@@ -1801,6 +2003,9 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
struct rzg3s_sysc *sysc = host->sysc;
int ret;
+ if (host->started)
+ return 0;
+
ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
if (ret)
return ret;
@@ -1833,6 +2038,8 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
if (ret)
goto assert_power_resets;
+ host->started = true;
+
return 0;
/*
@@ -1916,6 +2123,41 @@ static void rzv2h_pcie_release_lanes(void *data)
rzv2h_num_total_lanes -= host->num_lanes;
}
+static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
+ pm_nb);
+
+ switch (action) {
+ case PM_SUSPEND_PREPARE:
+ /* Disable link up/down interrupts. */
+ disable_irq(host->event_irq);
+ break;
+
+ case PM_POST_SUSPEND:
+ /* Enable link up/down interrupts and force link re-scan. */
+ if (host->started) {
+ /*
+ * The link re-train in resume triggers the link up IRQ
+ * if there are connected devices. Clear the DL_UpDown
+ * status and trigger link_rescan to avoid
+ * re-enumerating the already connected devices.
+ */
+ writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
+ host->axi + RZG3S_PCI_PEIS0);
+ WRITE_ONCE(host->link_rescan, true);
+ writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
+ host->axi + RZG3S_PCI_PEIE0);
+ irq_wake_thread(host->event_irq, host);
+ }
+ enable_irq(host->event_irq);
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
static int rzg3s_pcie_probe(struct platform_device *pdev)
{
struct pci_host_bridge *bridge;
@@ -2008,15 +2250,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
if (ret)
goto power_resets_assert;
+ host->started = true;
+ host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
+ ret = register_pm_notifier(&host->pm_nb);
+ if (ret)
+ goto host_probe_teardown;
+
bridge->sysdata = host;
bridge->ops = &rzg3s_pcie_root_ops;
bridge->child_ops = &rzg3s_pcie_child_ops;
ret = pci_host_probe(bridge);
if (ret)
- goto host_probe_teardown;
+ goto pm_notifier_unregister;
+
+ pci_lock_rescan_remove();
+ rzg3s_pcie_cache_connected_dev_id(host);
+ pci_unlock_rescan_remove();
+
+ /*
+ * The link re-train triggers the link up IRQ if there are connected
+ * devices. Clear DL_UpDown status and trigger link_rescan to avoid
+ * re-enumerating the devices detected while pci_host_probe() and loose
+ * any potential event.
+ */
+ writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+ WRITE_ONCE(host->link_rescan, true);
+ irq_wake_thread(host->event_irq, host);
+ writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
return 0;
+pm_notifier_unregister:
+ unregister_pm_notifier(&host->pm_nb);
host_probe_teardown:
clk_disable_unprepare(host->port.refclk);
rzg3s_pcie_teardown_irqdomain(host);
@@ -2050,8 +2315,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
static int rzg3s_pcie_resume_noirq(struct device *dev)
{
struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
+ int ret;
+
+ ret = rzg3s_pcie_host_start(host);
+ if (ret)
+ return ret;
+
+ if (rzg3s_pcie_device_replaced(host)) {
+ struct pci_host_bridge *bridge =
+ pci_host_bridge_from_priv(host);
+ struct pci_dev *port __free(pci_dev_put) =
+ pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
- return rzg3s_pcie_host_start(host);
+ if (port && port->subordinate) {
+ pci_walk_bus(port->subordinate,
+ pci_dev_set_disconnected, NULL);
+ }
+ }
+
+ return 0;
}
static const struct dev_pm_ops rzg3s_pcie_pm_ops = {
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes
2026-10-07 5:36 ` [PATCH v6 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes Claudiu Beznea
@ 2026-10-07 5:53 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:53 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: wsa+renesas, linux-pci
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] pci: link event incorrectly removes Root Port instead of endpoint devices
- [High] pci: missing pci_dev_set_disconnected() on link down causes MMIO aborts
- [High] pci: probe error path disables clocks before freeing IRQ
- [High] pci: NULL pointer dereference in rzg3s_pcie_link_event() on spurious IRQ
- [Medium] pm: missing hibernation PM events in notifier causes deadlock
- [Low] cleanup: mixing goto cleanup labels and __free() in rzg3s_pcie_link_event()
--
commit f03b110c046274fb59aa60ec476f662ddada8707
Author: John Madieu <john.madieu.xa@bp.renesas.com>
Subject: PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes
This commit enables PCIe link-state change detection for RZ/G3 and RZ/V2
hardware by hooking into the DL_UpDown interrupt. On link up/down, the driver
removes and rescans devices, adds suspend/resume PM notifiers to avoid
deadlocks, and modifies probe to tolerate absent devices initially.
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index ebe89e6a0796a..c2fbacfe67044 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1105,12 +1119,188 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)
[ ... ]
> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
> +{
> + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> + struct pci_bus *bus = bridge->bus, *child;
> + u32 val;
> + int ret;
> +
> + pci_lock_rescan_remove();
> +
> + val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> + if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> + if (!list_empty(&bus->devices)) {
[Severity: High]
Does this code risk a NULL pointer dereference if a spurious IRQ fires early
in the probe sequence?
If the interrupt fires before pci_host_probe() initializes bridge->bus, then
bus will be NULL here. The IRQ handler rzg3s_pcie_event_irq_thread() does not
mask the raw status with the interrupt enable register, so it might process
events before initialization is complete.
> + dev_info(host->dev,
> + "PCIe link down, removing devices\n");
> + rzg3s_pcie_remove_devices(bus);
[Severity: High]
Does this remove the correct devices? Here bus points to bridge->bus,
which is the system Root Bus (bus 0). Removing devices from the Root Bus
will destroy the SoC's integrated Root Port device itself, rather than the
endpoint devices on the subordinate bus.
[Severity: High]
Also, does this path miss setting the devices as disconnected? Unlike the
device replacement path below, there is no call to pci_dev_set_disconnected()
before removing the devices.
Since the link is down and the hardware is inaccessible, any unprotected MMIO
accesses in the endpoint driver's .remove() callback will fail and trigger
synchronous external aborts.
> + }
> + goto unlock;
> + }
> +
> + if (!list_empty(&bus->devices)) {
> + bool replaced = rzg3s_pcie_device_replaced(host);
> +
[ ... ]
> + if (replaced) {
> + struct pci_dev *port __free(pci_dev_put) =
> + pci_get_slot(bus, PCI_DEVFN(0, 0));
[Severity: Low]
This isn't a bug, but is it acceptable to mix __free() with goto unlock
labels in the same function?
The kernel cleanup guidelines discourage mixing goto-based cleanup labels with
scoped-based cleanups as it can create confusing ownership semantics.
> +
> + if (port && port->subordinate)
> + pci_walk_bus(port->subordinate,
> + pci_dev_set_disconnected, NULL);
> + }
> +
> + rzg3s_pcie_remove_devices(bus);
[Severity: High]
Does this also remove the Root Port instead of the endpoint devices? Similar
to the link down path, bus points to the Root Bus instead of the subordinate
bus.
> + }
[ ... ]
> +unlock:
> + pci_unlock_rescan_remove();
> +}
> +
> +static irqreturn_t rzg3s_pcie_event_irq_thread(int irq, void *data)
> +{
> + struct rzg3s_pcie_host *host = data;
> + bool bounced;
> + u32 status;
> +
> + status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
> + bounced = status & RZG3S_PCI_PEIS0_DL_UPDOWN;
> +
> + if (!bounced && !READ_ONCE(host->link_rescan))
> + return IRQ_NONE;
[Severity: High]
Does this handler need to mask the status against the interrupt enable
register (PEIE0)?
Without it, a spurious interrupt could cause the handler to process a
latched DL_UPDOWN event before it is officially unmasked, potentially
calling rzg3s_pcie_link_event() when bridge->bus is still uninitialized.
> +
> + /* Clear the DL_UpDown status (W1C) */
> + if (bounced)
> + writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> + WRITE_ONCE(host->link_rescan, false);
[ ... ]
> @@ -1916,6 +2123,41 @@ static void rzv2h_pcie_release_lanes(void *data)
[ ... ]
> +static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
> + pm_nb);
> +
> + switch (action) {
> + case PM_SUSPEND_PREPARE:
[Severity: Medium]
Does this switch statement miss the hibernation PM events?
If events like PM_HIBERNATION_PREPARE or PM_RESTORE_PREPARE are not handled,
the link event IRQ could remain unmasked during hibernation. If a link flap
occurs, the handler might run and attempt to acquire locks while devices are
frozen, leading to the deadlock this notifier was designed to prevent.
> + /* Disable link up/down interrupts. */
> + disable_irq(host->event_irq);
> + break;
[ ... ]
> @@ -2008,15 +2250,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
[ ... ]
> +pm_notifier_unregister:
> + unregister_pm_notifier(&host->pm_nb);
> host_probe_teardown:
> clk_disable_unprepare(host->port.refclk);
> rzg3s_pcie_teardown_irqdomain(host);
[Severity: High]
Does this error path disable clocks before freeing the event IRQ?
If the IRQ is still registered and active, it could fire after the reference
clock is gated. The IRQ handler reads the AXI register RZG3S_PCI_PEIS0, and
accessing unclocked AXI registers will typically cause a synchronous external
abort.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=8
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v6 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()
2026-10-07 5:36 [PATCH v6 0/9] PCI: rzg3s-host: Add PCIe hotplug support Claudiu Beznea
` (7 preceding siblings ...)
2026-10-07 5:36 ` [PATCH v6 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes Claudiu Beznea
@ 2026-10-07 5:36 ` Claudiu Beznea
2026-10-07 5:54 ` sashiko-bot
8 siblings, 1 reply; 19+ messages in thread
From: Claudiu Beznea @ 2026-10-07 5:36 UTC (permalink / raw)
To: lpieralisi, kwilczynski, mani, robh, bhelgaas, p.zabel
Cc: claudiu.beznea, linux-pci, linux-kernel, linux-renesas-soc,
Claudiu Beznea
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Add bridge::reset_root_port(). This allows, on some devices, performing
reads and writes to an NVMe endpoint after a link down/link up cycle
without failures.
The implementation of struct pci_host_bridge::reset_root_port() masks all
enabled interrupts and synchronizes them before resetting the controller
to prevent asynchronous events from interfering with the reset operation.
rzg3s_pcie_pm_notifier() disables the link event IRQ, and waits for a
running IRQ thread to complete (through disable_irq()). If that thread is
handling a link down event, it calls .reset_root_port(), which unmasks the
link interrupt before returning, undoing the mask. Therefore, mask the link
interrupt again after disable_irq() has returned.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v6:
- call pci_host_handle_link_down() if at least a link event occured
and there are devices on the bus.
- dropped struct rzg3s_pcie_host::stared from here since it is now
part of patch 8/9
- read the link IRQs in .reset_root_port() and restore them at the
end
- flush the IRQ mask before synchronize()
- in .reset_root_port() clear the link IRQ before enabling it to cope
with the new link handling procedure
- due to these dropped the tags
Changes in v5:
- lock around reads/writes on RZG3S_PCI_PINTRCVIE
- in rzg3s_pcie_host_reset_root_port() kept non-relaxed reads/writes
on interrupt disable registers (even though the spin lock/unlock are around
them) to emphasize the need for updates before the IRQs are synchronized and
relaxed variants on interrupt enable path
- collected tags
Changes in v4:
- this patch is new and is a split from patch 7 of v3
- use a boolean to keep track of the controller start state; with this
the state management code from patch 7 of v3 was dropped
drivers/pci/controller/Kconfig | 1 +
drivers/pci/controller/pcie-rzg3s-host.c | 100 ++++++++++++++++++++++-
2 files changed, 98 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index d246bbe37948..3e2889a7f1a0 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -296,6 +296,7 @@ config PCIE_RENESAS_RZG3S_HOST
depends on ARCH_RENESAS || COMPILE_TEST
select MFD_SYSCON
select IRQ_MSI_LIB
+ select PCI_HOST_COMMON
help
Say Y here if you want PCIe host controller support on Renesas RZ/G3S
SoC.
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index c2fbacfe6704..a8b222625a2a 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1193,8 +1193,27 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
u32 val;
int ret;
+ /*
+ * A latched DL_UpDown means at least one real link transition took
+ * place since the last enumeration. If devices are enumerated
+ * recover the Root Port first.
+ */
+ if (bounced && !list_empty(&bus->devices)) {
+ struct pci_dev *port __free(pci_dev_put) =
+ pci_get_slot(bus, PCI_DEVFN(0, 0));
+
+ if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
+ pci_host_handle_link_down(port);
+ }
+
pci_lock_rescan_remove();
+ /*
+ * Read the link state after the recovery: the .reset_root_port()
+ * retrains the link, so a device that is still present comes back
+ * up here and is re-enumerated right away instead of waiting for
+ * the next DL_UpDown event.
+ */
val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
if (!list_empty(&bus->devices)) {
@@ -1997,7 +2016,7 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
return ret;
}
-static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
+static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host, bool set_started)
{
const struct rzg3s_pcie_soc_data *data = host->data;
struct rzg3s_sysc *sysc = host->sysc;
@@ -2038,7 +2057,8 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
if (ret)
goto assert_power_resets;
- host->started = true;
+ if (set_started)
+ host->started = true;
return 0;
@@ -2053,6 +2073,77 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
return ret;
}
+static int rzg3s_pcie_host_reset_root_port(struct pci_host_bridge *bridge,
+ struct pci_dev *pdev)
+{
+ struct rzg3s_pcie_host *host = pci_host_bridge_priv(bridge);
+ unsigned long flags;
+ u32 irqs, link_irqs;
+ int ret;
+
+ /* Mask link up/down interrupts. */
+ link_irqs = readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
+ writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
+
+ /* Mask INTx and MSI interrupts. */
+ raw_spin_lock_irqsave(&host->hw_lock, flags);
+ irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
+ writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
+ raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+ /* Flush IRQ masks. */
+ readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
+ readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
+
+ /*
+ * Make sure the next operations are not disturbed by any pending
+ * IRQs.
+ */
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ synchronize_irq(host->msi.irq);
+ for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
+ synchronize_irq(host->intx_irqs[i]);
+
+ ret = rzg3s_pcie_host_stop(host);
+ if (ret) {
+ dev_err(host->dev, "Failed to stop the host!\n");
+ goto unmask_irqs;
+ }
+
+ ret = rzg3s_pcie_host_start(host, false);
+ if (ret) {
+ dev_err(host->dev, "Failed to start the host!\n");
+
+ /*
+ * Don't unmask IRQs. We are in a bad state here and we
+ * can recover only through a suspend/resume cycle. Just
+ * return and preserve the stop state.
+ */
+ return ret;
+ }
+
+unmask_irqs:
+ /* Unmask INTx and MSI interrupts. */
+ raw_spin_lock_irqsave(&host->hw_lock, flags);
+ writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
+ raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+ /*
+ * rzg3s_pcie_host_start() retrains the link. If the link comes
+ * back up, DL_UpDown gets latched in PEIS0. Clear the latch.
+ * The link state is evaluated after this function returns, so
+ * the event is not lost. For the same reason there is no need
+ * to set host->link_rescan.
+ */
+ writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+ /* Unmask link up/down interrupts. */
+ writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);
+
+ host->started = true;
+
+ return ret;
+}
+
static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
{
struct device_node *np = host->dev->of_node;
@@ -2133,6 +2224,8 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
case PM_SUSPEND_PREPARE:
/* Disable link up/down interrupts. */
disable_irq(host->event_irq);
+ if (host->started)
+ writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
break;
case PM_POST_SUSPEND:
@@ -2259,6 +2352,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
bridge->sysdata = host;
bridge->ops = &rzg3s_pcie_root_ops;
bridge->child_ops = &rzg3s_pcie_child_ops;
+ bridge->reset_root_port = rzg3s_pcie_host_reset_root_port;
ret = pci_host_probe(bridge);
if (ret)
goto pm_notifier_unregister;
@@ -2317,7 +2411,7 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
int ret;
- ret = rzg3s_pcie_host_start(host);
+ ret = rzg3s_pcie_host_start(host, true);
if (ret)
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v6 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()
2026-10-07 5:36 ` [PATCH v6 9/9] PCI: rzg3s-host: Add bridge::reset_root_port() Claudiu Beznea
@ 2026-10-07 5:54 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-10-07 5:54 UTC (permalink / raw)
To: Claudiu Beznea; +Cc: linux-pci, wsa+renesas
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] pci: rzg3s-host: Missing sysfs/AER vs event_irq synchronization causing AXI abort
- [High] pci: PINTRCVIE INTx/MSI mask overwritten after unlocked hardware reset
- [Medium] pci: goto-based error handling mixed with __free() scope cleanups
- [Medium] pcie: rzg3s: data race on PEIE0 register during concurrent sysfs reset and suspend
--
commit 52776c6784c4d35d5a362a3b52bff852eb31f68f
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
PCI: rzg3s-host: Add bridge::reset_root_port()
This commit adds the bridge::reset_root_port() implementation to allow
reads and writes to an NVMe endpoint after a link down/up cycle. It
masks all enabled interrupts, resets the controller, and synchronizes
IRQs to prevent asynchronous events from interfering with the reset.
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index c2fbacfe67044..a8b222625a2a6 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1193,8 +1193,27 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
> u32 val;
> int ret;
>
> + /*
> + * A latched DL_UpDown means at least one real link transition took
> + * place since the last enumeration. If devices are enumerated
> + * recover the Root Port first.
> + */
> + if (bounced && !list_empty(&bus->devices)) {
> + struct pci_dev *port __free(pci_dev_put) =
> + pci_get_slot(bus, PCI_DEVFN(0, 0));
[Severity: Medium]
This adds a __free() variable to a function that relies on goto-based error
handling (e.g. goto unlock; later in this function).
The global cleanup guidelines state that goto and cleanup helpers should never
be mixed in the same function. Should all resources in this function be
converted to scope-based cleanup, or should we continue using traditional
cleanup here?
> +
> + if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> + pci_host_handle_link_down(port);
> + }
> +
> pci_lock_rescan_remove();
>
> + /*
> + * Read the link state after the recovery: the .reset_root_port()
> + * retrains the link, so a device that is still present comes back
> + * up here and is re-enumerated right away instead of waiting for
> + * the next DL_UpDown event.
> + */
> val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
[Severity: Critical]
Can reading this AXI register cause a synchronous external abort (kernel panic)
if a concurrent sysfs/AER thread calls rzg3s_pcie_host_stop() and disables
the clocks?
Since rzg3s_pcie_host_reset_root_port() intentionally omits
synchronize_irq(host->event_irq), and no other locks are held here to
exclude the sysfs reset path, it seems this thread can read the register
while power resets are asserted.
> if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> if (!list_empty(&bus->devices)) {
[ ... ]
> @@ -2053,6 +2073,77 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
> return ret;
> }
>
> +static int rzg3s_pcie_host_reset_root_port(struct pci_host_bridge *bridge,
> + struct pci_dev *pdev)
> +{
> + struct rzg3s_pcie_host *host = pci_host_bridge_priv(bridge);
> + unsigned long flags;
> + u32 irqs, link_irqs;
> + int ret;
> +
> + /* Mask link up/down interrupts. */
> + link_irqs = readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> + writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +
> + /* Mask INTx and MSI interrupts. */
> + raw_spin_lock_irqsave(&host->hw_lock, flags);
> + irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> + writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
> + raw_spin_unlock_irqrestore(&host->hw_lock, flags);
[Severity: High]
This caches the RZG3S_PCI_PINTRCVIE register in the local variable irqs,
then drops the lock.
If an endpoint driver concurrently calls request_irq(), free_irq(), or
enable_irq() for an INTx/MSI interrupt during the unlocked window below,
won't the stale irqs value be written back later, permanently losing or
causing spurious IRQ masks?
> +
> + /* Flush IRQ masks. */
> + readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> + readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +
> + /*
> + * Make sure the next operations are not disturbed by any pending
> + * IRQs.
> + */
> + if (IS_ENABLED(CONFIG_PCI_MSI))
> + synchronize_irq(host->msi.irq);
> + for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> + synchronize_irq(host->intx_irqs[i]);
> +
> + ret = rzg3s_pcie_host_stop(host);
> + if (ret) {
> + dev_err(host->dev, "Failed to stop the host!\n");
> + goto unmask_irqs;
> + }
> +
> + ret = rzg3s_pcie_host_start(host, false);
> + if (ret) {
> + dev_err(host->dev, "Failed to start the host!\n");
> +
> + /*
> + * Don't unmask IRQs. We are in a bad state here and we
> + * can recover only through a suspend/resume cycle. Just
> + * return and preserve the stop state.
> + */
> + return ret;
> + }
> +
> +unmask_irqs:
> + /* Unmask INTx and MSI interrupts. */
> + raw_spin_lock_irqsave(&host->hw_lock, flags);
> + writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
[Severity: High]
This restores the potentially stale irqs local variable, overwriting
any mask changes made concurrently by the IRQ subsystem while the lock
was dropped.
> + raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> + /*
> + * rzg3s_pcie_host_start() retrains the link. If the link comes
> + * back up, DL_UpDown gets latched in PEIS0. Clear the latch.
> + * The link state is evaluated after this function returns, so
> + * the event is not lost. For the same reason there is no need
> + * to set host->link_rescan.
> + */
> + writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> + /* Unmask link up/down interrupts. */
> + writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);
[Severity: Medium]
If a user concurrently triggers a sysfs reset while the system is
entering suspend, can this unconditionally restore an unmasked PEIE0 state
and overwrite the interrupt mask set by PM_SUSPEND_PREPARE?
Looking at rzg3s_pcie_pm_notifier(), it executes in PM_SUSPEND_PREPARE and
zeroes PEIE0 to mask interrupts for system sleep. A concurrent sysfs reset
will read the unmasked state, complete the reset, and blindly write the
unmasked state back, leaving interrupts unmasked during suspend and
potentially causing wake-up or power domain transition failures.
> +
> + host->started = true;
> +
> + return ret;
> +}
[ ... ]
> @@ -2133,6 +2224,8 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
> case PM_SUSPEND_PREPARE:
> /* Disable link up/down interrupts. */
> disable_irq(host->event_irq);
> + if (host->started)
> + writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
[Severity: Medium]
This zeroes PEIE0 for system sleep, but a racing sysfs reset in
rzg3s_pcie_host_reset_root_port() can unconditionally overwrite this
state with its saved unmasked value.
> break;
>
> case PM_POST_SUSPEND:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261007053645.1391545-1-claudiu.beznea@kernel.org?part=9
^ permalink raw reply [flat|nested] 19+ messages in thread