[PATCH v5 0/9] PCI: rzg3s-host: Add PCIe hotplug support

[PATCH v5 0/9] PCI: rzg3s-host: Add PCIe hotplug support

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds hotplug support for the rzg3s-host driver. This is a
continuation of the series John posted initially at [1]. Along with it,
prerequisites fixes and cleanup patches were added.

Thank you,
Claudiu

Changes in v5:
- collected tags
- added patch 3/9 ("PCI: rzg3s-host: Select PCI_HOST_COMMON")
- addressed sashiko review comments (detailed in individual patches)

Changes in v4:
- simplified comments in patch 2/8
- kept only DL_UpDown changes in patch 7/8
- added patch 8/8 for the .reset_root_port() support

Changes in v3:
- added patches 1-6
- re-worked patch 7 to use struct pci_host_bridge::reset_root_port() API

[1] https://lore.kernel.org/all/20260630141720.3938514-1-john.madieu.xa@bp.renesas.com/

Claudiu Beznea (8):
  PCI: rzg3s-host: Follow hardware manual clock/reset initialization
    order
  PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume
    phase
  PCI: rzg3s-host: Select PCI_HOST_COMMON
  PCI: rzg3s-host: Drop nop instructions
  PCI: rzg3s-host: Move host configuration code together
  PCI: rzg3s-host: Move suspend/resume code into dedicated functions
  PCI: rzg3s-host: Move IRQ domain setup code
  PCI: rzg3s-host: Add bridge::reset_root_port()

John Madieu (1):
  PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

 drivers/pci/controller/Kconfig           |   1 +
 drivers/pci/controller/pcie-rzg3s-host.c | 901 ++++++++++++++---------
 2 files changed, 574 insertions(+), 328 deletions(-)

-- 
2.43.0

[PATCH v5 1/9] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order

[Claudiu Beznea]

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 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

Re: [PATCH v5 1/9] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=1

