Re: [PATCH] drm/amd/display: fall back to software I2C on hardware engine failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing resource leak in acquire_i2c_hw_engine leaves DDC pin permanently open on setup failure.
- [High] TOCTOU race on shared I2C hardware buffer flag `i2c_hw_buffer_in_use` bypasses global mutual exclusion.
--

commit 715340ad0a0bb4f4ee8ee30d4c2c692856bedacf
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: fall back to software I2C on hardware engine failure

This commit implements a fallback mechanism to the bit-banging software I2C
engine when the hardware engine fails to complete longer transfers. It retries
the transfer on the software engine instead of outright failing, which resolves
EDID read failures on DCE6/Oland hardware.

> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> index f5261e8d76785..238c17e6f51db 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> @@ -72,9 +72,16 @@ bool dce_i2c_submit_command(
>  
>  	dce_i2c_hw = acquire_i2c_hw_engine(pool, ddc);

[Severity: High]
This isn't a bug introduced by this patch, but does acquire_i2c_hw_engine()
leak the DDC pin on setup failure?

In acquire_i2c_hw_engine(), dal_ddc_open() is called successfully, but if
the hardware engine is busy (e.g., locked by DMCU), setup_engine() returns
false. The error path then calls release_engine() without calling
dal_ddc_close():

    if (!setup_engine(dce_i2c_hw)) {
        release_engine(dce_i2c_hw);
        return NULL;
    }

This leaves the pin leaked in the opened state. As a result, subsequent calls
to dal_ddc_open() will return GPIO_RESULT_ALREADY_OPENED, which breaks all
future I2C transfers (and display hotplug/EDID detection) on that port until
reboot. Is there a missing dal_ddc_close() here?

[Severity: High]
This is a pre-existing issue, but does acquire_i2c_hw_engine() have a TOCTOU
race on the shared pool->i2c_hw_buffer_in_use flag?

The function checks and sets pool->i2c_hw_buffer_in_use without a global
lock:

    if (pool->i2c_hw_buffer_in_use || !is_engine_available(dce_i2c_hw))
        return NULL;
    ...
    pool->i2c_hw_buffer_in_use = true;

Because I2C adapter locks in amdgpu_dm_i2c_xfer() are per-connector,
concurrent I2C transfers on different display connectors can execute
acquire_i2c_hw_engine() simultaneously. Both threads can read
pool->i2c_hw_buffer_in_use as false, proceed to configure their respective
hardware engines concurrently, and set the flag to true. Can this bypass
global mutual exclusion and cause hardware timeouts or corrupted I2C
transactions?

> -	if (dce_i2c_hw)
> -		return dce_i2c_submit_command_hw(pool, ddc, cmd, dce_i2c_hw);
> +	if (dce_i2c_hw && dce_i2c_submit_command_hw(pool, ddc, cmd, dce_i2c_hw))
> +		return true;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826170549.21985-1-neptune@imm0nv1nhtv.is-a.dev?part=1