Re: [PATCH v6 04/10] PCI: dw-rockchip: Add helper function for enhanced LTSSM control mode

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Variable parameter passed to FIELD_PREP_WM16() causes build failures and contains a latent logic bug.

Pre-existing issues:
- [Low] Incomplete refactoring of enhanced LTSSM control mode setup.
--

commit 754de1ab627f784507a4b699acc006187c6a91fb
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Add helper function for enhanced LTSSM control mode

Remove code duplication and improve readability by introducing a new
function to setup the enhanced LTSSM mode.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 7472ec78b4738..6f84b07406d49 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -696,6 +696,18 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static void rockchip_pcie_enable_enhanced_ltssm_control_mode(struct rockchip_pcie *rockchip,
> +							     u32 flags)
> +{
> +	u32 val;
> +
> +	/* Enable the enhanced control mode of signal app_ltssm_enable */
> +	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
> +	if (flags)
> +		val |= FIELD_PREP_WM16(flags, 1);

[Severity: Medium]
Does passing the variable flags to FIELD_PREP_WM16() violate the macro's
requirement for a compile-time constant mask?

If rockchip_pcie_enable_enhanced_ltssm_control_mode() is outlined, will this
trigger a BUILD_BUG_ON_MSG(!__builtin_constant_p) build failure?

Also, if a future caller passes a multi-bit mask to flags (e.g. BIT(1) |
BIT(3)), would the macro evaluate to (1 << 1) & mask, setting only BIT(1)
and writing 0 to BIT(3), despite enabling both in the upper 16 bits?

> +	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> +}

[Severity: Low]
This is a pre-existing issue, but since the goal of this helper is to remove
code duplication for setting up enhanced LTSSM mode, should the exact same
duplicate sequence in rockchip_pcie_rc_reset_root_port() also be replaced
with rockchip_pcie_enable_enhanced_ltssm_control_mode(rockchip, 0)?

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_rc_reset_root_port() {
    ...
    /* LTSSM enable control mode */
    val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
    rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
    ...
}

[ ... ]

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