[PATCH v5 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase

[Claudiu Beznea]

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 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

Re: [PATCH v5 2/9] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase

[sashiko-bot]

> 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/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=2

[PATCH v5 3/9] PCI: rzg3s-host: Select PCI_HOST_COMMON

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pci_host_common_link_train_delay() depends on PCI_HOST_COMMON. Select it
to avoid any build failures.

Fixes: 2b0d1a605ef2 ("PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none, this patch is new

 drivers/pci/controller/Kconfig | 1 +
 1 file changed, 1 insertion(+)

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.
-- 
2.43.0

Re: [PATCH v5 3/9] PCI: rzg3s-host: Select PCI_HOST_COMMON

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Kconfig: unnecessary selection of PCI_HOST_COMMON causes kernel bloat
--

commit 45902cd2b000fac7ca52cf6e5a8c266554c39e27
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

PCI: rzg3s-host: Select PCI_HOST_COMMON

This commit adds a select for PCI_HOST_COMMON to the PCIE_RENESAS_RZG3S_HOST
kconfig option. It states this is needed for pci_host_common_link_train_delay()
to avoid build failures.

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index d246bbe37948e..3e2889a7f1a09 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

[Severity: Medium]
Is this select actually required for the build?

The commit message notes that pci_host_common_link_train_delay() depends on
PCI_HOST_COMMON. However, looking at the header,
pci_host_common_link_train_delay() is implemented as a static inline function
entirely within pci-host-common.h and only requires linux/delay.h. There
doesn't appear to be any link dependency on pci-host-common.c.

Since selecting a tristate from a bool config forces PCI_HOST_COMMON and
PCI_ECAM to be built-in rather than modules, could this be pulling in
unneeded infrastructure and unnecessarily increasing the vmlinux size?

>  	help
>  	  Say Y here if you want PCIe host controller support on Renesas RZ/G3S
>  	  SoC.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=3

Re: [PATCH v5 3/9] PCI: rzg3s-host: Select PCI_HOST_COMMON

[Claudiu Beznea]

On 10/2/26 14:22, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Kconfig: unnecessary selection of PCI_HOST_COMMON causes kernel bloat
> --
> 
> commit 45902cd2b000fac7ca52cf6e5a8c266554c39e27
> Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> PCI: rzg3s-host: Select PCI_HOST_COMMON
> 
> This commit adds a select for PCI_HOST_COMMON to the PCIE_RENESAS_RZG3S_HOST
> kconfig option. It states this is needed for pci_host_common_link_train_delay()
> to avoid build failures.
> 
>> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
>> index d246bbe37948e..3e2889a7f1a09 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
> 
> [Severity: Medium]
> Is this select actually required for the build?
> 
> The commit message notes that pci_host_common_link_train_delay() depends on
> PCI_HOST_COMMON. However, looking at the header,
> pci_host_common_link_train_delay() is implemented as a static inline function
> entirely within pci-host-common.h and only requires linux/delay.h. There
> doesn't appear to be any link dependency on pci-host-common.c.

That's true. I did a mistake here. I should have select this flag on patch 9 
which uses pci_host_handle_link_down().

> 
> Since selecting a tristate from a bool config forces PCI_HOST_COMMON and
> PCI_ECAM to be built-in rather than modules, could this be pulling in
> unneeded infrastructure and unnecessarily increasing the vmlinux size?
> 
>>   	help
>>   	  Say Y here if you want PCIe host controller support on Renesas RZ/G3S
>>   	  SoC.
> 

[PATCH v5 4/9] PCI: rzg3s-host: Drop nop instructions

[Claudiu Beznea]

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 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

Re: [PATCH v5 4/9] PCI: rzg3s-host: Drop nop instructions

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=4

[PATCH v5 5/9] PCI: rzg3s-host: Move host configuration code together

[Claudiu Beznea]

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 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

Re: [PATCH v5 5/9] PCI: rzg3s-host: Move host configuration code together

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=5

[PATCH v5 6/9] PCI: rzg3s-host: Move suspend/resume code into dedicated functions

[Claudiu Beznea]

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 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

Re: [PATCH v5 6/9] PCI: rzg3s-host: Move suspend/resume code into dedicated functions

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=6

[PATCH v5 7/9] PCI: rzg3s-host: Move IRQ domain setup code

[Claudiu Beznea]

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 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

Re: [PATCH v5 7/9] PCI: rzg3s-host: Move IRQ domain setup code

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=7

[PATCH v5 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[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;
  - 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_host_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.

While at it, make probe tolerant of an absent device. Previously, if the
link failed to come up during rzg3s_pcie_host_init(), probe tore the
controller back down and failed. Distinguish this case with -ENODEV,
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 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 | 193 +++++++++++++++++++++--
 1 file changed, 177 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 3b62b1be5b2a..78e783928b9d 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,12 @@ 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)
+ * @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
  * @num_lanes: The number of lanes
  */
 struct rzg3s_pcie_host {
@@ -340,9 +345,12 @@ struct rzg3s_pcie_host {
 	struct rzg3s_pcie_msi msi;
 	struct rzg3s_pcie_port port;
 	raw_spinlock_t hw_lock;
+	struct notifier_block pm_nb;
+	int event_irq;
 	int intx_irqs[PCI_NUM_INTX];
 	int max_link_speed;
 	enum rzg3s_pcie_controller_id controller_id;
+	bool link_rescan;
 	u8 num_lanes;
 };
 
@@ -1095,6 +1103,98 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
+static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
+	struct pci_bus *bus = bridge->bus;
+	u32 val;
+
+	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
+	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
+		struct pci_dev *dev, *tmp;
+
+		dev_info(host->dev, "PCIe link down, removing devices\n");
+
+		pci_lock_rescan_remove();
+		list_for_each_entry_safe_reverse(dev, tmp, &bus->devices,
+						 bus_list)
+			pci_stop_and_remove_bus_device(dev);
+		pci_unlock_rescan_remove();
+	} else {
+		struct pci_bus *child;
+		int ret;
+
+		dev_info(host->dev, "PCIe link up, rescanning bus\n");
+
+		/*
+		 * Attempt link speed negotiation now that the link is up.
+		 * Failure is non-fatal: the device works at the negotiated
+		 * speed.
+		 */
+		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_lock_rescan_remove();
+		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);
+		pci_unlock_rescan_remove();
+		pm_runtime_put_sync(&bridge->dev);
+	}
+}
+
+static irqreturn_t rzg3s_pcie_event_irq_thread(int irq, void *data)
+{
+	struct rzg3s_pcie_host *host = data;
+	u32 status;
+
+	status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
+
+	if (!(status & RZG3S_PCI_PEIS0_DL_UPDOWN) &&
+	    !READ_ONCE(host->link_rescan))
+		return IRQ_NONE;
+
+	/* Clear the DL_UpDown status (W1C) */
+	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+	WRITE_ONCE(host->link_rescan, false);
+
+	rzg3s_pcie_link_event(host);
+
+	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;
+}
+
 static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
 {
 	while (--count >= 0) {
@@ -1106,6 +1206,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;
@@ -1152,22 +1263,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);
@@ -1679,16 +1789,21 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
 				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
 				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
 				 PCIE_LINK_WAIT_MAX_RETRIES);
-	if (ret)
-		goto config_deinit_post;
+	if (ret) {
+		/*
+		 * Link is down. Leave the controller running so the
+		 * DL_UpDown handler can enumerate a device that appears
+		 * later.
+		 */
+		dev_info(host->dev, "PCIe link down, waiting for DL_UpDown\n");
+		ret = -ENODEV;
+	}
 
 	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
 	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
 
-	return 0;
+	return ret;
 
-config_deinit_post:
-	host->data->config_deinit(host);
 config_deinit_and_refclk:
 	clk_disable_unprepare(host->port.refclk);
 config_deinit:
@@ -1723,8 +1838,14 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 
 	ret = rzg3s_pcie_host_init(host);
 	if (ret) {
-		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
-		goto teardown_irqdomain;
+		if (ret != -ENODEV) {
+			dev_err_probe(dev, ret,
+				      "Failed to initialize the HW!\n");
+			goto teardown_irqdomain;
+		}
+
+		/* Link is down: hotplug via DL_UpDown will recover. */
+		return 0;
 	}
 
 	ret = rzg3s_pcie_set_max_link_speed(host);
@@ -1905,6 +2026,31 @@ 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. */
+		writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
+			       host->axi + RZG3S_PCI_PEIE0);
+		WRITE_ONCE(host->link_rescan, true);
+		enable_irq(host->event_irq);
+		irq_wake_thread(host->event_irq, host);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int rzg3s_pcie_probe(struct platform_device *pdev)
 {
 	struct pci_host_bridge *bridge;
@@ -1997,15 +2143,30 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto power_resets_assert;
 
+	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;
+
+	/*
+	 * Unmask the PCIe event IRQ at the end of probe to avoid
+	 * spurious link-state events during controller setup and bus
+	 * enumeration. From here on, DL_UpDown events trigger the link
+	 * IRQ thread to (re)scan the bus.
+	 */
+	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);
-- 
2.43.0

