Re: [PATCH v2 09/15] PCI: rcar-gen4: Add Root Port reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] goto mixing: functions mix goto-based cleanup and guard() declarations
--

commit f67d0a2e310ff1eec161853a2c467d65ff545950
Author: Koichiro Den <den@valinux.co.jp>
PCI: rcar-gen4: Add Root Port reset support

This commit implements the reset_root_port() callback for the host bridge to
allow PCI error recovery to reset and reinitialize the controller. It also
adds synchronization around the APP interrupt enables during the reset.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index d1ea942727111..d3f04358bbe31 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -593,6 +619,7 @@ static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
>  	writel(0, rcar->base + AXIINTCCONT);
>  
>  	/* Deconfigure MSI interrupt signal */
> +	guard(raw_spinlock_irqsave)(&rcar->app_lock);
>  	val = readl(rcar->base + PCIEINTSTS0EN);
>  	val &= ~MSI_CTRL_INT;
>  	writel(val, rcar->base + PCIEINTSTS0EN);

[Severity: Low]
Does this mix goto-based cleanup and guard() declarations within
rcar_gen4_pcie_host_msi_init()?

The cleanup subsystem guidelines recommend converting all resources to
scope-based cleanup or none at all in a given function to avoid confusing
ownership semantics. Since rcar_gen4_pcie_host_msi_init() still uses a
goto-based error path to reach this block, would it be better to avoid
injecting guard() here and stick to explicit locking, or convert the entire
function to scope-based cleanup?

[ ... ]
> @@ -691,14 +718,83 @@ static int rcar_gen4_pcie_host_setup(struct dw_pcie_rp *pp)
[ ... ]
> +static int rcar_gen4_pcie_reset_root_port(struct pci_host_bridge *bridge,
> +					  struct pci_dev *pdev)
> +{
> +	struct rcar_gen4_pcie *rcar = dev_get_drvdata(bridge->dev.parent);
> +	struct dw_pcie *dw = &rcar->dw;
> +	struct dw_pcie_rp *pp = &dw->pp;
> +	struct device *dev = dw->dev;
> +	int ret;
> +
> +	guard(mutex)(&rcar->reset_lock);
> +
> +	rcar_gen4_pcie_quiesce_irqs(rcar);
> +
> +	dw_pcie_stop_link(dw);
> +
> +	rcar_gen4_pcie_host_perst_assert(pp, true);
> +
> +	ret = rcar->drvdata->reinit(rcar);
> +	if (ret)
> +		goto out;
[ ... ]
> +out:
> +	rcar_gen4_pcie_resume_irqs(rcar, !ret);
> +	if (ret)
> +		dev_err(dev, "Root Port reset failed (%d); interrupts stay masked until a reset succeeds\n",
> +			ret);
> +
> +	return ret;
> +}

[Severity: Low]
Is it appropriate to mix guard(mutex) and goto-based cleanup flow in the
newly introduced rcar_gen4_pcie_reset_root_port() function?

Similar to the previous case, mixing goto labels with guard() can lead to
ambiguous cleanup paths. Could this function be written using entirely
scope-based constructs, or alternatively, entirely using manual lock
management to comply with the cleanup guidelines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928165230.3397664-1-den@valinux.co.jp?part=9