drm/bridge: it6505: DP audio support + shared-DAI hw_params fix
Subsystems (4)
v4 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 feedback on that original posting 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. The extcon notifier and the threaded IRQ are quiesced before the work items are cancelled, so they cannot requeue them. It is needed before patch 4 so that wiring up the new hdmi-audio-codec platform_device does not add another caller into an already-broken teardown path.
Patch 2 adds the missing pm_runtime_disable() in the same teardown path, another pre-existing bug: unbind followed by a rebind triggered "Unbalanced pm_runtime_enable!".
Patch 3 fixes a third 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 4 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 4 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 5 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/
---
v4:
* patch 1: quiesce the scheduling sources before cancelling work:
unregister the extcon notifier (made idempotent for that) and
disable the IRQ first, and only cancel the work items once the
DRM-facing interfaces are removed; initialise extcon_wq at probe
time to avoid a WARN_ON(!work->func) splat when unbinding a
never-attached bridge; retitled. Both flagged by Sashiko AI
review, the WARN_ON also confirmed by Chen-Yu Tsai.
* new patch 2: disable runtime PM on remove, a pre-existing issue
flagged by Sashiko AI review
* patch 3 (formerly 2): no changes, collected Chen-Yu Tsai's
Reviewed-by
* patch 4 (formerly 3): start out muted until ALSA unmutes the
stream; update the mute state and the cached stream parameters
under audio_lock; probe the audio input and enable audio under
audio_lock in the audio-FIFO-error IRQ handler, re-checking the
mute state under the lock; unregister the audio codec device only
after event sources and work items are quiesced. All flagged by
Sashiko AI review.
* patch 5 (formerly 4): move the audio parameter defaults from it6505_variable_config() to probe time so a display hotplug no
longer clobbers the parameters cached from hw_params, flagged by
Sashiko AI review
v3: https://lore.kernel.org/all/[email protected]/
* 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/[email protected]/
* 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 (4):
drm/bridge: it6505: quiesce event sources and work on remove() drm/bridge: it6505: disable runtime PM on 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 | 213 +++++++++++++++++++++------- 1 file changed, 163 insertions(+), 50 deletions(-)
base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
Signature · 1 line
2.55.0
PATCH v4 1/5 drm/bridge: it6505: quiesce event sources and work on remove()
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.
Cancelling the work items alone would not be enough: the threaded IRQ handler and the extcon notifier both stay live until devres teardown after remove() returns, and both can requeue a work item right after it was cancelled. Remove the DRM-facing interfaces first, then quiesce the scheduling sources by unregistering the extcon notifier and disabling the interrupt, and only then cancel the work items.
Unregistering the extcon notifier on remove() means it6505_remove_notifier_module() can now run twice, from .detach() and from remove(), while devm_extcon_unregister_notifier() WARNs when the notifier is not registered. Track the registration in event_nb.notifier_call to make the helper idempotent.
Initialise extcon_wq in probe rather than in it6505_use_notifier_module(): cancel_work_sync() on a work item that was never INIT_WORK()'d trips WARN_ON(!work->func) in __flush_work(), which would fire when unbinding a device whose bridge was never attached.
---
v4:
* retitled from "drm/bridge: it6505: cancel outstanding work before teardown in remove()"
* quiesce the scheduling sources (extcon notifier, threaded IRQ) before cancelling the work items, and cancel only after the DRM-facing interfaces are removed, closing the requeue race flagged by Sashiko AI review
* make it6505_remove_notifier_module() idempotent so both .detach() and remove() can call it
* initialise extcon_wq in probe instead of relying on cancel_work_sync() coping with a zeroed work_struct, which would trip WARN_ON(!work->func) in __flush_work(); flagged by Sashiko AI review and confirmed by Chen-Yu Tsai
v3: new patch
drivers/gpu/drm/bridge/ite-it6505.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/gpu/drm/bridge/ite-it6505.c +15 -7
@@ -2935,11 +2935,11 @@ static int it6505_use_notifier_module(struct it6505 *it6505)struct device *dev = it6505->dev; it6505->event_nb.notifier_call = it6505_extcon_notifier; -INIT_WORK (&it6505->extcon_wq, it6505_extcon_work); ret =devm_extcon_register_notifier (it6505->dev, it6505->extcon,EXTCON_DISP_DP , &it6505->event_nb);if (ret) { + it6505->event_nb.notifier_call =NULL ;dev_err (dev,"failed to register notifier for DP" );return ret; }
@@ -2951,13 +2951,14 @@ static int it6505_use_notifier_module(struct it6505 *it6505)static void it6505_remove_notifier_module (struct it6505 *it6505) { -if (it6505->extcon) { -devm_extcon_unregister_notifier (it6505->dev, - it6505->extcon,EXTCON_DISP_DP , - &it6505->event_nb); +if (!it6505->extcon || !it6505->event_nb.notifier_call) +return ; -flush_work (&it6505->extcon_wq); - } +devm_extcon_unregister_notifier (it6505->dev, it6505->extcon, +EXTCON_DISP_DP , &it6505->event_nb); + it6505->event_nb.notifier_call =NULL ; + +flush_work (&it6505->extcon_wq); }static void __maybe_unused it6505_delayed_audio (struct work_struct *work)
@@ -3615,6 +3616,7 @@ static int it6505_i2c_probe(struct i2c_client *client)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); +INIT_WORK (&it6505->extcon_wq, it6505_extcon_work);init_completion (&it6505->extcon_completion);memset (it6505->dpcd,0 ,sizeof (it6505->dpcd)); it6505->powered = false;
@@ -3647,6 +3649,12 @@ static void it6505_i2c_remove(struct i2c_client *client)drm_bridge_remove (&it6505->bridge);drm_dp_aux_unregister (&it6505->aux);it6505_debugfs_remove (it6505); +it6505_remove_notifier_module (it6505); +disable_irq (it6505->irq); +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);it6505_poweroff (it6505);it6505_remove_edid (it6505); }
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v4 1/5] drm/bridge: it6505: quiesce event sources and work on remove()
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:
New issues:
- [Medium] TOCTOU race in it6505_remove_notifier_module() can trigger a devres WARN_ON.
Pre-existing issues:
- [High] State mismatch causing permanently disabled IRQs when regulator_disable() fails in it6505_poweroff(). --
--- Patch [1]: [PATCH v4 1/5] drm/bridge: it6505: quiesce event sources and work on remove() --- commit edc834b1a3770ae6b495236b92078500aa7563c6
Author: Daniel Golle <[email protected]>
Subject: drm/bridge: it6505: quiesce event sources and work on remove()
This patch tears down the bridge and cancels background work in the correct order during remove(). It unregisters the extcon notifier and disables interrupts before cancelling work items to prevent them from requeuing.
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
Quoted 6 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-2@@ -2951,13 +2951,14 @@ static int it6505_use_notifier_module(struct it6505 *it6505)static void it6505_remove_notifier_module (struct it6505 *it6505) { -if (it6505->extcon) { - devm_extcon_unregister_notifier(it6505->dev,
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_use_notifier_module(struct it6505 *it6505) · line 2959
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +5
-4- it6505->extcon,EXTCON_DISP_DP , - &it6505->event_nb); +if (!it6505->extcon || !it6505->event_nb.notifier_call) +return ; -flush_work (&it6505->extcon_wq); - } +devm_extcon_unregister_notifier (it6505->dev, it6505->extcon, +EXTCON_DISP_DP , &it6505->event_nb); + it6505->event_nb.notifier_call =NULL ;
[Severity: Medium]
Could this introduce a time-of-check to time-of-use (TOCTOU) race condition?
The commit message mentions making this function idempotent so it can be called from both .detach() and remove(). However, the check of it6505->event_nb.notifier_call and its subsequent modification don't appear to be synchronized.
If .detach() and remove() run concurrently, could both threads pass the check simultaneously and call devm_extcon_unregister_notifier(), triggering a WARN_ON in devres_release() due to a double-free attempt?
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +2
-0+ +flush_work (&it6505->extcon_wq); }
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_i2c_remove(struct i2c_client *client) · line 3658
Quoted 11 lines
drivers/gpu/drm/bridge/ite-it6505.c +6
-0@@ -3647,6 +3649,12 @@ static void it6505_i2c_remove(struct i2c_client *client)drm_bridge_remove (&it6505->bridge);drm_dp_aux_unregister (&it6505->aux);it6505_debugfs_remove (it6505); +it6505_remove_notifier_module (it6505); +disable_irq (it6505->irq); +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);it6505_poweroff (it6505);
[Severity: High]
This isn't a bug introduced by this patch, but could this power off sequence lead to permanently disabled IRQs?
Looking at it6505_poweroff(), if regulator_disable() fails:
drivers/gpu/drm/bridge/ite-it6505.c:it6505_poweroff() {
...
disable_irq_nosync(it6505->irq);
...
if (pdata->pwr18) {
err = regulator_disable(pdata->pwr18);
if (err)
return err;
}
...
it6505->powered = false;
}The early return would leave it6505->powered == true despite the IRQ being disabled.
When the device is powered back on, it6505_poweron() checks the state:
drivers/gpu/drm/bridge/ite-it6505.c:it6505_poweron() {
if (it6505->powered) {
DRM_DEV_DEBUG_DRIVER(dev, "it6505 already powered on");
return 0;
}
...
enable_irq(it6505->irq);
...
}Would this early return skip enable_irq(), leaving the IRQ permanently disabled and the hardware deaf to hotplug and link training interrupts?
Quoted 2 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0it6505_remove_edid (it6505); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v4 2/5 drm/bridge: it6505: disable runtime PM on remove
In reply to Daniel Golleit6505_i2c_probe() enables runtime PM but it6505_i2c_remove() never disables it, leaving the device with an unbalanced disable depth after unbind. Binding the driver again then triggers the "Unbalanced pm_runtime_enable!" warning.
Disable runtime PM in remove() after all work items have been cancelled (the extcon work calls pm_runtime_get_sync() and pm_runtime_put_sync()) and before powering the chip off manually.
---
v4: new patch
drivers/gpu/drm/bridge/ite-it6505.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/bridge/ite-it6505.c +1 -0
@@ -3655,6 +3655,7 @@ static void it6505_i2c_remove(struct i2c_client *client)cancel_work_sync (&it6505->hdcp_wait_ksv_list);cancel_delayed_work_sync (&it6505->hdcp_work);cancel_work_sync (&it6505->extcon_wq); +pm_runtime_disable (&client->dev);it6505_poweroff (it6505);it6505_remove_edid (it6505); }
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v4 2/5] drm/bridge: it6505: disable runtime PM on remove
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:
Pre-existing issues:
- [High] The driver leaks the runtime PM usage counter if unbound while the DisplayPort cable is connected, breaking subsequent driver binds.
- [High] Resource registration asymmetry for the DRM DP AUX adapter causes registration failures on re-attach.
- [Medium] Missing error check on I2C reads in the IRQ handler causes negative error codes to be misinterpreted as phantom interrupts.
- [Low] The PM callback structure is assigned unconditionally, violating conditional compilation guidelines and creating dead code. --
--- Patch [2]: [PATCH v4 2/5] drm/bridge: it6505: disable runtime PM on remove --- commit 6b8ae3352685a773af0a6bbc5a90e489dc50ff6b
Author: Daniel Golle <[email protected]>
drm/bridge: it6505: disable runtime PM on remove
This commit disables runtime PM in the remove callback for the it6505 bridge driver. It cancels work items before disabling runtime PM and powering off the chip, which prevents the device from having an unbalanced disable depth after unbind.
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_i2c_remove(struct i2c_client *client) · line 3658
Quoted 9 lines
drivers/gpu/drm/bridge/ite-it6505.c +1
-0@@ -3655,6 +3655,7 @@ static void it6505_i2c_remove(struct i2c_client *client)cancel_work_sync (&it6505->hdcp_wait_ksv_list);cancel_delayed_work_sync (&it6505->hdcp_work);cancel_work_sync (&it6505->extcon_wq); +pm_runtime_disable (&client->dev);
[Severity: High]
This is a pre-existing issue, but does the driver leak the runtime PM usage counter if unbound while the DisplayPort cable is connected?
If the driver is unbound while a display is connected, it6505_extcon_work() has previously called pm_runtime_get_sync() and elevated the PM usage counter.
The remove callback disables runtime PM via pm_runtime_disable() but fails to drop the usage counter.
Because the usage count belongs to the struct device and persists across bind/unbind cycles, a subsequent bind will start with an elevated usage count. When extcon_work runs on the new bind, pm_runtime_get_sync() will return 1, bypassing the it6505_poweron() sequence.
Can this cause the hardware to remain unpowered and all subsequent I2C operations to fail?
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_i2c_remove(struct i2c_client *client) · line 3661
Quoted 3 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0it6505_poweroff (it6505);it6505_remove_edid (it6505); }
[Severity: High]
This is a pre-existing issue, but does the resource registration asymmetry for the DRM DP AUX adapter cause registration failures on re-attach?
The driver registers the AUX channel via drm_dp_aux_register() in the it6505_bridge_attach() callback, but unregisters it via drm_dp_aux_unregister() in the I2C driver's it6505_i2c_remove() callback.
If the parent DRM device is unbound, the bridge is detached but the I2C driver remains bound. When the DRM device is rebound, the bridge is attached again, and drm_dp_aux_register() is called a second time on the same instance without it having been unregistered.
Can this cause kernel warnings and registration failures in the I2C core?
[Severity: Medium]
This is a pre-existing issue, but can the missing error check on I2C reads in the IRQ handler cause negative error codes to be misinterpreted as phantom interrupts?
In it6505_int_threaded_handler(), the driver reads interrupt status registers via it6505_read(). If the I2C read fails, it returns a negative error code:
int_status[0] = it6505_read(it6505, INT_STATUS_01);
int_status[1] = it6505_read(it6505, INT_STATUS_02);
int_status[2] = it6505_read(it6505, INT_STATUS_03);This negative integer is stored in the int_status array and passed to it6505_test_bit(), which accesses it as an unsigned int.
Since a negative value has most of its bits set to 1, it6505_test_bit() evaluates to true for multiple interrupt sources.
Does this cause spurious execution of interrupt sub-handlers and unpredictable state changes?
[Severity: Low]
This is a pre-existing issue, but is the PM callback structure assigned unconditionally?
In the it6505_i2c_driver structure, the .pm field is assigned directly to &it6505_bridge_pm_ops.
According to the Power Management subsystem guidelines, this assignment should be wrapped with the pm_ptr() macro to ensure the pointer becomes NULL when CONFIG_PM is disabled.
Does this unconditional assignment create dead code and compiler warnings when power management is disabled?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v4 3/5 drm/bridge: it6505: guard against zero channel count in audio infoframe
In reply to Daniel Golleit6505->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.
---
v4: no changes
v3: new patch, split out to fix pre-existing bugs flagged by
automated review of v2's audio series; needed before that series starts exercising the audio-FIFO-error IRQ path in a configuration where channel_count == 0 is actually reachable.
drivers/gpu/drm/bridge/ite-it6505.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/ite-it6505.c +4 -1
@@ -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 ]);
@@ -2991,7 +2994,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 ; }
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v4 3/5] drm/bridge: it6505: guard against zero channel count in audio infoframe
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] Kernel panic due to division by zero when calculating audio input frequency.
- [High] Kernel panic due to division by zero when calculating video pixel clock. --
--- Patch [3]: [PATCH v4 3/5] drm/bridge: it6505: guard against zero channel count in audio infoframe --- commit 8ace85a52fcbf9bce4f2a154064b6799263b545d
Author: Daniel Golle <[email protected]>
Subject: drm/bridge: it6505: guard against zero channel count in audio infoframe
This commit guards against a zero channel count in it6505_enable_audio_infoframe() to prevent an out-of-bounds stack read. It also fixes a debug print logging an incorrect channel count on failure.
drivers/gpu/drm/bridge/ite-it6505.c
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
While reviewing this, I noticed a couple of pre-existing issues in this file:
drivers/gpu/drm/bridge/ite-it6505.c:it6505_calc_video_info() {
...
if (sum == 0) {
DRM_DEV_DEBUG_DRIVER(dev, "calc video timing error");
return;
}
sum /= 3;
pclk = 13500 * 2048 / sum;
...
}[Severity: High]
This is a pre-existing issue, but can this integer division shrink small sums to 0?
If the accumulated readings are very small (e.g., sum is 1 or 2), the integer division sum /= 3 will truncate the sum to 0.
The subsequent division for pclk will divide by zero and panic the kernel.
Should there be a check to ensure sum is still non-zero after the division by 3?
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_enable_audio_infoframe(struct it6505 *it6505) · line 1583
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +3
-0@@ -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 ]);
drivers/gpu/drm/bridge/ite-it6505.c:it6505_enable_audio() {
...
regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz",
regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);
...
}[Severity: High]
This isn't a bug introduced by this patch, but is it possible for regbe to be 0 here?
If the hardware measures no audio clock, this register could return 0.
The debug print macro then divides by regbe, which would cause a kernel panic due to division by zero.
Could we check if regbe is non-zero before using it as a divisor?
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v4 4/5 drm/bridge: it6505: Add audio support
In reply to Daniel GolleFrom: 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. The audio state starts out muted so the audio-FIFO-error IRQ cannot enable audio before ALSA has configured and unmuted the stream.
it6505_enable_audio()/it6505_disable_audio() can be called from several contexts: the HPD-low path, the audio-FIFO-error IRQ, 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. The mute state and the stream parameters cached by .hw_params are updated under the same lock, and the audio-FIFO-error IRQ handler (a threaded IRQ, so it may sleep) probes the audio input and re-enables audio in a single critical section, re-checking the mute state there so it cannot undo a concurrent ALSA-requested mute.
On remove, unregister the audio codec platform device only once the event sources and the other work items have been quiesced, so nothing can call back into the freed codec device through plugged_cb, and cancel the delayed audio work after that, when nothing is left that could requeue it.
---
v4:
* start out muted (audio.mute = true) so the audio-FIFO-error IRQ cannot enable audio before ALSA has configured and unmuted the stream
* update the mute state and the cached stream parameters under audio_lock, and split out __it6505_enable_audio() so the audio-FIFO-error IRQ handler can probe the audio input and enable audio in one critical section, re-checking the mute state under the lock; closes a race where a FIFO error racing with an ALSA mute could undo the mute
* unregister the audio codec platform device only after event sources and work items are quiesced, closing a use-after-free of codec_dev; cancel delayed_audio after the codec is gone
All of the above flagged by Sashiko AI review of v3.
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 his 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 | 162 +++++++++++++++++++++++----- 1 file changed, 133 insertions(+), 29 deletions(-)
drivers/gpu/drm/bridge/ite-it6505.c +133 -29
@@ -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,21 @@ static void it6505_disable_audio(struct it6505 *it6505)it6505_set_bits (it6505,REG_RESET_CTRL ,AUDIO_RESET ,AUDIO_RESET ); } -static void it6505_enable_audio (struct it6505 *it6505) +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); + +__it6505_disable_audio (it6505);it6505_setup_audio_channel_status (it6505);it6505_setup_audio_format (it6505);
@@ -1629,6 +1640,14 @@ static void it6505_enable_audio(struct it6505 *it6505)it6505_set_bits (it6505,REG_DATA_MUTE_CTRL ,EN_AUD_MUTE ,0x00 ); } +static void it6505_enable_audio (struct it6505 *it6505) +{ +mutex_lock (&it6505->audio_lock); +if (!it6505->audio.mute) +__it6505_enable_audio (it6505); +mutex_unlock (&it6505->audio_lock); +} +static bool it6505_use_step_train_check (struct it6505 *it6505) {if (it6505->link.revision >=0x12 )
@@ -2323,19 +2342,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,8 +2639,10 @@ static void it6505_irq_audio_fifo_error(struct it6505 *it6505)DRM_DEV_DEBUG_DRIVER (dev,"audio fifo error Interrupt" ); -if (it6505_audio_input (it6505)) -it6505_enable_audio (it6505); +mutex_lock (&it6505->audio_lock); +if (!it6505->audio.mute &&it6505_audio_input (it6505)) +__it6505_enable_audio (it6505); +mutex_unlock (&it6505->audio_lock); }static void it6505_irq_link_train_fail (struct it6505 *it6505)
@@ -2964,7 +2978,7 @@ 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);
@@ -2978,11 +2992,12 @@ 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; +u8 word_length;int i =0 ; DRM_DEV_DEBUG_DRIVER(dev, "%s %d Hz, %d bit, %d channels\n", __func__,
@@ -2998,8 +3013,6 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,return -EINVAL ; } - it6505->audio.channel_count = params->cea.channels; -while (i <ARRAY_SIZE (audio_sample_rate_map) && params->sample_rate != audio_sample_rate_map[i].sample_rate_value) {
@@ -3010,21 +3023,20 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505, params->sample_rate);return -EINVAL ; } - it6505->audio.sample_rate = audio_sample_rate_map[i].rate;switch (params->sample_width) {case 16 : - it6505->audio.word_length =WORD_LENGTH_16BIT ; + word_length =WORD_LENGTH_16BIT ;break ;case 18 : - it6505->audio.word_length =WORD_LENGTH_18BIT ; + word_length =WORD_LENGTH_18BIT ;break ;case 20 : - it6505->audio.word_length =WORD_LENGTH_20BIT ; + word_length =WORD_LENGTH_20BIT ;break ;case 24 :case 32 : - it6505->audio.word_length =WORD_LENGTH_24BIT ; + word_length =WORD_LENGTH_24BIT ;break ;default : DRM_DEV_DEBUG_DRIVER(dev, "wordlength: %d bit not support",
@@ -3032,21 +3044,70 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,return -EINVAL ; } +mutex_lock (&it6505->audio_lock); + it6505->audio.channel_count = params->cea.channels; + it6505->audio.sample_rate = audio_sample_rate_map[i].rate; + it6505->audio.word_length = word_length; +mutex_unlock (&it6505->audio_lock); +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); +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_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 __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_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) { +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 ; +} + +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;
@@ -3057,6 +3118,40 @@ 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; + + it6505->audio.mute = true; +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);
@@ -3566,6 +3661,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;
@@ -3616,6 +3712,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);
@@ -3658,6 +3760,8 @@ static void it6505_i2c_remove(struct i2c_client *client)cancel_work_sync (&it6505->hdcp_wait_ksv_list);cancel_delayed_work_sync (&it6505->hdcp_work);cancel_work_sync (&it6505->extcon_wq); +platform_device_unregister (it6505->audio_pdev); +cancel_delayed_work_sync (&it6505->delayed_audio);pm_runtime_disable (&client->dev);it6505_poweroff (it6505);it6505_remove_edid (it6505);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v4 4/5] drm/bridge: it6505: Add audio support
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Data race and dangling pointer dereference (Use-After-Free) in it6505_plugged_status_to_codec.
Pre-existing issues:
- [High] Use-After-Free of device_node ep in it6505_parse_dt.
- [High] Potential divide-by-zero kernel panic if the audio input frequency register reads as 0. --
--- Patch [4]: [PATCH v4 4/5] drm/bridge: it6505: Add audio support --- commit cb584e38b3bb2386c2c945750f3e9126c5d8acfa
Author: Jiaxin Yu <[email protected]>
drm/bridge: it6505: Add audio support
This commit adds audio support for the it6505 bridge by registering an hdmi-audio-codec platform device. It wires up the previously unused audio setup and shutdown helpers via hdmi_codec_ops. The patch also introduces locking to serialize audio enable/disable paths across different contexts.
Quoted 3 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void it6505_enable_audio(struct it6505 *it6505) · line 1641
Quoted 3 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0@@ -1629,6 +1640,14 @@ static void it6505_enable_audio(struct it6505 *it6505)it6505_set_bits (it6505,REG_DATA_MUTE_CTRL ,EN_AUD_MUTE ,0x00 ); }
[Severity: High]
This is a pre-existing issue, but since this patch wires up the audio helpers and makes them reachable, can this code cause a divide-by-zero panic?
In __it6505_enable_audio(), the variable regbe is read from the hardware:
regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz",
regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);If the hardware register reads as 0 (e.g., due to an uninitialized clock or bus error), regbe will be 0. If CONFIG_DRM_USE_DYNAMIC_DEBUG is disabled, the macro expands to a standard function call, meaning the division 6750 / regbe is unconditionally evaluated before the call, triggering a kernel panic.
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505, · line 3112
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +4
-4-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_hook_plugged_cb (struct device *dev, +void *data, +hdmi_codec_plugged_cb fn, +struct device *codec_dev) {struct it6505 *it6505 = data;
[Severity: High]
Can this cause a use-after-free if the hdmi-audio-codec driver is unbound concurrently?
When the codec driver unbinds, its remove function calls this hook to set it6505->plugged_cb and it6505->codec_dev to NULL. However, this is done without holding mode_lock.
Concurrently, if an HPD interrupt or DRM ioctl invokes it6505_bridge_detect(), it calls it6505_plugged_status_to_codec():
static void it6505_plugged_status_to_codec(struct it6505 *it6505)
{
enum drm_connector_status status = it6505->connector_status;
if (it6505->plugged_cb && it6505->codec_dev)
it6505->plugged_cb(it6505->codec_dev, ...);
}Could the detect thread pass the if-condition, get preempted while the codec unbinds and its module unloads, and then resume to call the now-dangling function pointer with the freed codec_dev?
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_i2c_probe(struct i2c_client *client) · line 3720
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +6
-0@@ -3616,6 +3712,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; + } +
[Severity: High]
This is a pre-existing issue, but does this code lead to a use-after-free during device probe?
In it6505_parse_dt() (which is called earlier in it6505_i2c_probe()), the ep node is released via of_node_put(ep) and subsequently dereferenced:
ep = of_graph_get_endpoint_by_regs(np, 1, 0);
of_node_put(ep);
if (ep) {
len = it6505_get_data_lanes_count(ep, 1, 4);If of_node_put(ep) drops the last reference, the node is freed, and the subsequent call to it6505_get_data_lanes_count(ep, ...) will access freed memory.
Quoted 1 line
drivers/gpu/drm/bridge/ite-it6505.c +0
-0INIT_WORK (&it6505->link_works, it6505_link_training_work);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v4 5/5 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.
For the cached parameters to survive until a display shows up they must also not be clobbered when it appears: it6505_variable_config() re-applies the compile-time audio defaults on every fresh DPCD read. Apply the defaults once at probe time instead, so a hotplug uses whatever ALSA last configured.
---
v4:
* move the audio parameter defaults from it6505_variable_config() to probe time: it runs again on every fresh DPCD read (i.e. on display hotplug) and would clobber the parameters cached from hw_params with the 2ch/48kHz defaults, defeating this patch's purpose; flagged by Sashiko AI review of v3
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 | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
drivers/gpu/drm/bridge/ite-it6505.c +10 -13
@@ -1369,16 +1369,6 @@ static void it6505_variable_config(struct it6505 *it6505) it6505->link_state =LINK_IDLE ; it6505->hdcp_desired =HDCP_DESIRED ; it6505->auto_train_retry =AUTO_TRAIN_RETRY ; - it6505->audio.select =AUDIO_SELECT ; - it6505->audio.sample_rate =AUDIO_SAMPLE_RATE ; - it6505->audio.channel_count =AUDIO_CHANNEL_COUNT ; - it6505->audio.type =AUDIO_TYPE ; - it6505->audio.i2s_input_format =I2S_INPUT_FORMAT ; - it6505->audio.i2s_justified =I2S_JUSTIFIED ; - it6505->audio.i2s_data_delay =I2S_DATA_DELAY ; - it6505->audio.i2s_ws_channel =I2S_WS_CHANNEL ; - it6505->audio.i2s_data_sequence =I2S_DATA_SEQUENCE ; - it6505->audio.word_length =AUDIO_WORD_LENGTH ;memset (it6505->sha1_input,0 ,sizeof (it6505->sha1_input));memset (it6505->bksvs,0 ,sizeof (it6505->bksvs)); }
@@ -3004,9 +2994,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);
@@ -3137,6 +3124,16 @@ static int it6505_register_audio_driver(struct device *dev) };struct platform_device *pdev; + it6505->audio.select =AUDIO_SELECT ; + it6505->audio.sample_rate =AUDIO_SAMPLE_RATE ; + it6505->audio.channel_count =AUDIO_CHANNEL_COUNT ; + it6505->audio.type =AUDIO_TYPE ; + it6505->audio.i2s_input_format =I2S_INPUT_FORMAT ; + it6505->audio.i2s_justified =I2S_JUSTIFIED ; + it6505->audio.i2s_data_delay =I2S_DATA_DELAY ; + it6505->audio.i2s_ws_channel =I2S_WS_CHANNEL ; + it6505->audio.i2s_data_sequence =I2S_DATA_SEQUENCE ; + it6505->audio.word_length =AUDIO_WORD_LENGTH ; it6505->audio.mute = true;INIT_DELAYED_WORK (&it6505->delayed_audio, it6505_delayed_audio);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v4 5/5] drm/bridge: it6505: Don't reject audio hw_params without an encoder
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Division by zero kernel panic in __it6505_enable_audio() when REG_AUDIO_INPUT_FREQ reads 0. --
--- Patch [5]: [PATCH v4 5/5] drm/bridge: it6505: Don't reject audio hw_params without an encoder --- commit ce61845cbb90df0a24913039e8ff09c7c4ff53e7
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() to ensure stream parameters are always cached, even when no display is attached. It also moves the application of audio parameter defaults to probe time.
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_audio_setup_hw_params(struct it6505 *it6505, · line 3009
Quoted 11 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-3@@ -3004,9 +2994,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 can this code cause a division by zero kernel panic?
Looking at __it6505_enable_audio() in drivers/gpu/drm/bridge/ite-it6505.c, if it6505_read() returns 0 (e.g. if no I2S clock is present or the read fails), regbe is used as a denominator:
regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ); DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz", regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);
When DRM debugging is enabled, this would evaluate the arguments and divide by zero. Should there be a check to ensure regbe is non-zero before it is used in the DRM_DEV_DEBUG_DRIVER macro?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5