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

[PATCH v3 0/4] 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 fixes a pre-existing use-after-free in the driver's remove()
path: none of link_works, hdcp_wait_ksv_list or hdcp_work were ever
cancelled before devres frees the it6505 struct, unless DRM core
happened to call the bridge's .detach() first. It is needed before
patch 3 so that wiring up the new hdmi-audio-codec platform_device
does not add another caller into an already-broken teardown path.

Patch 2 fixes a second pre-existing bug: it6505->audio.channel_count
is zero until either DP link training or a valid ALSA hw_params call
has run, but it6505_enable_audio_infoframe() indexes an 8-entry table
with channel_count - 1 unconditionally, which is an out-of-bounds
stack read if the audio-FIFO-error IRQ fires first. It also fixes a
debug print that logged the previous channel count instead of the
rejected one. Needed before patch 3 for the same reason as patch 1:
that patch starts exercising the audio-FIFO-error IRQ path in a
configuration (shared I2S bus) where it is actually likely to fire
before any display has ever been hotplugged.

Patch 3 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 4 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/
---
v3:
 * new patch 1: cancel link_works, hdcp_wait_ksv_list and hdcp_work in
   it6505_i2c_remove(), fixing a pre-existing use-after-free flagged
   by automated review of v2
 * new patch 2: guard it6505_enable_audio_infoframe() against
   channel_count == 0 and fix a debug log printing the wrong channel
   count, both pre-existing bugs flagged by automated review of v2
 * patch 3 (formerly patch 1): serialise it6505_enable_audio() /
   it6505_disable_audio() with a new audio_lock mutex, resolving the
   "input welcome on whether a lock is warranted" note in the commit
   message; track explicit mute state so the audio-FIFO-error IRQ
   no longer re-enables audio out from under an ALSA-requested mute,
   both issues flagged by automated review of v2
 * patch 4 (formerly patch 2): no changes

v2: https://lore.kernel.org/all/cover.1784561622.git.daniel@makrotopia.org/
 * 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 (3):
  drm/bridge: it6505: cancel outstanding work before teardown in
    remove()
  drm/bridge: it6505: guard against zero channel count in audio
    infoframe
  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 | 138 +++++++++++++++++++++++-----
 1 file changed, 116 insertions(+), 22 deletions(-)


base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
-- 
2.55.0

[PATCH v3 1/4] drm/bridge: it6505: cancel outstanding work before teardown in remove()

[Daniel Golle]

it6505_i2c_remove() tears down the bridge and frees its state without
cancelling any of the driver's background work. struct it6505 is
allocated with devm_drm_bridge_alloc(), so devres frees it as soon as
remove() returns; link_works, hdcp_wait_ksv_list, hdcp_work and
extcon_wq can all still be pending or running at that point and will
then dereference freed memory.

link_works and extcon_wq are flushed in it6505_bridge_detach(), but
that only runs if DRM core calls the bridge's .detach() before the
i2c client is unbound -- not guaranteed if the i2c driver is unbound
independently of the DRM device that consumes it. hdcp_wait_ksv_list
and hdcp_work are never cancelled anywhere.

