Re: [PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized `channel_count` due to the early return can lead to an out-of-bounds stack array access in `it6505_enable_audio_infoframe()` if a hotplug race condition occurs.
- [Medium] Returning early when `bridge.encoder` is NULL bypasses audio parameter caching, causing DP audio to use stale or incorrect hardware parameters when a display is later hotplugged.
--

--- Patch [2]: [PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder ---
commit 716c566c9be6893ad8cd2b80d06ce9aa0a5b67df
Author: Daniel Golle <daniel@makrotopia.org>

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. Now that it6505 registers an hdmi-audio-codec,
this callback runs whenever a stream is configured on the I2S DAI it is
wired to, including when that DAI is shared with another codec.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 698ae01b3a474..e88960c82a239 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2979,7 +2979,7 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
>  			     params->cea.channels);
>  
>  	if (!it6505->bridge.encoder)
> -		return -ENODEV;
> +		return 0;

[Severity: High]
Can this early return lead to an out-of-bounds stack array access during a
hotplug race condition?

If the early return prevents it6505->audio.channel_count from being
initialized, it will remain 0. If the stream is unmuted and delayed_audio is
queued, a subsequent DP hotplug within 180ms could unblock it6505_irq_hpd().

If delayed_audio fires before it6505_irq_hpd() executes
it6505_variable_config() to set channel_count to 2, it will call
it6505_enable_audio_infoframe() with channel_count=0:

it6505_enable_audio_infoframe() {
        ...
        it6505_write(it6505, REG_AUD_INFOFRAM_DB3,
                     audio_info_ca[it6505->audio.channel_count - 1]);
        ...
}

Would this result in reading audio_info_ca[-1] from the stack?


[Severity: Medium]
Will returning early here bypass the audio parameter caching?

If an ALSA stream on a shared DAI is played with non-default parameters
while DP is disconnected, it6505_audio_setup_hw_params() will return 0 and
skip updating the it6505->audio state.

When a DP display is later hotplugged, could it6505_irq_audio_fifo_error()
auto-enable audio using stale or default parameters instead of the active
stream's parameters?

>  
>  	if (params->cea.channels <= 1 || params->cea.channels > 8) {
>  		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784393979.git.daniel@makrotopia.org?part=2