Re: [PATCH v5 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] PCI: deadlock during hibernation due to unhandled PM notifiers
- [High] PCI: probe succeeds with uninitialized controller on DT error
- [High] PCI: NULL pointer dereference if event IRQ fires before root bus creation
- [Medium] PCI: ghost devices left on bus if link bounces during suspend
--

commit e916ae87086b955078f323f09316792067184bfe
Author: John Madieu <john.madieu.xa@bp.renesas.com>
Subject: PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit adds support for reacting to link-state changes by handling
the DL_UpDown signal on RZ/G3 and RZ/V2 PCIe controllers. It introduces
a link event interrupt, hotplug-like logic, and PM notifiers to serialize
with device suspend/resume.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 3b62b1be5b2a1..78e783928b9dc 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1095,6 +1103,98 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
>  	return ret;
>  }
>  
> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> +	struct pci_bus *bus = bridge->bus;
> +	u32 val;
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		struct pci_dev *dev, *tmp;
> +
> +		dev_info(host->dev, "PCIe link down, removing devices\n");
> +
> +		pci_lock_rescan_remove();
> +		list_for_each_entry_safe_reverse(dev, tmp, &bus->devices,

[Severity: High]
The event IRQ is requested and enabled in rzg3s_pcie_host_setup() before
bridge->bus is allocated by pci_host_probe().

If the hardware generates a spurious interrupt during this narrow window,
bridge->bus could be NULL when this handler runs.

Will this dereference a NULL pointer?

> +						 bus_list)
> +			pci_stop_and_remove_bus_device(dev);
> +		pci_unlock_rescan_remove();
> +	} else {
> +		struct pci_bus *child;
> +		int ret;
> +
> +		dev_info(host->dev, "PCIe link up, rescanning bus\n");

[Severity: Medium]
On resume, the PM notifier wakes the IRQ thread.

If the link is currently up, it seems this branch is taken and rescans the bus
without removing the old devices that might have been hot-swapped while the
link was down.

Could this leave ghost devices on the bus if the link goes down and comes back
up while the system is suspended?

[ ... ]
> @@ -1723,8 +1838,14 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>  
>  	ret = rzg3s_pcie_host_init(host);
>  	if (ret) {
> -		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
> -		goto teardown_irqdomain;
> +		if (ret != -ENODEV) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to initialize the HW!\n");
> +			goto teardown_irqdomain;
> +		}
> +
> +		/* Link is down: hotplug via DL_UpDown will recover. */
> +		return 0;

[Severity: High]
If a mandatory resource is missing, such as resource_list_first_type()
returning NULL in rzg3s_pcie_config_init(), it returns -ENODEV.

This would be caught here as a safe link-down state and return success,
potentially leaving the controller uninitialized.

Does this pattern mask genuine configuration errors?

[ ... ]
> @@ -1905,6 +2026,31 @@ 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;

[Severity: High]
The switch statement only handles PM_SUSPEND_PREPARE and PM_POST_SUSPEND,
completely omitting PM_HIBERNATION_PREPARE and PM_RESTORE_PREPARE.

If a link event occurs during hibernation transitions, the threaded interrupt
could attempt to acquire the same lock the PM core is holding.

Can this missing hibernation support open a window for a deadlock?

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +			       host->axi + RZG3S_PCI_PEIE0);
> +		WRITE_ONCE(host->link_rescan, true);
> +		enable_irq(host->event_irq);
> +		irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=8