Cancel all four synchronously in remove() itself so teardown is safe
regardless of ordering. cancel_work_sync() on extcon_wq is safe even
when it was never INIT_WORK()'d, since it6505 is zero-allocated and a
zeroed work_struct is reported as not pending.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: new patch, split out to fix a pre-existing use-after-free flagged
    by automated review of v2's audio series; needed before that
    series adds another cancel_delayed_work_sync() call next to the
    ones this commit is adding.

 drivers/gpu/drm/bridge/ite-it6505.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 8ecb43611dba..2f349aefb705 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3644,6 +3644,10 @@ static void it6505_i2c_remove(struct i2c_client *client)
 {
 	struct it6505 *it6505 = i2c_get_clientdata(client);
 
+	cancel_work_sync(&it6505->link_works);
+	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
+	cancel_delayed_work_sync(&it6505->hdcp_work);
+	cancel_work_sync(&it6505->extcon_wq);
 	drm_bridge_remove(&it6505->bridge);
 	drm_dp_aux_unregister(&it6505->aux);
 	it6505_debugfs_remove(it6505);
-- 
2.55.0

[PATCH v3 2/4] drm/bridge: it6505: guard against zero channel count in audio infoframe

[Daniel Golle]

it6505->audio.channel_count is zero from allocation until either DP
link training has run once (it6505_variable_config()) or a valid
hw_params call has cached a channel count. it6505_enable_audio() can
be reached before either of those has happened, e.g. from the
audio-FIFO-error IRQ, which is unmasked unconditionally at poweron.
it6505_enable_audio_infoframe() then indexes the 8-entry
audio_info_ca[] table with channel_count - 1, an out-of-bounds stack
read when channel_count is still 0.

Bail out of it6505_enable_audio_infoframe() when channel_count is not
yet known instead of indexing with it.

While here, fix the debug print in it6505_audio_setup_hw_params(): on
the invalid-channel-count path it logged the previously cached
channel_count instead of the actually-rejected params->cea.channels.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: new patch

 drivers/gpu/drm/bridge/ite-it6505.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 2f349aefb705..f2ed76a05f6b 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1575,6 +1575,9 @@ static void it6505_enable_audio_infoframe(struct it6505 *it6505)
 	struct device *dev = it6505->dev;
 	u8 audio_info_ca[] = { 0x00, 0x00, 0x01, 0x03, 0x07, 0x0B, 0x0F, 0x1F };
 
+	if (!it6505->audio.channel_count)
+		return;
+
 	DRM_DEV_DEBUG_DRIVER(dev, "infoframe channel_allocation:0x%02x",
 			     audio_info_ca[it6505->audio.channel_count - 1]);
 
@@ -2990,7 +2993,7 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,
 
 	if (params->cea.channels <= 1 || params->cea.channels > 8) {
 		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
-				     it6505->audio.channel_count);
+				     params->cea.channels);
 		return -EINVAL;
 	}
 
-- 
2.55.0

[PATCH v3 3/4] 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.

it6505_enable_audio()/it6505_disable_audio() can be called from
several contexts: DP link training (link_works), the HPD-low path,
the audio-FIFO-error IRQ, and now also the delayed audio-enable work
and the .mute_stream/.audio_shutdown hdmi_codec_ops added here. None
of these are mutually exclusive, so serialize the actual register
sequences with a new audio_lock mutex.

Also track the mute state requested through .mute_stream/
.audio_shutdown in it6505->audio.mute, and have the audio-FIFO-error
IRQ handler skip re-enabling audio while muted: without this, an
audio-FIFO error following an ALSA-requested mute would silently
re-enable audio and could send the very InfoFrame-without-data
sequence the delayed-enable logic above is meant to avoid.

Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3:
 * serialise it6505_enable_audio()/it6505_disable_audio() with a new
   audio_lock mutex, resolving the "input welcome on whether a lock
   is warranted" note below
 * track requested mute state in it6505->audio.mute and have the
   audio-FIFO-error IRQ handler skip re-enabling audio while muted,
   both addressing automated review of v2

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

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.
---
 drivers/gpu/drm/bridge/ite-it6505.c | 126 ++++++++++++++++++++++++----
 1 file changed, 108 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index f2ed76a05f6b..37036ef5403f 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -407,6 +407,7 @@ struct it6505_audio_data {
 	u8 i2s_data_delay;
 	u8 i2s_ws_channel;
 	u8 i2s_data_sequence;
+	bool mute;
 };
 
 struct it6505_audio_sample_rate_map {
@@ -439,6 +440,7 @@ struct it6505 {
 	struct mutex extcon_lock;
 	struct mutex mode_lock; /* used to bridge_detect */
 	struct mutex aux_lock; /* used to aux data transfers */
+	struct mutex audio_lock; /* serializes audio enable/disable */
 	struct regmap *regmap;
 	struct drm_display_mode source_output_mode;
 	struct drm_display_mode video_info;
@@ -476,6 +478,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;
@@ -1594,7 +1597,7 @@ static void it6505_enable_audio_infoframe(struct it6505 *it6505)
 			EN_AUD_CTRL_PKT);
 }
 
