[PATCH v2 0/2] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[PATCH v2 0/2] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[Daniel Golle]

This series revives HDMI/DP audio support for the ITE IT6505 DP
bridge, originally posted by Jiaxin Yu [1], rebased onto current
-next and with the v3 review feedback addressed: the stale
&client->dev usages reported by AngeloGioacchino Del Regno [2] are
gone (the driver meanwhile gained it6505->dev), and the
"#sound-dai-cells" binding property requested by Chen-Yu Tsai [3] has
since been added by commit 325af1bef5b9 ("dt-bindings: display:
bridge: it6505: Add #sound-dai-cells").

Patch 1 wires up the previously-unused it6505 audio helpers via
hdmi_codec_ops, which unblocks the mt8186-mt6366 sound card that
references it6505 as the I2S3 codec.

Patch 2 is a follow-up fix so that audio hw_params are accepted even
when the it6505 DP output has no display (and thus no encoder)
attached. This is needed when the it6505 shares its I2S bus with
another codec, as on the MediaTek MT8186 "steelix" Chromebooks, where
the shared I2S3 must keep working for the speaker path regardless of
the it6505 link state.

Tested on a MediaTek MT8186 (google,steelix) Chromebook.
Actual HDMI/DP audio output could not be tested lacking the USB-C
adapter cable. However, making the it6505 at least probe and fixing
the obviously missing things makes the sound card on the Chromebook
come up and internal speakers and microphone as well as the headset
mini-jack work as expected (both tested).

[1] https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
[2] https://lore.kernel.org/all/c35ef2d8-ab40-484b-9a4c-38f2f3e7d99c@collabora.com/
[3] https://lore.kernel.org/all/CAGXv+5G2tP9i8VrUc6-xs2d72_nL9XH9iSCeixzA2AM7X5fXOQ@mail.gmail.com/
---
v2:
 * it6505: store the hdmi-codec platform_device and unregister it on
   i2c remove, fixing a resource leak / use-after-free
 * it6505: initialise the delayed audio work before registering the
   codec device
 * it6505: synchronously cancel the delayed audio work on audio
   shutdown and on driver remove (cancel_delayed_work_sync)
 * it6505: rework the mute path to cancel pending work synchronously
   and disable audio immediately when muting, removing a race
 * patch 2: drop the encoder check entirely instead of returning 0
   early, so the stream parameters are always cached; rewrite the
   commit message accordingly

v1: https://lore.kernel.org/all/cover.1784393979.git.daniel@makrotopia.org/
Daniel Golle (1):
  drm/bridge: it6505: Don't reject audio hw_params without an encoder

Jiaxin Yu (1):
  drm/bridge: it6505: Add audio support

 drivers/gpu/drm/bridge/ite-it6505.c | 104 +++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 19 deletions(-)


base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
-- 
2.55.0

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

[Daniel Golle]

From: Jiaxin Yu <jiaxin.yu@mediatek.com>

Add audio support for it6505 by bridging to the hdmi-codec, registering
an "hdmi-audio-codec" platform device from probe. The it6505's audio
setup/shutdown helpers were merged earlier as unused code; wire them up
via hdmi_codec_ops so the DAI actually appears, which unblocks the
mt8186-mt6366 sound card that references it6505 as the I2S3 codec.

Some DP-to-HDMI dongles get into a bad state if InfoFrame is sent
without audio data, so it6505's audio is only enabled once the stream
is unmuted.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
djg: respin of Jiaxin Yu's v3, rebased onto current mainline. Changes
from v3: hdmi_codec_ops lost .trigger, so drive enable/disable from
.mute_stream; drop the now-redundant __maybe_unused; use it6505->dev
instead of &client->dev, addressing AngeloGioacchino Del Regno's v3
review. Also fix issues in v3: initialise delayed_audio before
registering the codec device; keep and unregister the platform_device
and cancel delayed_audio on remove to avoid a leak and a
use-after-free; cancel the delayed work on audio shutdown; and
actually disable audio (not just cancel the pending enable) when the
stream is muted.

Note: it6505_enable_audio()/it6505_disable_audio() can still run
concurrently from the delayed work, the .mute_stream / .audio_shutdown
callbacks and the audio-FIFO-error IRQ; as in v3 these are left
unserialised -- input welcome on whether a lock is warranted.

v2:
 * store the hdmi-codec platform_device and unregister it on i2c
   remove, fixing a resource leak and use-after-free on unbind
 * initialise the delayed audio work before registering the codec
   device instead of after
 * synchronously cancel the delayed audio work on audio shutdown and
   on driver remove (cancel_delayed_work_sync)
 * rework the mute path to cancel pending work synchronously and
   disable audio immediately when muting, removing a race with the
   delayed enable work

 drivers/gpu/drm/bridge/ite-it6505.c | 101 +++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 8ecb43611dba..d84441926e8e 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -476,6 +476,7 @@ struct it6505 {
 	bool enable_enhanced_frame;
 	hdmi_codec_plugged_cb plugged_cb;
 	struct device *codec_dev;
+	struct platform_device *audio_pdev;
 	struct delayed_work delayed_audio;
 	struct it6505_audio_data audio;
 	struct dentry *debugfs;
@@ -2320,19 +2321,12 @@ static void it6505_stop_link_train(struct it6505 *it6505)
 
 static void it6505_link_train_ok(struct it6505 *it6505)
 {
-	struct device *dev = it6505->dev;
-
 	it6505->link_state = LINK_OK;
 	/* disalbe mute enable avi info frame */
 	it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, 0x00);
 	it6505_set_bits(it6505, REG_INFOFRAME_CTRL,
 			EN_VID_CTRL_PKT, EN_VID_CTRL_PKT);
 
-	if (it6505_audio_input(it6505)) {
-		DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
-		it6505_enable_audio(it6505);
-	}
-
 	if (it6505->hdcp_desired)
 		it6505_start_hdcp(it6505);
 }
@@ -2960,7 +2954,7 @@ static void it6505_remove_notifier_module(struct it6505 *it6505)
 	}
 }
 
