Re: [PATCH 2/2] drm/amd/display: close DDC on I2C engine setup failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless read-modify-write of the shared `pool->i2c_hw_buffer_in_use` flag allows concurrent access to hardware I2C resources.
--

commit 12dc016e9bb9b2e123aaf512011f08b0b29e4bd1
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: close DDC on I2C engine setup failure

acquire_i2c_hw_engine() opens the DDC pins before setting up the
hardware engine. If setup_engine() fails, the error path releases the
engine but leaves the DDC pins open and the engine's DDC pointer set.

> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> index 05892ab4529f6..e7a05494abab0 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> @@ -485,6 +485,8 @@ struct dce_i2c_hw *acquire_i2c_hw_engine(
>  
>  	if (!setup_engine(dce_i2c_hw)) {
>  		release_engine(dce_i2c_hw);
> +		dal_ddc_close(dce_i2c_hw->ddc);
> +		dce_i2c_hw->ddc = NULL;
>  		return NULL;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write of the
shared pool->i2c_hw_buffer_in_use flag allow concurrent access to hardware I2C
resources?

In drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c:acquire_i2c_hw_engine(),
there is an initial lockless check:

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

And shortly after this diff hunk, it sets the flag:

    pool->i2c_hw_buffer_in_use = true;

Since amdgpu_dm_i2c_xfer() is called by the Linux I2C core, it serializes
operations per-adapter (per port). However, different DDC ports have different
adapters and can run concurrently. If they call dc_submit_i2c() ->
dce_i2c_submit_command() -> acquire_i2c_hw_engine() without taking a
driver-wide lock to serialize access across the single shared resource_pool,
both threads can read false and bypass the exclusion.

This could lead to hardware I2C data corruption or engine lockups.
Furthermore, one thread could clear the flag prematurely in
dce_i2c_submit_command_hw() while the other is still active, corrupting the
state machine and allowing a third thread to incorrectly enter. Userspace can
trigger this trivially by opening multiple /dev/i2c-* nodes for different AMD
display ports and issuing I2C transactions simultaneously.

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