-static void it6505_disable_audio(struct it6505 *it6505)
+static void __it6505_disable_audio(struct it6505 *it6505)
 {
 	it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_AUD_MUTE, EN_AUD_MUTE);
 	it6505_set_bits(it6505, REG_AUDIO_SRC_CTRL, M_AUDIO_I2S_EN, 0x00);
@@ -1602,13 +1605,23 @@ static void it6505_disable_audio(struct it6505 *it6505)
 	it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, AUDIO_RESET);
 }
 
+static void it6505_disable_audio(struct it6505 *it6505)
+{
+	mutex_lock(&it6505->audio_lock);
+	__it6505_disable_audio(it6505);
+	mutex_unlock(&it6505->audio_lock);
+}
+
 static void it6505_enable_audio(struct it6505 *it6505)
 {
 	struct device *dev = it6505->dev;
 	int regbe;
 
 	DRM_DEV_DEBUG_DRIVER(dev, "start");
-	it6505_disable_audio(it6505);
+
+	mutex_lock(&it6505->audio_lock);
+
+	__it6505_disable_audio(it6505);
 
 	it6505_setup_audio_channel_status(it6505);
 	it6505_setup_audio_format(it6505);
@@ -1627,6 +1640,8 @@ static void it6505_enable_audio(struct it6505 *it6505)
 	DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz",
 			     regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);
 	it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_AUD_MUTE, 0x00);
+
+	mutex_unlock(&it6505->audio_lock);
 }
 
 static bool it6505_use_step_train_check(struct it6505 *it6505)
@@ -2323,19 +2338,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);
 }
@@ -2627,6 +2635,9 @@ static void it6505_irq_audio_fifo_error(struct it6505 *it6505)
 
 	DRM_DEV_DEBUG_DRIVER(dev, "audio fifo error Interrupt");
 
+	if (it6505->audio.mute)
+		return;
+
 	if (it6505_audio_input(it6505))
 		it6505_enable_audio(it6505);
 }
@@ -2963,7 +2974,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);
@@ -2977,9 +2988,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;
@@ -3034,18 +3045,55 @@ 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);
 
+	it6505->audio.mute = true;
+	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);
+
+	it6505->audio.mute = 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;
 
@@ -3056,6 +3104,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);
@@ -3565,6 +3646,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
 	mutex_init(&it6505->extcon_lock);
 	mutex_init(&it6505->mode_lock);
 	mutex_init(&it6505->aux_lock);
+	mutex_init(&it6505->audio_lock);
 
 	it6505->bridge.of_node = client->dev.of_node;
 	it6505->connector_status = connector_status_disconnected;
@@ -3615,6 +3697,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);
@@ -3647,6 +3735,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);
 	cancel_work_sync(&it6505->link_works);
 	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
 	cancel_delayed_work_sync(&it6505->hdcp_work);
-- 
2.55.0

[PATCH v3 4/4] 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>
---
v3: no changes

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 37036ef5403f..435601b6929e 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2999,9 +2999,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",
 				     params->cea.channels);
-- 
2.55.0

Re: [PATCH v3 2/4] drm/bridge: it6505: guard against zero channel count in audio infoframe

[Chen-Yu Tsai]

On Tue, Jul 21, 2026 at 10:26 AM Daniel Golle <daniel@makrotopia.org> wrote:
>
> it6505->audio.channel_count is zero from allocation until either DP
> link training has run once (it6505_variable_config()) or a valid
> hw_params call has cached a channel count. it6505_enable_audio() can
> be reached before either of those has happened, e.g. from the
> audio-FIFO-error IRQ, which is unmasked unconditionally at poweron.
> it6505_enable_audio_infoframe() then indexes the 8-entry
> audio_info_ca[] table with channel_count - 1, an out-of-bounds stack
> read when channel_count is still 0.
>
> Bail out of it6505_enable_audio_infoframe() when channel_count is not
> yet known instead of indexing with it.
>
> While here, fix the debug print in it6505_audio_setup_hw_params(): on
> the invalid-channel-count path it logged the previously cached
> channel_count instead of the actually-rejected params->cea.channels.
>
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Re: [PATCH v3 1/4] drm/bridge: it6505: cancel outstanding work before teardown in remove()

