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 <[email protected]> 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/[email protected]?part=2
