Re: [PATCH v8 07/29] phy: rockchip: usbdp: Handle rk_udphy_reset_deassert_all errors in init check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] The return value of `regmap_read` is ignored, leading to the use of an uninitialized stack variable.
- [Medium] The patch fails to address identical reset deassertion vulnerabilities in `rk_udphy_init`, leaving the driver susceptible to the exact same SError it claims to fix.
--

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

phy: rockchip: usbdp: Handle rk_udphy_reset_deassert_all errors in init check

This commit fixes a theoretical SError issue by ensuring that the return
value of rk_udphy_reset_deassert_all is checked. It bails out early if
deasserting the reset lines fails before attempting further register access.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/message/20260619154903.2225A1F000E9%40smtp.kernel.org

> Fix theoretical issue, that failure to deassert the reset lines would
> result in an SError in follow-up register access. Note, that this cannot
> happen with current code, since the Rockchip reset driver never returns
> any errors for (de)asserting reset lines.

[Severity: Medium]
This is a pre-existing issue, but does this same vulnerability still exist in
rk_udphy_init() where reset deassertions are also unchecked?

For example:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_init() {
	...
	rk_udphy_reset_deassert(udphy, "pma_apb");
	rk_udphy_reset_deassert(udphy, "pcs_apb");

	/* Step 2: set init sequence and phy refclk */
	ret = regmap_multi_reg_write(udphy->pma_regmap, ...
	...
}

If rk_udphy_reset_deassert() fails here, wouldn't the follow-up
regmap_multi_reg_write() trigger the exact same SError?

Should the reset deassertions in rk_udphy_init() also be checked?

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index f7ff55f03b7cf..b8de0a47a6165 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -936,7 +936,9 @@ static int rk_udphy_get_initial_status(struct rk_udphy *udphy)
>  		return ret;
>  	}
>  
> -	rk_udphy_reset_deassert_all(udphy);
> +	ret = rk_udphy_reset_deassert_all(udphy);
> +	if (ret)
> +		goto exit;
>  
>  	regmap_read(udphy->pma_regmap, CMN_LANE_MUX_AND_EN_OFFSET, &value);
>  	if (FIELD_GET(CMN_DP_LANE_MUX_ALL, value) && FIELD_GET(CMN_DP_LANE_EN_ALL, value)) {

[Severity: Medium]
This is a pre-existing issue, but does this code evaluate an uninitialized
variable if the read fails?

If regmap_read() fails and returns an error, value is not initialized, and
the subsequent FIELD_GET() checks will operate on uninitialized stack memory.

Should the return value of regmap_read() be checked here?

>  		dev_info(udphy->dev, "Started with DP PHY pre-enabled; seamless takeover unsupported\n");
>  		rk_udphy_grfreg_write(udphy->vogrf, &cfg->vogrfcfg[udphy->id].hpd_trigger, false);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-rockchip-usbdp-cleanup-v8-0-47f682987895@collabora.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy