Re: [PATCH v2 1/2] drm/bridge: it6505: Add audio support

[sashiko-bot@kernel.org]

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 <jiaxin.yu@mediatek.com>

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/20230730180803.22570-4-jiaxin.yu@mediatek.com/

> 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/cover.1784561622.git.daniel@makrotopia.org?part=1