-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);
@@ -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)
 {
 	struct device *dev = it6505->dev;
 	int i = 0;
@@ -3031,18 +3025,52 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,
 	return 0;
 }
 
-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);
 
+	cancel_delayed_work_sync(&it6505->delayed_audio);
 	if (it6505->powered)
 		it6505_disable_audio(it6505);
 }
 
-static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
-						       void *data,
-						       hdmi_codec_plugged_cb fn,
-						       struct device *codec_dev)
+static int it6505_audio_hw_params(struct device *dev, void *data,
+				  struct hdmi_codec_daifmt *daifmt,
+				  struct hdmi_codec_params *params)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+
+	return it6505_audio_setup_hw_params(it6505, params);
+}
+
+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);
+	} else {
+		queue_delayed_work(system_wq, &it6505->delayed_audio,
+				   msecs_to_jiffies(180));
+	}
+
+	return 0;
+}
+
+static int it6505_audio_hook_plugged_cb(struct device *dev,
+					void *data,
+					hdmi_codec_plugged_cb fn,
+					struct device *codec_dev)
 {
 	struct it6505 *it6505 = data;
 
@@ -3053,6 +3081,39 @@ static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
 	return 0;
 }
 
+static const struct hdmi_codec_ops it6505_audio_codec_ops = {
+	.hw_params = it6505_audio_hw_params,
+	.mute_stream = it6505_audio_mute,
+	.audio_shutdown = it6505_audio_shutdown,
+	.hook_plugged_cb = it6505_audio_hook_plugged_cb,
+};
+
+static int it6505_register_audio_driver(struct device *dev)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &it6505_audio_codec_ops,
+		.max_i2s_channels = 8,
+		.i2s = 1,
+		.no_capture_mute = 1,
+		.data = it6505,
+	};
+	struct platform_device *pdev;
+
+	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);
+
+	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
+					     PLATFORM_DEVID_AUTO, &codec_data,
+					     sizeof(codec_data));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	it6505->audio_pdev = pdev;
+	DRM_DEV_DEBUG_DRIVER(dev, "bound to %s", HDMI_CODEC_DRV_NAME);
+
+	return 0;
+}
+
 static inline struct it6505 *bridge_to_it6505(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct it6505, bridge);
