Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [High] The interrupt handler can still execute and re-queue work items after `cancel_work_sync()` in `it6505_i2c_remove()`, leading to Use-After-Free. - [High] Missing `pm_runtime_disable()` in `it6505_i2c_remove()` leaves the device in an inconsistent PM state. - [High] Unprotected modification and TOCTOU race on `it6505->audio.mute` leading to unintended audio enablement. - [Medium] Cached audio parameters are overwritten by default values on display hotplug, defeating the secondary goal of the patch. - [Medium] Audio configuration fields in `it6505->audio` are modified without consistent locking, leading to potential race conditions. -- --- Patch [4]: [PATCH v3 4/4] drm/bridge: it6505: Don't reject audio hw_params without an encoder --- commit af4a3bfaf41a8f5bf333fd6908bc4e3f4fabd9a8 Author: Daniel Golle <[email protected]> drm/bridge: it6505: Don't reject audio hw_params without an encoder it6505_audio_setup_hw_params() returns -ENODEV when the bridge is not attached to a DRM encoder. This commit drops the encoder check so the parameters are always cached. > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 37036ef5403fb..435601b6929ed 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -2999,9 +2999,6 @@ static int it6505_audio_setup_hw_params(struct it6505 > *it6505, > params->sample_rate, params->sample_width, > params->cea.channels); > > - if (!it6505->bridge.encoder) > - return -ENODEV; > - [Severity: Medium] This is a pre-existing issue, but does removing this check fully solve the problem if the cached parameters are overwritten on hotplug? When a stream starts with no display attached, it6505_audio_setup_hw_params() caches the correct settings as intended. However, if a display is hotplugged later, the HPD interrupt handler sees a disconnected state beforehand and zeroed it6505->dpcd. Upon reconnect, it finds it6505->dpcd[0] == 0 and calls it6505_variable_config(): it6505->audio.sample_rate = AUDIO_SAMPLE_RATE; it6505->audio.channel_count = AUDIO_CHANNEL_COUNT; Does this unconditionally reset the audio variables to default values, causing audio to play with 2 channels and 48kHz rather than the dynamically configured stream parameters? [Severity: Medium] This is a pre-existing issue, but does this function need to take the audio_lock when modifying the audio configuration fields? In it6505_audio_setup_hw_params(): it6505->audio.channel_count = params->cea.channels; This modifies the fields in it6505->audio without taking it6505->audio_lock. Since it6505_enable_audio() can be called from asynchronous paths (like it6505_delayed_audio or IRQ) which takes audio_lock and reads these fields, could this result in hardware being programmed with a partially updated state if readers see a mix of old and new parameters? > if (params->cea.channels <= 1 || params->cea.channels > 8) { > DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support", > params->cea.channels); [Severity: High] This is a pre-existing issue, but can the interrupt handler still execute and re-queue work items after they are cancelled during driver unbind? In it6505_i2c_remove(): cancel_work_sync(&it6505->link_works); cancel_work_sync(&it6505->hdcp_wait_ksv_list); cancel_delayed_work_sync(&it6505->hdcp_work); cancel_work_sync(&it6505->extcon_wq); Since the device interrupt (it6505->irq) is not disabled before cancelling these work items, could an incoming interrupt spawn the threaded handler and call schedule_work(&it6505->link_works) after cancel_work_sync() returns, leading to a use-after-free when the work item executes? [Severity: High] This is a pre-existing issue, but is there a missing pm_runtime_disable() in the driver removal path? While it6505_i2c_probe() successfully enables runtime PM with pm_runtime_enable(dev), it6505_i2c_remove() does not appear to call pm_runtime_disable(dev) before returning. Can this leave the PM core with references to the unbound device, leading to a resource leak or lifecycle violation? [Severity: High] This is a pre-existing issue, but is there a time-of-check to time-of-use race on the audio mute state? In it6505_irq_audio_fifo_error(), the mute state is read outside of the lock: if (it6505->audio.mute) return; if (it6505_audio_input(it6505)) it6505_enable_audio(it6505); If ALSA mutes the stream concurrently, it6505_audio_mute() sets mute to true and disables audio. After it returns, if the IRQ wakes up and unconditionally calls it6505_enable_audio() which takes the lock and turns audio back on, could this defeat the mute and cause hardware malfunction by sending an InfoFrame without audio data? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
