Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Concurrent execution of audio hardware configuration sequences without serialization. - [High] The AUDIO_FIFO_ERROR threaded IRQ blindly re-enables the audio stream, circumventing ALSA's requested mute state. - [Low] The debug log for unsupported channel counts prints the old/driver-configured channel count instead of the requested invalid count. Pre-existing issues: - [High] Missing cancellation for pre-existing workqueues in `it6505_i2c_remove` leads to Use-After-Free. -- --- Patch [1]: [PATCH v2 1/2] drm/bridge: it6505: Add audio support --- commit 368cb1a1072339bd0c22621289d60b4bd60b3bc6 Author: Jiaxin Yu <[email protected]> drm/bridge: it6505: Add audio support This commit adds audio support for the it6505 by bridging to the hdmi-codec and registering an "hdmi-audio-codec" platform device. It wires up the audio setup and shutdown helpers via hdmi_codec_ops. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 8ecb43611dba8..d84441926e8e0 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c [ ... ] > @@ -2974,9 +2968,9 @@ static void __maybe_unused it6505_delayed_audio(struct > work_struct *work) > it6505_enable_audio(it6505); > } > > -static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505, > - struct hdmi_codec_params > - *params) > +static int it6505_audio_setup_hw_params(struct it6505 *it6505, > + struct hdmi_codec_params > + *params) > { [Severity: Low] Does the error logging in this function print the wrong variable? When an unsupported channel count is passed in params, the code errors out: if (params->cea.channels <= 1 || params->cea.channels > 8) { DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support", it6505->audio.channel_count); return -EINVAL; } Should this log the invalid requested channel count (params->cea.channels) instead of the previously configured or uninitialized driver state (it6505->audio.channel_count)? > struct device *dev = it6505->dev; > int i = 0; [ ... ] > @@ -3031,18 +3025,52 @@ static int __maybe_unused > it6505_audio_setup_hw_params(struct it6505 *it6505, > +static int it6505_audio_mute(struct device *dev, void *data, > + bool enable, int direction) > +{ > + struct it6505 *it6505 = dev_get_drvdata(dev); > + > + DRM_DEV_DEBUG_DRIVER(dev, "mute: %d", enable); > + > + /* > + * Some DP-to-HDMI dongles get into a bad state if the InfoFrame is > + * sent without audio data, so only enable it6505's audio once the > + * stream is unmuted (i.e. actually playing). > + */ > + if (enable) { > + cancel_delayed_work_sync(&it6505->delayed_audio); > + if (it6505->powered) > + it6505_disable_audio(it6505); [Severity: High] The commit message asks for input on whether a lock is warranted since it6505_enable_audio() and it6505_disable_audio() can run concurrently. Could this lack of serialization leave the hardware in a corrupted state? These functions perform multi-step, non-atomic sequences of register writes, such as toggling AUDIO_RESET and EN_AUD_MUTE. If it6505_audio_mute() calls it6505_disable_audio() asynchronously while the IRQ handler or delayed work is calling it6505_enable_audio(), they could interleave and break audio playback. [Severity: High] Does the AUDIO_FIFO_ERROR IRQ handler respect the mute state requested here? If the stream is muted by ALSA, it6505_audio_mute() disables audio. However, if an AUDIO_FIFO_ERROR IRQ fires, it6505_irq_audio_fifo_error() appears to blindly restart the audio stream: drivers/gpu/drm/bridge/ite-it6505.c:it6505_irq_audio_fifo_error() { ... if (it6505_audio_input(it6505)) it6505_enable_audio(it6505); } Could this bypass the software mute state and send InfoFrames without audio data, potentially triggering the DP-to-HDMI dongle failure this delayed-enable workaround was meant to prevent? > + } else { > + queue_delayed_work(system_wq, &it6505->delayed_audio, > + msecs_to_jiffies(180)); > + } > + > + return 0; > +} > + [ ... ] > @@ -3644,6 +3711,8 @@ static void it6505_i2c_remove(struct i2c_client *client) > { > struct it6505 *it6505 = i2c_get_clientdata(client); > > + platform_device_unregister(it6505->audio_pdev); > + cancel_delayed_work_sync(&it6505->delayed_audio); > drm_bridge_remove(&it6505->bridge); > drm_dp_aux_unregister(&it6505->aux); > it6505_debugfs_remove(it6505); [Severity: High] This is a pre-existing issue, but is it possible for other background works to trigger a use-after-free here? it6505_i2c_remove() cancels the newly added delayed_audio work, but struct it6505 is allocated via devm_drm_bridge_alloc(). Once this function returns, devres will immediately free the it6505 memory. If any of the previously existing works (link_works, hdcp_wait_ksv_list, or hdcp_work) are pending or executing, could they dereference the freed struct it6505 memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
