drm/bridge: it6505: DP audio support + shared-DAI hw_params fix
Subsystems (4)
v2 cover drm/bridge: it6505: DP audio support + shared-DAI hw_params fix
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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[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/[email protected]/
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
Signature · 1 line
2.55.0
PATCH v2 1/2 drm/bridge: it6505: Add audio support
From: Jiaxin Yu <[email protected]>
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.
---
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(-)
drivers/gpu/drm/bridge/ite-it6505.c +85 -16
@@ -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);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v2 1/2] drm/bridge: it6505: Add audio support
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void __maybe_unused it6505_delayed_audio(struct work_struct *work) · line 2974
Quoted 11 lines
drivers/gpu/drm/bridge/ite-it6505.c +3
-3@@ -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)?
Quoted 2 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0struct device *dev = it6505->dev;int i =0 ;
[ ... ]
Quoted 7 lines
drivers/gpu/drm/bridge/ite-it6505.c +6
-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);
drivers/gpu/drm/bridge/ite-it6505.c static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505, · line 3040
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +10
-0+ +/* +* 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?
Quoted 8 lines
drivers/gpu/drm/bridge/ite-it6505.c +8
-0+ }else { +queue_delayed_work (system_wq, &it6505->delayed_audio, +msecs_to_jiffies (180 )); + } + +return 0 ; +} +
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_i2c_remove(struct i2c_client *client) · line 3718
Quoted 9 lines
drivers/gpu/drm/bridge/ite-it6505.c +2
-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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v2 2/2 drm/bridge: it6505: Don't reject audio hw_params without an encoder
In reply to Daniel Golleit6505_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.
---
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(-)
drivers/gpu/drm/bridge/ite-it6505.c +0 -3
@@ -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);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v2 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
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 <[email protected]>
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.
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_audio_setup_hw_params(struct it6505 *it6505, · line 2984
Quoted 11 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-3@@ -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]);
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_audio_setup_hw_params(struct it6505 *it6505, · line 2984
Quoted 3 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0if (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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2