Re: [PATCH v6 12/13] drm/bridge: it6505: Add audio support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing PM runtime reference in audio callbacks leads to TOCTOU race with device power-off.
--

--- Patch [12]: [PATCH v6 12/13] drm/bridge: it6505: Add audio support ---
commit 10341e11c958ca412de2f96980c0f7ee9f5854c5
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>

drm/bridge: it6505: Add audio support
    
Add audio support for it6505 by bridging to the hdmi-codec: register
an "hdmi-audio-codec" platform device from probe and wire up the
previously unused audio helpers via hdmi_codec_ops. This unblocks the
mt8186-mt6366 sound card which references it6505 as the I2S3 codec.

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 aeaa7b5ecf5aa..c560f1258f6d7 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
> -static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
> +static void it6505_audio_shutdown(struct device *dev, void *data)
>  {
>  	struct it6505 *it6505 = dev_get_drvdata(dev);
>  
> +	mutex_lock(&it6505->audio_lock);
> +	it6505->audio.mute = true;
>  	if (it6505->powered)
> -		it6505_disable_audio(it6505);
> +		__it6505_disable_audio(it6505);
> +	mutex_unlock(&it6505->audio_lock);
> +	cancel_delayed_work_sync(&it6505->delayed_audio);
> +}
[ ... ]
> +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);
> +
> +	/*
> +	 * Delay enabling audio until the stream is unmuted; InfoFrames
> +	 * without audio data upset some DP-to-HDMI dongles.
> +	 */
> +	if (enable) {
> +		mutex_lock(&it6505->audio_lock);
> +		it6505->audio.mute = true;
> +		if (it6505->powered)
> +			__it6505_disable_audio(it6505);
> +		mutex_unlock(&it6505->audio_lock);
> +		cancel_delayed_work_sync(&it6505->delayed_audio);
> +	} else {
> +		mutex_lock(&it6505->audio_lock);
> +		it6505->audio.mute = false;
> +		mutex_unlock(&it6505->audio_lock);
> +		queue_delayed_work(system_wq, &it6505->delayed_audio,
> +				   msecs_to_jiffies(180));
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this code race with device power-off?

If the audio stream is unmuted via ALSA, it6505_audio_mute() queues
it6505->delayed_audio to run after 180ms.

If a user unplugs the DP cable during this delay, it6505_extcon_work() calls
pm_runtime_put_sync() on the parent device, invoking the runtime suspend
callback it6505_poweroff().

it6505_poweroff() disables regulators and asserts the reset GPIO before
clearing the it6505->powered flag at the very end of the function. It does
not cancel the delayed work.

If it6505_delayed_audio() (or a concurrent it6505_audio_mute() /
it6505_audio_shutdown()) checks it6505->powered before it6505_poweroff()
updates the flag, they could proceed to issue I2C writes while the device's
power is physically cut. This can result in I2C bus lockups since unpowered
chips may pull SDA/SCL lines low.

Should these functions use a PM runtime reference (e.g., via
pm_runtime_get_if_active()) to safely access the hardware?

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