Re: [PATCH v5 8/9] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[Claudiu Beznea]

On 10/2/26 14:25, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] PCI: deadlock during hibernation due to unhandled PM notifiers
> - [High] PCI: probe succeeds with uninitialized controller on DT error
> - [High] PCI: NULL pointer dereference if event IRQ fires before root bus creation
> - [Medium] PCI: ghost devices left on bus if link bounces during suspend
> --
> 
> commit e916ae87086b955078f323f09316792067184bfe
> Author: John Madieu <john.madieu.xa@bp.renesas.com>
> Subject: PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes
> 
> This commit adds support for reacting to link-state changes by handling
> the DL_UpDown signal on RZ/G3 and RZ/V2 PCIe controllers. It introduces
> a link event interrupt, hotplug-like logic, and PM notifiers to serialize
> with device suspend/resume.
> 
>> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
>> index 3b62b1be5b2a1..78e783928b9dc 100644
>> --- a/drivers/pci/controller/pcie-rzg3s-host.c
>> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> [ ... ]
>> @@ -1095,6 +1103,98 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
>>   	return ret;
>>   }
>>   
>> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
>> +{
>> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
>> +	struct pci_bus *bus = bridge->bus;
>> +	u32 val;
>> +
>> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
>> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
>> +		struct pci_dev *dev, *tmp;
>> +
>> +		dev_info(host->dev, "PCIe link down, removing devices\n");
>> +
>> +		pci_lock_rescan_remove();
>> +		list_for_each_entry_safe_reverse(dev, tmp, &bus->devices,
> 
> [Severity: High]
> The event IRQ is requested and enabled in rzg3s_pcie_host_setup() before
> bridge->bus is allocated by pci_host_probe().
> 
> If the hardware generates a spurious interrupt during this narrow window,
> bridge->bus could be NULL when this handler runs.
> 
> Will this dereference a NULL pointer?

That shouldn't happen. The initialization masks the link interrupt at controller 
level. The interrupt is unmasked at the controller level only at the end of the 
probe, after bridge->bus becomes valid.

> 
>> +						 bus_list)
>> +			pci_stop_and_remove_bus_device(dev);
>> +		pci_unlock_rescan_remove();
>> +	} else {
>> +		struct pci_bus *child;
>> +		int ret;
>> +
>> +		dev_info(host->dev, "PCIe link up, rescanning bus\n");
> 
> [Severity: Medium]
> On resume, the PM notifier wakes the IRQ thread.
> 
> If the link is currently up, it seems this branch is taken and rescans the bus
> without removing the old devices that might have been hot-swapped while the
> link was down.
> 
> Could this leave ghost devices on the bus if the link goes down and comes back
> up while the system is suspended?

Yes, it could. I'll update it in a similar way it is done in pciehp_core.c


> 
> [ ... ]
>> @@ -1723,8 +1838,14 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>>   
>>   	ret = rzg3s_pcie_host_init(host);
>>   	if (ret) {
>> -		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
>> -		goto teardown_irqdomain;
>> +		if (ret != -ENODEV) {
>> +			dev_err_probe(dev, ret,
>> +				      "Failed to initialize the HW!\n");
>> +			goto teardown_irqdomain;
>> +		}
>> +
>> +		/* Link is down: hotplug via DL_UpDown will recover. */
>> +		return 0;
> 
> [Severity: High]
> If a mandatory resource is missing, such as resource_list_first_type()
> returning NULL in rzg3s_pcie_config_init(), it returns -ENODEV.
> 
> This would be caught here as a safe link-down state and return success,
> potentially leaving the controller uninitialized.
> 
> Does this pattern mask genuine configuration errors?


That is valid. I'll update it.

> 
> [ ... ]
>> @@ -1905,6 +2026,31 @@ 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;
> 
> [Severity: High]
> The switch statement only handles PM_SUSPEND_PREPARE and PM_POST_SUSPEND,
> completely omitting PM_HIBERNATION_PREPARE and PM_RESTORE_PREPARE.
> 
> If a link event occurs during hibernation transitions, the threaded interrupt
> could attempt to acquire the same lock the PM core is holding.
> 
> Can this missing hibernation support open a window for a deadlock?

The hibernation wasn't treated since it is currently unavailable on any of the 
SoCs supporting this driver.

> 
>> +
>> +	case PM_POST_SUSPEND:
>> +		/* Enable link up/down interrupts and force link re-scan. */
>> +		writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
>> +			       host->axi + RZG3S_PCI_PEIE0);
>> +		WRITE_ONCE(host->link_rescan, true);
>> +		enable_irq(host->event_irq);
>> +		irq_wake_thread(host->event_irq, host);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
> 

[PATCH v5 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()

[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.

struct rzg3s_pcie_host::started boolean was added to keep track of the
controller state, since rzg3s_pcie_host_start() and rzg3s_pcie_host_stop()
are called from multiple paths: .reset_root_port(), .suspend_noirq() and
.resume_noirq(). The flag prevents double start/stop and breaking
reference counters for clocks and resets.

rzg3s_pcie_pm_notifier() disables the link event IRQ, and disable_irq()
waits for a running IRQ thread to complete. 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.

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 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/pcie-rzg3s-host.c | 83 +++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 78e783928b9d..eea215818633 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -331,6 +331,7 @@ struct rzg3s_pcie_port {
  * @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 {
@@ -351,6 +352,7 @@ struct rzg3s_pcie_host {
 	int max_link_speed;
 	enum rzg3s_pcie_controller_id controller_id;
 	bool link_rescan;
+	bool started;
 	u8 num_lanes;
 };
 
@@ -1115,6 +1117,11 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
 
 		dev_info(host->dev, "PCIe link down, removing devices\n");
 
+		for_each_pci_bridge(dev, bridge->bus) {
+			if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
+				pci_host_handle_link_down(dev);
+		}
+
 		pci_lock_rescan_remove();
 		list_for_each_entry_safe_reverse(dev, tmp, &bus->devices,
 						 bus_list)
@@ -1869,6 +1876,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 */
@@ -1890,6 +1900,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 */
@@ -1905,12 +1917,15 @@ 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;
 	int ret;
 
+	if (host->started)
+		return 0;
+
 	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
 	if (ret)
 		return ret;
@@ -1943,6 +1958,9 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
 	if (ret)
 		goto assert_power_resets;
 
+	if (set_started)
+		host->started = true;
+
 	return 0;
 
 	/*
@@ -1956,6 +1974,64 @@ 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;
+	int ret;
+
+	/* Mask link up/down interrupts. */
+	writel(0, host->axi + RZG3S_PCI_PEIE0);
+
+	/* Mask INTx and MSI interrupts. */
+	raw_spin_lock_irqsave(&host->hw_lock, flags);
+	irqs = readl(host->axi + RZG3S_PCI_PINTRCVIE);
+	writel(0, host->axi + RZG3S_PCI_PINTRCVIE);
+	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+	/*
+	 * 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);
+
+	/* Unmask link up/down interrupts. */
+	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, 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;
@@ -2036,6 +2112,7 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
 	case PM_SUSPEND_PREPARE:
 		/* Disable link up/down interrupts. */
 		disable_irq(host->event_irq);
+		writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
 		break;
 
 	case PM_POST_SUSPEND:
@@ -2143,6 +2220,7 @@ 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)
@@ -2151,6 +2229,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;
@@ -2201,7 +2280,7 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 {
 	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
 
-	return rzg3s_pcie_host_start(host);
+	return rzg3s_pcie_host_start(host, true);
 }
 
 static const struct dev_pm_ops rzg3s_pcie_pm_ops = {
-- 
2.43.0

Re: [PATCH v5 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pci: rzg3s: INTx/MSI mask state loss due to stale register snapshot
- [High] pci: rzg3s: fatal unpowered MMIO access in PM notifier after reset failure
- [High] pci: rzg3s: lockless iteration over bus->devices causes list corruption
- [Medium] pci: rzg3s: missing I/O flush allows IRQs to fire after synchronize_irq
--

commit c5ffdc83b56df877b7b137c1229cfe9487c3fb34
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: PCI: rzg3s-host: Add bridge::reset_root_port()

This commit adds a reset_root_port() callback for the rzg3s-host bridge.
It introduces the started boolean to track the controller state and prevents
double start/stop from breaking reference counters. It also updates the link
event and power management notifier paths to handle the interrupts correctly.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 78e783928b9dc..eea215818633b 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1115,6 +1117,11 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
>  
>  		dev_info(host->dev, "PCIe link down, removing devices\n");
>  
> +		for_each_pci_bridge(dev, bridge->bus) {
> +			if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> +				pci_host_handle_link_down(dev);
> +		}
> +
>  		pci_lock_rescan_remove();

[Severity: High]
Is it safe to iterate over bus->devices without holding the PCI
rescan/remove lock?

It looks like concurrent modifications to the list from sysfs rescan or
hotplug events could cause list corruption or a use-after-free. The
necessary lock is correctly taken immediately after this loop for the removal
step, but appears to be omitted for this iteration.

[ ... ]
> @@ -1956,6 +1974,64 @@ 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;
> +	int ret;
> +
> +	/* Mask link up/down interrupts. */
> +	writel(0, host->axi + RZG3S_PCI_PEIE0);
> +
> +	/* Mask INTx and MSI interrupts. */
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	irqs = readl(host->axi + RZG3S_PCI_PINTRCVIE);
> +	writel(0, host->axi + RZG3S_PCI_PINTRCVIE);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	/*
> +	 * Make sure the next operations are not disturbed by any pending
> +	 * IRQs.
> +	 */
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		synchronize_irq(host->msi.irq);

[Severity: Medium]
Does the posted write to the interrupt mask register need to be flushed
with a dummy read before calling synchronize_irq()?

Without a read-back to ensure the mask has reached the physical hardware, the
controller might still emit an interrupt while synchronize_irq() runs,
potentially creating a race window where the handler crashes when the
controller clocks are subsequently cut in rzg3s_pcie_host_stop().

> +	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);

[Severity: High]
Could this unconditional restore overwrite mask changes made by concurrent
INTx mask/unmask operations?

Since hw_lock was dropped earlier to wait for IRQs and execute the
controller stop/start sequence, any intermediate updates to the register by
other drivers dynamically masking or unmasking their IRQs might be
permanently lost here when the old snapshot is restored.

> +
> +	/* Unmask link up/down interrupts. */
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +
> +	host->started = true;
> +
> +	return ret;
> +}
[ ... ]
> @@ -2036,6 +2112,7 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  	case PM_SUSPEND_PREPARE:
>  		/* Disable link up/down interrupts. */
>  		disable_irq(host->event_irq);
> +		writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);

[Severity: High]
Could this write cause a synchronous external abort if the controller is
currently unpowered?

If rzg3s_pcie_host_start() fails inside rzg3s_pcie_host_reset_root_port(),
the function intentionally returns early, leaving host->started false and
the controller in reset with clocks disabled. A subsequent system suspend
would unconditionally hit this AXI write, which might hang the bus since
the target peripheral is held in reset.

>  		break;
>  
>  	case PM_POST_SUSPEND:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=9

Re: [PATCH v5 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()

[Claudiu Beznea]

On 10/2/26 14:25, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] pci: rzg3s: INTx/MSI mask state loss due to stale register snapshot
> - [High] pci: rzg3s: fatal unpowered MMIO access in PM notifier after reset failure
> - [High] pci: rzg3s: lockless iteration over bus->devices causes list corruption
> - [Medium] pci: rzg3s: missing I/O flush allows IRQs to fire after synchronize_irq
> --
> 
> commit c5ffdc83b56df877b7b137c1229cfe9487c3fb34
> Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Subject: PCI: rzg3s-host: Add bridge::reset_root_port()
> 
> This commit adds a reset_root_port() callback for the rzg3s-host bridge.
> It introduces the started boolean to track the controller state and prevents
> double start/stop from breaking reference counters. It also updates the link
> event and power management notifier paths to handle the interrupts correctly.
> 
>> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
>> index 78e783928b9dc..eea215818633b 100644
>> --- a/drivers/pci/controller/pcie-rzg3s-host.c
>> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> [ ... ]
>> @@ -1115,6 +1117,11 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
>>   
>>   		dev_info(host->dev, "PCIe link down, removing devices\n");
>>   
>> +		for_each_pci_bridge(dev, bridge->bus) {
>> +			if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>> +				pci_host_handle_link_down(dev);
>> +		}
>> +
>>   		pci_lock_rescan_remove();
> 
> [Severity: High]
> Is it safe to iterate over bus->devices without holding the PCI
> rescan/remove lock?

Doing this will lead to deadlock on the following calls:
pci_host_handle_link_down() ->
   pcie_do_recovery() ->
     pci_host_reset_root_port() ->
       pci_lock_rescan_remove()

> 
> It looks like concurrent modifications to the list from sysfs rescan or
> hotplug events could cause list corruption or a use-after-free. The
> necessary lock is correctly taken immediately after this loop for the removal
> step, but appears to be omitted for this iteration.

I kept it as is to align with the current usage of pci_host_handle_link_down() 
in other drivers.

> 
> [ ... ]
>> @@ -1956,6 +1974,64 @@ 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;
>> +	int ret;
>> +
>> +	/* Mask link up/down interrupts. */
>> +	writel(0, host->axi + RZG3S_PCI_PEIE0);
>> +
>> +	/* Mask INTx and MSI interrupts. */
>> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
>> +	irqs = readl(host->axi + RZG3S_PCI_PINTRCVIE);
>> +	writel(0, host->axi + RZG3S_PCI_PINTRCVIE);
>> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
>> +
>> +	/*
>> +	 * Make sure the next operations are not disturbed by any pending
>> +	 * IRQs.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>> +		synchronize_irq(host->msi.irq);
> 
> [Severity: Medium]
> Does the posted write to the interrupt mask register need to be flushed
> with a dummy read before calling synchronize_irq()?

OK

> 
> Without a read-back to ensure the mask has reached the physical hardware, the
> controller might still emit an interrupt while synchronize_irq() runs,
> potentially creating a race window where the handler crashes when the
> controller clocks are subsequently cut in rzg3s_pcie_host_stop().
> 
>> +	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);
> 
> [Severity: High]
> Could this unconditional restore overwrite mask changes made by concurrent
> INTx mask/unmask operations?
> 
> Since hw_lock was dropped earlier to wait for IRQs and execute the
> controller stop/start sequence, any intermediate updates to the register by
> other drivers dynamically masking or unmasking their IRQs might be
> permanently lost here when the old snapshot is restored.

[1] had the mechanism to avoid this. It was discussed (not exactly on this 
particular use case) to drop it.

[1] 
https://lore.kernel.org/all/n7cafny3y6rr63tx6rkt5umxl3f4rsxpu2ojze7luudzzr4flb@egj7dru2bnhq

> 
>> +
>> +	/* Unmask link up/down interrupts. */
>> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
>> +
>> +	host->started = true;
>> +
>> +	return ret;
>> +}
> [ ... ]
>> @@ -2036,6 +2112,7 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>>   	case PM_SUSPEND_PREPARE:
>>   		/* Disable link up/down interrupts. */
>>   		disable_irq(host->event_irq);
>> +		writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> 
> [Severity: High]
> Could this write cause a synchronous external abort if the controller is
> currently unpowered?
> 
> If rzg3s_pcie_host_start() fails inside rzg3s_pcie_host_reset_root_port(),
> the function intentionally returns early, leaving host->started false and
> the controller in reset with clocks disabled. A subsequent system suspend
> would unconditionally hit this AXI write, which might hang the bus since
> the target peripheral is held in reset.

Will update to take into account the host->started in both PM_SUSPEND_PREPARE 
and PM_POST_SUSPEND.

> 
>>   		break;
>>   
>>   	case PM_POST_SUSPEND:
>