Re: [PATCH v6 10/10] PCI: dw-rockchip: Clear debug buffer before entering L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect arguments passed to rockchip_pcie_writel_apb() swap register offset and value.
- [Medium] Undocumented modification of controller DBI state during suspend.
--

commit 8a75a527ec46d932d4c3b6e7ed428836e9df376b
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Clear debug buffer before entering L2

This commit aims to clear the debug buffer before entering the L2 state to
ease debugging link training issues on resume. It introduces a helper to clear
the general debug register and invokes it during suspend.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index e675918ba1848..a508f4fffd2ac 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -77,6 +77,9 @@
>  #define  PCIE_CLKREQ_NOT_READY		FIELD_PREP_WM16(BIT(0), 0)
>  #define  PCIE_CLKREQ_PULL_DOWN		FIELD_PREP_WM16(GENMASK(13, 12), 1)
>  
> +/* General Debug Register */
> +#define PCIE_CLIENT_GENERAL_DEBUG	0x104
> +

[ ... ]

> @@ -1043,6 +1046,11 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	return ret;
>  }
>  
> +static inline void rockchip_pcie_link_status_clear(struct rockchip_pcie *rockchip)
> +{
> +	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_DEBUG, 0x0);

[Severity: High]
Are the arguments to rockchip_pcie_writel_apb() swapped here?

Looking at the function definition earlier in the file, the signature expects
the value before the register offset:

static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, u32 val,
				     u32 reg)

This call passes the register offset (PCIE_CLIENT_GENERAL_DEBUG, which is
0x104) as the data value to be written to register offset 0x0. This fails to
clear the intended debug register and might unexpectedly corrupt the controller
register at offset 0x0.

> +}
> +
>  static int rockchip_pcie_suspend(struct device *dev)
>  {
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> @@ -1056,6 +1064,11 @@ static int rockchip_pcie_suspend(struct device *dev)
>  
>  	rockchip->intx = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_MASK_LEGACY);
>  
> +	/* All sub-devices are in D3hot by PCIe stack */
> +	dw_pcie_dbi_ro_wr_dis(pci);

[Severity: Medium]
Is this modification of the DBI state intentional for this commit?

This introduces an undocumented change to disable the controller's DBI 
read-only writes unconditionally during the suspend sequence. Since there is no
explanation in the commit message and no matching enable call, could this be an
accidental inclusion unrelated to clearing the debug buffer?

> +
> +	rockchip_pcie_link_status_clear(rockchip);
> +
>  	ret = dw_pcie_suspend_noirq(pci);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-rockchip-pcie-system-suspend-v6-0-fbabcca9921b@collabora.com?part=10