Re: [PATCH v2 2/3] phy: cix: Add Sky1 DP PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] phy: cix: missing state restore on cix_dp_phy_set_lane_voltage() error path
- [Medium] phy: cix: Race condition in CDB transaction completion due to ignored busy pulse timeout
--

commit 813f2a99f8250a74e6b3856914829e75ea21a786
Author: Luo Liu <Luo.Liu@cixtech.com>

phy: cix: Add Sky1 DP PHY driver

This commit adds a PHY driver for the CIX Sky1 dedicated DisplayPort PMA.
It supports configuring the link rate, lane count, SSC, and voltage swing
using the generic PHY DP operations.

> diff --git a/drivers/phy/cix/phy-cix-dp.c b/drivers/phy/cix/phy-cix-dp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-dp.c

[ ... ]

> +static int cix_dp_phy_cdb_complete(struct cix_dp *dp)
> +{
> +	u32 value;
> +
> +	readl_poll_timeout_atomic(dp->base + CIX_DP_CDB_READY, value,
> +				  !(value & BIT(0)), 0, 1);
> +
> +	return cix_dp_phy_cdb_ready(dp);
> +}

[Severity: Medium]
Does this explicitly suppress the return value from the busy-pulse poll?

If the hardware takes longer than 1us to drop the CIX_DP_CDB_READY bit to 0
after CIX_DP_CDB_ENABLE is written, the first polling loop will time out and
the error is ignored. The second polling loop in cix_dp_phy_cdb_ready()
could then immediately read the stale READY == 1 state and return success
before the hardware actually processes the command.

Can this race condition lead to PHY initialization failures by allowing
overlapping commands to be issued to the hardware?

[ ... ]

> +static int cix_dp_phy_set_lane_voltage(struct cix_dp *dp, u32 lane,
> +				       const struct cix_dp_voltage_conf *cfg)
> +{
> +	int value;
> +	int ret;
> +
> +	value = cix_dp_phy_cdb_read(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane));
> +	if (value < 0)
> +		return value;
> +
> +	ret = cix_dp_phy_cdb_write(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane),
> +				   value | BIT(0));
> +	if (ret)
> +		return ret;
> +
> +	ret = cix_dp_phy_cdb_write(dp, CIX_DP_TX_TXCC_CTRL_LANE(lane),
> +				   cfg->tx_txcc_ctrl);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If this write or any subsequent configuration write fails, the function
returns immediately. Does this skip the cleanup step at the end of the
function that restores the CIX_DP_TX_DIAG_ACYA_LANE register?

It appears that returning early leaves the hardware stuck in a diagnostic
override state (with BIT(0) still set), which might silently corrupt future
operations or link training attempts. Should these error paths ensure the
original state is restored?

> +
> +	ret = cix_dp_phy_cdb_write(dp, CIX_DP_DRV_DIAG_TX_DRV_LANE(lane),
> +				   cfg->drv_diag_tx_drv);
> +	if (ret)
> +		return ret;
> +
> +	ret = cix_dp_phy_cdb_write(dp,
> +				   CIX_DP_TX_TXCC_MGNFS_MULT_000_LANE(lane),
> +				   cfg->tx_txcc_mgnfs_mult_000);
> +	if (ret)
> +		return ret;
> +
> +	ret = cix_dp_phy_cdb_write(dp,
> +				   CIX_DP_TX_TXCC_CPOST_MULT_00_LANE(lane),
> +				   cfg->tx_txcc_cpost_mult_00);
> +	if (ret)
> +		return ret;
> +
> +	return cix_dp_phy_cdb_write(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane),
> +				    value & ~BIT(0));
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012123618.648930-1-Luo.Liu@cixtech.com?part=2

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