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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: cix: premature hardware ready polling in CDB transactions
--

commit 9a56e63287f31e17b6e8c0dd1873f84b0c04b8d1
Author: Luo Liu <Luo.Liu@cixtech.com>

phy: cix: Add Sky1 DP PHY driver

Add a PHY driver for the CIX Sky1 dedicated DisplayPort PMA. Support
link rate, lane count, SSC and voltage swing via the generic PHY DP
ops.

> diff --git a/drivers/phy/cix/phy-cix-dp.c b/drivers/phy/cix/phy-cix-dp.c
> new file mode 100644
> index 0000000000000..b5adc9c295060
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-dp.c
[ ... ]
> +static int cix_dp_phy_cdb_write(struct cix_dp *dp, u32 offset, u32 data)
> +{
> +	int ret;
> +
> +	ret = cix_dp_phy_cdb_ready(dp);
> +	if (ret)
> +		return ret;
> +
> +	writel(offset, dp->base + CIX_DP_CDB_ADDR);
> +	writel(data, dp->base + CIX_DP_CDB_WDATA);
> +	writel(CIX_DP_CDB_SEL_WRITE, dp->base + CIX_DP_CDB_SEL_WR);
> +	writel(1, dp->base + CIX_DP_CDB_ENABLE);
> +
> +	return cix_dp_phy_cdb_ready(dp);

[Severity: Medium]
Is there a risk of a race condition here if the hardware takes a few clock
cycles to pull the CIX_DP_CDB_READY bit low after CIX_DP_CDB_ENABLE is set?

Because cix_dp_phy_cdb_ready() uses readl_poll_timeout(), it immediately
performs a read. If the PHY IP drops the ready bit asynchronously (e.g.,
due to a clock domain crossing), the CPU might read the stale idle state
where the ready bit is still 1 and return success instantly.

This could cause the write operation to return before the hardware
transaction actually completes, potentially overlapping with the next
transaction and corrupting PHY configuration.

> +}
> +
> +static int cix_dp_phy_cdb_read(struct cix_dp *dp, u32 offset)
> +{
> +	int ret;
> +
> +	ret = cix_dp_phy_cdb_ready(dp);
> +	if (ret)
> +		return ret;
> +
> +	writel(offset, dp->base + CIX_DP_CDB_ADDR);
> +	writel(CIX_DP_CDB_SEL_READ, dp->base + CIX_DP_CDB_SEL_WR);
> +	writel(1, dp->base + CIX_DP_CDB_ENABLE);
> +
> +	ret = cix_dp_phy_cdb_ready(dp);
> +	if (ret)
> +		return ret;
> +
> +	return readl(dp->base + CIX_DP_CDB_RDATA) & GENMASK(15, 0);

[Severity: Medium]
Similarly, could this early ready check cause cix_dp_phy_cdb_read() to
read stale data?

If the hardware hasn't pulled the ready bit to 0 yet,
cix_dp_phy_cdb_ready() will return 0 immediately and we will read
CIX_DP_CDB_RDATA before the hardware has actually populated it.

> +}
[ ... ]

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

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