Re: [PATCH v7 13/14] 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] drm/bridge: it6505: missing audio_lock in it6505_poweron() causes I2C data race
--

commit 180d8c850bc807b0a0b106e27c0aef15c059c696
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>

drm/bridge: it6505: Add audio support
    
This commit adds audio support for the it6505 bridge. It registers
an "hdmi-audio-codec" platform device, wires up audio helpers, and
introduces an audio_lock mutex to serialize audio enable/disable
against other tasks.

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
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2823,6 +2829,8 @@ static int it6505_poweroff(struct it6505 *it6505)
>  	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
>  	cancel_delayed_work_sync(&it6505->hdcp_work);
>  
> +	mutex_lock(&it6505->audio_lock);
> +
>  	if (pdata->gpiod_reset)
>  		gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
>  
> @@ -2842,6 +2850,7 @@ static int it6505_poweroff(struct it6505 *it6505)
>  
>  	it6505->powered = false;
>  	it6505->sink_count = 0;
> +	mutex_unlock(&it6505->audio_lock);
>  
>  	return 0;
>  }

[ ... ]

> @@ -2988,25 +2997,28 @@ static void it6505_remove_notifier_module(struct it6505 *it6505)
>  	flush_work(&it6505->extcon_wq);
>  }
>  
> -static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
> +static void it6505_delayed_audio(struct work_struct *work)
>  {
>  	struct it6505 *it6505 = container_of(work, struct it6505,
>  					     delayed_audio.work);
>  
>  	DRM_DEV_DEBUG_DRIVER(it6505->dev, "start");
>  
> -	if (!it6505->powered)
> +	if (it6505->enable_drv_hold)
>  		return;
>  
> -	if (!it6505->enable_drv_hold)
> -		it6505_enable_audio(it6505);
> +	mutex_lock(&it6505->audio_lock);
> +	if (it6505->powered && !it6505->audio.mute)
> +		__it6505_enable_audio(it6505);
> +	mutex_unlock(&it6505->audio_lock);
>  }

[Severity: High]
Does it6505_poweron() need to take the new audio_lock as well?

While this patch adds audio_lock to it6505_poweroff() to serialize with the
new it6505_delayed_audio() worker, it6505_poweron() sets it6505->powered = true
and performs hardware initialization without holding the lock:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_poweron() {
    ...
	it6505->powered = true;
	it6505_reset_logic(it6505);
	it6505_int_mask_enable(it6505);
	it6505_init(it6505);
    ...
}

If a device is runtime resumed and it6505_poweron() is called, could
it6505_delayed_audio() run concurrently? Since it6505->powered is set to true
early, it6505_delayed_audio() can acquire the audio_lock and execute
__it6505_enable_audio() while it6505_poweron() is still doing
it6505_reset_logic() and it6505_init().

For example, this sequence might occur when the bridge is powered off and an
ALSA stream is unmuted, queuing it6505_delayed_audio() with a 180ms delay.
If the device is runtime resumed shortly after (e.g., via cable hotplug),
it invokes it6505_poweron().

If it6505_delayed_audio() fires exactly as it6505_poweron() sets
it6505->powered = true, it can grab the audio_lock and begin I2C writes.
Meanwhile, it6505_poweron() continues concurrently because it doesn't take
the audio_lock.

Can this lead to a regression where the audio configuration is wiped out or
causes general I2C race conditions since the __it6505_enable_audio() 
configuration could be interleaved with it6505_init() or wiped out by the 
ALL_LOGIC_RESET in it6505_reset_logic()?

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