[Chen-Yu Tsai]

Hi Daniel,

On Tue, Jul 21, 2026 at 10:41 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] Calling cancel_work_sync() on the potentially uninitialized extcon_wq triggers a WARN_ON(!work->func) kernel splat.
>
> Pre-existing issues:
> - [High] Canceling work items without first disabling their scheduling sources (IRQ and extcon notifier) leaves a race window where works can be requeued, leading to a use-after-free.
> --
>
> --- Patch [1]: [PATCH v3 1/4] drm/bridge: it6505: cancel outstanding work before teardown in remove() ---
> commit 06b35e4209da852ce0af8e4b1dd2aec2e9e172b4
> Author: Daniel Golle <daniel@makrotopia.org>
>
> drm/bridge: it6505: cancel outstanding work before teardown in remove()
>
> This commit adds calls to cancel_work_sync() and cancel_delayed_work_sync()
> in it6505_i2c_remove() to ensure background works do not dereference freed
> memory after the device is unbound. It notes that extcon_wq might not be
> initialized, but claims a zero-allocated work_struct is safe to cancel.
>
> > cancel_work_sync() on extcon_wq is safe even
> > when it was never INIT_WORK()'d, since it6505 is zero-allocated and a
> > zeroed work_struct is reported as not pending.
>
> [Severity: Medium]
> Is this description accurate?
>
> Looking at __flush_work() in kernel/workqueue.c, the workqueue core asserts
> WARN_ON(!work->func) when flushing a work item. If the DRM bridge was never
> attached, extcon_wq remains zero-allocated.
>
> During module removal, calling cancel_work_sync(&it6505->extcon_wq) will
> unconditionally call __flush_work(work, true), which immediately triggers
> the WARN_ON because the work function is NULL. On systems with panic_on_warn,
> this leads to a kernel panic.

Sashiko seems to be correct here. This is going to cause a very loud
warning, which we likely don't want.

ChenYu

> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > index 8ecb43611dba8..2f349aefb7054 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -3644,6 +3644,10 @@ static void it6505_i2c_remove(struct i2c_client *client)
> >  {
> >       struct it6505 *it6505 = i2c_get_clientdata(client);
> >
> > +     cancel_work_sync(&it6505->link_works);
> > +     cancel_work_sync(&it6505->hdcp_wait_ksv_list);
> > +     cancel_delayed_work_sync(&it6505->hdcp_work);
> > +     cancel_work_sync(&it6505->extcon_wq);
>
> [Severity: High]
> This is a pre-existing issue, but does this leave a race window where these
> work items could be requeued?
>
> The driver uses devres for the IRQ and extcon notifier, which remain active
> during these cancel_work_sync() calls. Furthermore, the hardware interrupt
> is disabled after the works are cancelled.
>
> If we look further down in it6505_i2c_remove():
>
>         ...
>         drm_bridge_remove(&it6505->bridge);
>         drm_dp_aux_unregister(&it6505->aux);
>         it6505_debugfs_remove(it6505);
>         it6505_poweroff(it6505);
>         ...
>
> The interrupt is only disabled inside it6505_poweroff(it6505). If an
> interrupt or extcon event fires immediately after the work is cancelled but
> before it6505_poweroff() and devres cleanup, won't the handler run and
> re-queue the work via schedule_work()?
>
> This could lead to a use-after-free when devres frees the it6505 structure
> while the re-queued work is still pending.
>
> >       drm_bridge_remove(&it6505->bridge);
> >       drm_dp_aux_unregister(&it6505->aux);
> >       it6505_debugfs_remove(it6505);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784600387.git.daniel@makrotopia.org?part=1