@@ -3612,6 +3673,12 @@ static int it6505_i2c_probe(struct i2c_client *client)
 		return err;
 	}
 
+	err = it6505_register_audio_driver(dev);
+	if (err < 0) {
+		dev_err(dev, "Failed to register audio driver: %d", err);
+		return err;
+	}
+
 	INIT_WORK(&it6505->link_works, it6505_link_training_work);
 	INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
 	INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
@@ -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);
-- 
2.55.0

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

[Daniel Golle]

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. On
mt8186-corsola the speaker amplifier (rt1019) and it6505 share I2S3, so
when the it6505 DP output has no display attached (bridge.encoder is
NULL) the -ENODEV propagates up through dpcm_be_dai_hw_params() and tears
down the whole backend, breaking speaker playback.

The check is harmful in its own right: the rest of the function only
caches the stream parameters (channel count, rate, word length) in
software, none of which needs an encoder. Returning early leaves
it6505->audio.channel_count at 0, which would then index
audio_info_ca[-1] in it6505_enable_audio_infoframe() should a display be
hotplugged while a stream is running, and makes such a hotplug play with
stale parameters.

Drop the encoder check so the parameters are always cached; the actual
audio output is already gated by it6505->powered.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2:
 * drop the encoder check entirely instead of returning 0 early, so
   the stream parameters are always cached even while no display is
   attached; rewrite the commit message to explain why the check is
   harmful

 drivers/gpu/drm/bridge/ite-it6505.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index d84441926e8e..e20edbbb7c13 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2979,9 +2979,6 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
 			     params->sample_rate, params->sample_width,
 			     params->cea.channels);
 
-	if (!it6505->bridge.encoder)
-		return -ENODEV;
-
 	if (params->cea.channels <= 1 || params->cea.channels > 8) {
 		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
 				     it6505->audio.channel_count);
-- 
2.55.0

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

[sashiko-bot]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds check for `channel_count == 0` causes an out-of-bounds stack read in `it6505_enable_audio_infoframe`.
- [High] Missing cancellation of workqueues in the driver's remove path leads to Use-After-Free.
--

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

drm/bridge: it6505: Don't reject audio hw_params without an encoder

This commit drops the encoder check in it6505_audio_setup_hw_params() so
stream parameters are always cached. Previously, returning early left
it6505->audio.channel_count at 0, which could cause an out-of-bounds access
if a display was hotplugged while a stream was running.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index d84441926e8e0..e20edbbb7c133 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2979,9 +2979,6 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
>  			     params->sample_rate, params->sample_width,
>  			     params->cea.channels);
>  
> -	if (!it6505->bridge.encoder)
> -		return -ENODEV;
> -

[Severity: High]
This is a pre-existing issue, but if the SoC's I2S controller continuously
drives clocks (e.g. from an active shared speaker backend), could the IT6505
hardware audio FIFO overflow and trigger the INT_AUDIO_FIFO_ERROR interrupt
before ALSA configures the audio stream parameters?

If this happens, it6505_irq_audio_fifo_error() would call
it6505_enable_audio_infoframe() while it6505->audio.channel_count is still 0
(zero-initialized during probe). 

In it6505_enable_audio_infoframe(), it6505->audio.channel_count - 1 evaluates
to -1, causing an out-of-bounds stack read:

    audio_info_ca[it6505->audio.channel_count - 1]);

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

[Severity: High]
This is a pre-existing issue, but does the driver's remove path miss cancelling
several workqueues, leading to a potential use-after-free?

In it6505_i2c_remove():

    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);
        it6505_poweroff(it6505);
        it6505_remove_edid(it6505);
    }

Only delayed_audio is explicitly cancelled. 

If other background work items (link_works, hdcp_wait_ksv_list, hdcp_work,
extcon_wq) are pending or running when the device is unbound, could they access
the it6505 structure after it has been automatically freed by devres?

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