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