[PATCH v2] drm/bridge: tc358767: check regmap_write() return values

[PATCH v2] drm/bridge: tc358767: check regmap_write() return values

[Жамбакиев Радий Рикардинович]

From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

The switch to direct regmap_write() calls replaced the tc_write()
macro, which bailed out on failure. At three sites the error check
was not carried over, so the return value is overwritten before it
is ever examined:

The DP0_VIDSYNCDELAY write in tc_set_edp_video_mode(), and
both DP_PHY_CTRL writes in the main link PHY reset sequence in
tc_main_link_enable(), whose error is clobbered by the following
PHY_RDY poll

As a result, failed I2C transactions to the bridge are silently
ignored. A failed DP0_VIDSYNCDELAY write can leave the stream running
with stale THRESH_DLY and VID_SYNC_DLY values, causing display
corruption without any error being reported. A failed PHY reset
sequence can leave the main link PHY in reset; the PHY_RDY poll may
still succeed because the bit was set during the initial AUX link
setup, so link training proceeds on a bad PHY, or the real I2C error
is masked by a misleading "timeout waiting for phy become ready".

Propagate the errors instead. The callers of both functions already
handle failures by disabling the main link via tc_main_link_disable().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 6d0c38315915 ("drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors")
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
Changes since v1:
- Fixed text encoding of the patch itself, so git am applies
correctly
- Rephrased patch description to avoid checkpatch.pl commit
reference error
- Added explicit `From: ...` to the top of the patch body, to
prevent checkpatch.pl email name mismatch error
- Dropped Cc: stable because this issue is minor and does not
cause regressions or crashes.

 drivers/gpu/drm/bridge/tc358767.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7188935fdb82..d6b62546fed0 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1014,6 +1014,8 @@ static int tc_set_edp_video_mode(struct tc_data *tc,
 	ret = regmap_write(tc->regmap, DP0_VIDSYNCDELAY,
 		 FIELD_PREP(THRESH_DLY, max_tu_symbol) |
 		 FIELD_PREP(VID_SYNC_DLY, vid_sync_dly));
+	if (ret)
+		return ret;
 
 	ret = regmap_write(tc->regmap, DP0_TOTALVAL,
 			   FIELD_PREP(H_TOTAL, mode->htotal) |
@@ -1144,9 +1146,14 @@ static int tc_main_link_enable(struct tc_data *tc)
 	/* Reset/Enable Main Links */
 	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
 	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
+	if (ret)
+		return ret;
+
 	usleep_range(100, 200);
 	dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
 	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
+	if (ret)
+		return ret;
 
 	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 500, 100000);
 	if (ret) {
-- 
2.53.0

Re: [PATCH v2] drm/bridge: tc358767: check regmap_write() return values

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Meaningful code and commit message mismatch regarding error cleanup
--

commit eec31d58dcee33de416927375afdb370e4851289
Author: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

drm/bridge: tc358767: check regmap_write() return values

This commit propagates error values from regmap_write() in
tc_set_edp_video_mode() and tc_main_link_enable(). The commit message
asserts that the callers of both functions already handle failures by
disabling the main link via tc_main_link_disable().

> Propagate the errors instead. The callers of both functions already
> handle failures by disabling the main link via tc_main_link_disable().

[Severity: Medium]
Is this description accurate for both functions?

While the caller of tc_set_edp_video_mode() handles failures by calling
tc_main_link_disable(), it looks like the caller of tc_main_link_enable()
does not.

If tc_main_link_enable() fails, its caller tc_edp_bridge_atomic_enable()
only logs the error and returns:

drivers/gpu/drm/bridge/tc358767.c:tc_edp_bridge_atomic_enable() {
	...
	ret = tc_main_link_enable(tc);
	if (ret < 0) {
		dev_err(tc->dev, "main link enable error: %d\n", ret);
		return;
	}
	...
}

Will this mismatch leave the hardware in a partially initialized state
upon I2C failure if tc_main_link_enable() returns early?

> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index e3a57f8228dae..a98a48534bf60 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
[ ... ]
> @@ -1144,9 +1146,14 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	/* Reset/Enable Main Links */
>  	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
>  	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
Does returning an error here rely on the caller performing cleanup that it
currently lacks?

>  	usleep_range(100, 200);
>  	dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
>  	ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl);
> +	if (ret)
> +		return ret;
>  
>  	ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 500, 100000);
>  	if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826132251.113084-1-r.zhambakiev@prosoftsystems.ru?part=1