[PATCH v5 00/11] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[PATCH v5 00/11] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[Daniel Golle]

Revive HDMI/DP audio support for the it6505, originally posted by
Jiaxin Yu [1], rebased onto current -next with the review feedback
addressed: &client->dev is gone (the driver gained it6505->dev) and
"#sound-dai-cells" has since been added by commit 325af1bef5b9
("dt-bindings: display: bridge: it6505: Add #sound-dai-cells").

Patches 1-9 fix pre-existing bugs found during review of earlier
versions of this series; they carry Fixes: tags and are ordered first
so stable trees can pick them.

Patch 10 wires up the it6505 audio helpers via hdmi_codec_ops,
unblocking the mt8186-mt6366 sound card which references it6505 as
the I2S3 codec. Patch 11 makes hw_params succeed with no display
attached, needed when the I2S bus is shared with another codec as on
MT8186 "steelix" Chromebooks, where the speaker path must keep
working regardless of the it6505 link state.

Tested on a MT8186 (google,steelix) Chromebook: the sound card comes
up, speakers, microphone and headset jack work. HDMI/DP audio output
itself could not be tested for lack of a suitable adapter cable.

[1] https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
---
v5:
 * patch 1: serialise notifier registration state with extcon_lock
 * patch 2: also drop the runtime PM usage ref held for a connected
   display; retitled
 * new patches 3-7 and 9: more pre-existing fixes from v4 review
   (AUX unregister symmetry, poweroff error path, IRQ status read
   errors, two divisions by zero, of_node_put before use)
 * patch 10 (was 4): update plugged_cb/codec_dev under mode_lock
 * patches 8 and 11: no changes

v4: https://lore.kernel.org/all/cover.1784681487.git.daniel@makrotopia.org/
 * patch 1: quiesce extcon notifier and IRQ before cancelling work,
   initialise extcon_wq in probe; retitled
 * new patch 2: disable runtime PM on remove
 * patch 3 (was 2): collected Chen-Yu Tsai's Reviewed-by
 * patch 4 (was 3): start out muted; mute state and cached
   parameters updated under audio_lock; unregister codec after
   quiesce
 * patch 5 (was 4): apply audio defaults at probe time

v3: https://lore.kernel.org/all/cover.1784600387.git.daniel@makrotopia.org/
 * new patches 1 and 2, fixing a pre-existing use-after-free on
   remove() and an out-of-bounds read with channel_count == 0
 * patch 3 (was 1): serialise audio enable/disable with audio_lock;
   track mute state so the FIFO-error IRQ cannot undo a mute
 * patch 4 (was 2): no changes

v2: https://lore.kernel.org/all/cover.1784561622.git.daniel@makrotopia.org/
 * keep and unregister the hdmi-codec platform_device on remove
 * initialise delayed_audio before registering the codec device
 * cancel delayed_audio synchronously on shutdown and remove
 * disable audio immediately when muting
 * patch 2: drop the encoder check instead of returning 0 early

Daniel Golle (10):
  drm/bridge: it6505: quiesce event sources and work on remove()
  drm/bridge: it6505: balance and disable runtime PM on remove
  drm/bridge: it6505: unregister DP AUX adapter on bridge detach
  drm/bridge: it6505: complete poweroff even if disabling regulators
    fails
  drm/bridge: it6505: bail out of the IRQ handler when status reads fail
  drm/bridge: it6505: avoid division by zero in pixel clock calculation
  drm/bridge: it6505: avoid division by zero in audio FS debug print
  drm/bridge: it6505: guard against zero channel count in audio
    infoframe
  drm/bridge: it6505: hold endpoint OF node reference while parsing it
  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 | 247 +++++++++++++++++++++-------
 1 file changed, 190 insertions(+), 57 deletions(-)


base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
-- 
2.55.0

[PATCH v5 01/11] drm/bridge: it6505: quiesce event sources and work on remove()

[Daniel Golle]

struct it6505 is freed by devres right after remove() returns, but
remove() cancels none of the driver's work items, so link_works,
hdcp_wait_ksv_list, hdcp_work and extcon_wq can still run afterwards
and dereference freed memory. Cancelling alone would not be enough:
the threaded IRQ and the extcon notifier stay live until devres
teardown and can requeue the works.

Unregister the extcon notifier and disable the IRQ first, then cancel
all work. Make it6505_remove_notifier_module() idempotent, tracking
registration in event_nb.notifier_call under extcon_lock, as both
.detach() and remove() call it now. Also initialise extcon_wq in
probe: cancel_work_sync() on a never-initialised work item trips
WARN_ON(!work->func).

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v5: serialise notifier registration state with extcon_lock

v4:
 * quiesce event sources before cancelling work; retitled
 * initialise extcon_wq in probe to avoid WARN_ON(!work->func)

v3: new patch

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 8ecb43611dba..e9c84e4fb3a0 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2934,15 +2934,18 @@ static int it6505_use_notifier_module(struct it6505 *it6505)
 	int ret;
 	struct device *dev = it6505->dev;
 
+	mutex_lock(&it6505->extcon_lock);
 	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;
+		mutex_unlock(&it6505->extcon_lock);
 		dev_err(dev, "failed to register notifier for DP");
 		return ret;
 	}
+	mutex_unlock(&it6505->extcon_lock);
 
 	schedule_work(&it6505->extcon_wq);
 
@@ -2951,13 +2954,16 @@ 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,
+	mutex_lock(&it6505->extcon_lock);
+	if (it6505->extcon && it6505->event_nb.notifier_call) {
+		devm_extcon_unregister_notifier(it6505->dev, it6505->extcon,
+						EXTCON_DISP_DP,
 						&it6505->event_nb);
-
-		flush_work(&it6505->extcon_wq);
+		it6505->event_nb.notifier_call = NULL;
 	}
+	mutex_unlock(&it6505->extcon_lock);
+
+	flush_work(&it6505->extcon_wq);
 }
 
 static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
@@ -3615,6 +3621,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 +3654,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);
 }
-- 
2.55.0

Re: [PATCH v5 01/11] drm/bridge: it6505: quiesce event sources and work on remove()

[sashiko-bot]

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

New issues:
- [High] Setting `notifier_call` to NULL during unregistration causes a NULL pointer dereference in concurrent extcon notifier traversals.

Pre-existing issues:
- [High] The extcon notifier callback can queue `extcon_wq` after the work has been cancelled, leading to a Use-After-Free of the `it6505` structure.
--

--- Patch [1]: [PATCH v5 01/11] drm/bridge: it6505: quiesce event sources and work on remove() ---
commit ea23622a6daa324ec330db52ae5d71446a7f3ab2
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: quiesce event sources and work on remove()

The patch attempts to fix use-after-free issues during device removal by
unregistering the extcon notifier, disabling IRQs, and cancelling work items
before the devres cleanup frees the it6505 structure. It also initializes the
extcon workqueue in the probe function.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 8ecb43611dba8..e9c84e4fb3a09 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2951,13 +2954,16 @@ 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,
> +	mutex_lock(&it6505->extcon_lock);
> +	if (it6505->extcon && it6505->event_nb.notifier_call) {
> +		devm_extcon_unregister_notifier(it6505->dev, it6505->extcon,
> +						EXTCON_DISP_DP,
>  						&it6505->event_nb);
> -
> -		flush_work(&it6505->extcon_wq);
> +		it6505->event_nb.notifier_call = NULL;

[Severity: High]
Does setting notifier_call to NULL here cause a NULL pointer dereference in
concurrent extcon notifier traversals?

Extcon uses raw notifiers, which do not provide built-in locking or RCU grace
periods during traversal. If an extcon state change triggers extcon_sync()
(which calls raw_notifier_call_chain()) concurrently with this unbind process,
setting it6505->event_nb.notifier_call to NULL while the traversal thread is
examining this notifier block can cause a crash.

If the traversal thread fetches nb->notifier_call inside notifier_call_chain()
immediately after it is set to NULL, it will attempt to execute it.

>  	}
> +	mutex_unlock(&it6505->extcon_lock);
> +
> +	flush_work(&it6505->extcon_wq);
>  }

[ ... ]

> @@ -3647,6 +3654,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);

[Severity: High]
This is a pre-existing issue, but does this fully prevent a Use-After-Free
if the extcon notifier callback is running concurrently?

In it6505_remove_notifier_module(), devm_extcon_unregister_notifier() removes
the raw notifier but does not wait for concurrently executing callbacks to finish.
If the it6505_extcon_notifier() callback is executing or preempted on another
CPU, it can call schedule_work(&it6505->extcon_wq) after cancel_work_sync()
returns.

When the device unbinds, devres frees the it6505 structure, which can cause
the scheduled workqueue to dereference freed memory.

>  	it6505_poweroff(it6505);
>  	it6505_remove_edid(it6505);
>  }

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

[PATCH v5 02/11] drm/bridge: it6505: balance and disable runtime PM on remove

[Daniel Golle]

Runtime PM is enabled in probe but never disabled on remove, so
rebinding the driver warns "Unbalanced pm_runtime_enable!". The
extcon work also holds a usage reference while a display is
connected, which unbinding leaks; the count survives in struct
device, so after a rebind the device never runtime-suspends again.

Drop the usage reference held for a connected display and disable
runtime PM in remove().

Fixes: 10517777d302 ("drm/bridge: it6505: Adapt runtime power management framework")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v5: also drop the usage ref held for a connected display; retitled

v4: new patch

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index e9c84e4fb3a0..6fcf2dc03d16 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3660,6 +3660,9 @@ 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);
+	if (it6505->extcon_state)
+		pm_runtime_put_sync(&client->dev);
+	pm_runtime_disable(&client->dev);
 	it6505_poweroff(it6505);
 	it6505_remove_edid(it6505);
 }
-- 
2.55.0

Re: [PATCH v5 02/11] drm/bridge: it6505: balance and disable runtime PM on remove

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free of devres-managed resources when I2C device is unbound while DRM pipeline is active.
--

--- Patch [2]: [PATCH v5 02/11] drm/bridge: it6505: balance and disable runtime PM on remove ---
commit e15fa2b94105d4f1aa7e633d5a8987820994caa9
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: balance and disable runtime PM on remove

This commit balances the usage reference held for a connected display and
explicitly disables runtime PM in the I2C remove path. This prevents a
device from leaking the reference count across rebinds and resolves an
unbalanced pm_runtime_enable warning.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -3660,6 +3660,9 @@ 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);
> +	if (it6505->extcon_state)
> +		pm_runtime_put_sync(&client->dev);
> +	pm_runtime_disable(&client->dev);
>  	it6505_poweroff(it6505);
>  	it6505_remove_edid(it6505);
>  }

[Severity: High]
This is a pre-existing issue, but does this remove sequence lead to a
use-after-free if the device is unbound via sysfs while the DRM pipeline is
active?

When it6505_i2c_remove() returns, the driver core cleans up devres-managed
resources allocated during probe, including the regmap and regulators.

The bridge container itself is allocated via devm_drm_bridge_alloc(), which
uses a kref to manage its lifetime, meaning it outlives the devres cleanup
as long as the DRM pipeline holds a reference.

If the DRM pipeline is subsequently torn down, it will call bridge callbacks
like it6505_bridge_atomic_disable() which attempt to access the freed regmap
and regulators. Is there a way to ensure the bridge is fully disconnected and
hardware resources are safe from late access during unbind?

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

[PATCH v5 03/11] drm/bridge: it6505: unregister DP AUX adapter on bridge detach

[Daniel Golle]

The DP AUX adapter is registered in .attach() but only unregistered
in i2c remove(). Unbinding and rebinding the DRM device while the
i2c driver stays bound registers the adapter a second time, which
fails. Unregister it in .detach() instead, and also on the .attach()
error path, which so far leaked the registration.

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

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 6fcf2dc03d16..ff6471d3ef7a 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3091,6 +3091,7 @@ static int it6505_bridge_attach(struct drm_bridge *bridge,
 		ret = it6505_use_notifier_module(it6505);
 		if (ret < 0) {
 			dev_err(dev, "use notifier module failed");
+			drm_dp_aux_unregister(&it6505->aux);
 			return ret;
 		}
 	}
@@ -3104,6 +3105,7 @@ static void it6505_bridge_detach(struct drm_bridge *bridge)
 
 	flush_work(&it6505->link_works);
 	it6505_remove_notifier_module(it6505);
+	drm_dp_aux_unregister(&it6505->aux);
 }
 
 static enum drm_mode_status
@@ -3652,7 +3654,6 @@ static void it6505_i2c_remove(struct i2c_client *client)
 	struct it6505 *it6505 = i2c_get_clientdata(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);
-- 
2.55.0

Re: [PATCH v5 03/11] drm/bridge: it6505: unregister DP AUX adapter on bridge detach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing `drm_dp_aux_unregister()` from `it6505_i2c_remove()` introduces a critical Use-After-Free vulnerability if the I2C driver is unbound while the DRM bridge remains attached.
--

--- Patch [3]: [PATCH v5 03/11] drm/bridge: it6505: unregister DP AUX adapter on bridge detach ---
commit b259e088ec0ed97d1f41a3a8463e232914e7c924
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: unregister DP AUX adapter on bridge detach

This commit moves the DP AUX adapter unregistration from the I2C remove
path to the bridge detach and attach error paths. This is intended to
prevent duplicate registrations when unbinding and rebinding the DRM
device.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -3652,7 +3654,6 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  	struct it6505 *it6505 = i2c_get_clientdata(client);
>  
>  	drm_bridge_remove(&it6505->bridge);
> -	drm_dp_aux_unregister(&it6505->aux);

[Severity: High]
Does removing drm_dp_aux_unregister() here introduce a use-after-free
if the I2C driver is unbound while the DRM bridge remains attached?

If the I2C driver is unbound (for example, via sysfs) while the DRM
device is still active, the DP AUX character device (/dev/drm_dp_auxN)
appears to remain registered because drm_bridge_remove() does not invoke
it6505_bridge_detach() when the bridge is removed from the global list.

After it6505_i2c_remove() returns, the driver core's devres cleanup will
execute and free the it6505 structure and its managed resources, such as
the regmap.

If userspace subsequently interacts with the leaked character device,
it can invoke it6505_aux_transfer() which dereferences the freed memory:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_aux_transfer() {
	struct it6505 *it6505 = container_of(aux, struct it6505, aux);
	...
	// accesses freed it6505 and performs I/O on freed it6505->regmap
}

Should drm_dp_aux_unregister() be kept in the I2C remove path as well,
or is there another mechanism guaranteeing the AUX character device is
unregistered before devres cleanup occurs?

>  	it6505_debugfs_remove(it6505);
>  	it6505_remove_notifier_module(it6505);
>  	disable_irq(it6505->irq);

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

[PATCH v5 04/11] drm/bridge: it6505: complete poweroff even if disabling regulators fails

[Daniel Golle]

it6505_poweroff() returns early when regulator_disable() fails,
leaving it6505->powered set with the IRQ already disabled. The next
it6505_poweron() then takes its early return and never re-enables the
IRQ, leaving the bridge deaf to hotplug and link training interrupts.
Log the error and complete the power-off state transition instead.

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

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index ff6471d3ef7a..0f79ddfd1501 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2810,13 +2810,15 @@ static int it6505_poweroff(struct it6505 *it6505)
 	if (pdata->pwr18) {
 		err = regulator_disable(pdata->pwr18);
 		if (err)
-			return err;
+			dev_err(dev, "cannot disable pwr18 regulator: %d",
+				err);
 	}
 
 	if (pdata->ovdd) {
 		err = regulator_disable(pdata->ovdd);
 		if (err)
-			return err;
+			dev_err(dev, "cannot disable ovdd regulator: %d",
+				err);
 	}
 
 	it6505->powered = false;
-- 
2.55.0

Re: [PATCH v5 04/11] drm/bridge: it6505: complete poweroff even if disabling regulators fails

[sashiko-bot]

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

New issues:
- [High] Ignoring regulator_disable() failure causes a permanent regulator reference leak and hardware state corruption.

Pre-existing issues:
- [High] I2C over AUX reads and writes silently ignore timeout and NACK errors, returning success with garbage data.
- [High] Device power state functions lack synchronization, leading to race conditions between extcon events and system suspend.
- [High] Background works are not cancelled during system suspend, risking I/O operations on powered-down hardware.
- [High] Regulators are disabled in the same order they are enabled, violating power sequencing constraints.
--

--- Patch [4]: [PATCH v5 04/11] drm/bridge: it6505: complete poweroff even if disabling regulators fails ---
commit f1926053557b0f8b21512c835ff0e5a067508980
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: complete poweroff even if disabling regulators fails

This patch modifies the driver to continue the power-off sequence and set
the powered state to false even if regulator_disable() fails. This aims
to prevent the bridge from being left deaf to hotplug interrupts by ensuring
the state transition completes.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index ff6471d3ef7ab..0f79ddfd1501e 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2810,13 +2810,15 @@ static int it6505_poweroff(struct it6505 *it6505)
>  	if (pdata->pwr18) {
>  		err = regulator_disable(pdata->pwr18);

[Severity: High]
This is a pre-existing issue, but does disabling the regulators in the same
order they were enabled violate power sequencing constraints?

In it6505_poweron(), pwr18 is enabled before ovdd. Here in
it6505_poweroff(), pwr18 is disabled before ovdd.

Could removing core power before I/O power cause back-powering and latch-up
in CMOS logic, potentially damaging the hardware?

>  		if (err)
> -			return err;
> +			dev_err(dev, "cannot disable pwr18 regulator: %d",
> +				err);

[Severity: High]
If regulator_disable() fails (for example, due to an I2C timeout on a PMIC),
does ignoring the error here cause a permanent regulator reference leak?

When the regulator core returns an error during disable, it does not
decrement the consumer's enable_count.

Since this patch ignores the error and forces it6505->powered to false, the
next power-on will increment the enable_count from 1 to 2.

Because the count never reaches 0 again, will the regulator core permanently
leave the hardware powered on?

[ ... ]

While reviewing this driver, I also noticed a few other pre-existing issues:

it6505_aux_i2c_readb() {
    for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
        // ...
        if (ret >= 0)
            break;
    }

    for (i = 0; i < size; i++)
        buf[i] = it6505_read(it6505, REG_AUX_USER_RXB(0 + i));

    return size;
}

[Severity: High]
This is a pre-existing issue, but does this I2C read silently ignore timeout
and NACK errors?

When an AUX I2C transfer encounters a NACK or timeout, it6505_aux_i2c_wait()
returns a negative error code.

If the retry loop exhausts all retries without breaking, the function
unconditionally executes the register read loop and returns the requested
size as if the transfer succeeded.

Can this result in the DRM core receiving uninitialized memory or garbage
data instead of an error, leading to corrupted EDID parsing?


it6505_extcon_work() {
    // ...
    if (ret < 0)
        it6505_poweron(it6505);
}

[Severity: High]
This is a pre-existing issue, but do the device power state functions lack
synchronization?

During system suspend, pm_runtime_force_suspend() calls
it6505_bridge_suspend(), which invokes it6505_poweroff() without acquiring
extcon_lock.

Concurrently, if an extcon event triggers it6505_extcon_work() on the
non-freezable system_wq, it attempts pm_runtime_get_sync().

If runtime PM is disabled during suspend, this fails and the fallback path
directly calls it6505_poweron().

Could this race condition lead to unbalanced IRQ enable depths or corrupted
regulator states?


it6505_bridge_suspend() {
    // ...
    it6505_remove_edid(it6505);
    return it6505_poweroff(it6505);
}

[Severity: High]
This is a pre-existing issue, but does the suspend handler fail to cancel
background works before powering off the hardware?

it6505_bridge_suspend() calls it6505_poweroff() but does not call
cancel_work_sync() for link_works or extcon_wq.

Because these works are scheduled on the non-freezable system_wq, could
they execute concurrently during the suspend process and attempt I/O
operations on powered-down hardware?

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

[PATCH v5 05/11] drm/bridge: it6505: bail out of the IRQ handler when status reads fail

[Daniel Golle]

When reading the interrupt status registers fails, the negative error
codes end up in int_status[], where it6505_test_bit() sees almost all
bits set: every interrupt sub-handler runs on garbage and the error
values are even written back to the status registers. Return IRQ_NONE
instead.

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

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 0f79ddfd1501..0e141310ed3c 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2713,6 +2713,11 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
 	int_status[1] = it6505_read(it6505, INT_STATUS_02);
 	int_status[2] = it6505_read(it6505, INT_STATUS_03);
 
+	if (int_status[0] < 0 || int_status[1] < 0 || int_status[2] < 0) {
+		pm_runtime_put_sync(dev);
+		return IRQ_NONE;
+	}
+
 	it6505_write(it6505, INT_STATUS_01, int_status[0]);
 	it6505_write(it6505, INT_STATUS_02, int_status[1]);
 	it6505_write(it6505, INT_STATUS_03, int_status[2]);
-- 
2.55.0

[PATCH v5 06/11] drm/bridge: it6505: avoid division by zero in pixel clock calculation

[Daniel Golle]

it6505_calc_video_info() checks the sum of the three pixel clock
counter samples for zero before dividing it by 3, so sums of 1 or 2
truncate to 0 and the following pixel clock calculation divides by
zero. Divide first and check the result.

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

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 0e141310ed3c..fc95f8d4f412 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -750,12 +750,13 @@ static void it6505_calc_video_info(struct it6505 *it6505)
 		sum += rddata;
 	}
 
+	sum /= 3;
+
 	if (sum == 0) {
 		DRM_DEV_DEBUG_DRIVER(dev, "calc video timing error");
 		return;
 	}
 
-	sum /= 3;
 	pclk = 13500 * 2048 / sum;
 	it6505->video_info.clock = pclk;
 	it6505->video_info.hdisplay = hdew;
-- 
2.55.0

[PATCH v5 07/11] drm/bridge: it6505: avoid division by zero in audio FS debug print

[Daniel Golle]

REG_AUDIO_INPUT_FREQ reads back 0 when no audio clock has been
latched, and it6505_read() returns a negative errno on i2c failure.
The debug print divides by the readback, and its arguments are
evaluated even with debug output disabled. Only print when the value
is usable as a divisor.

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

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index fc95f8d4f412..81d2c9644531 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1622,8 +1622,11 @@ static void it6505_enable_audio(struct it6505 *it6505)
 	it6505_set_bits(it6505, REG_AUDIO_SRC_CTRL, AUDIO_FIFO_RESET, 0x00);
 	it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00);
 	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 (regbe > 0)
+		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);
 }
 
-- 
2.55.0

Re: [PATCH v5 07/11] drm/bridge: it6505: avoid division by zero in audio FS debug print

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing I2C read error handling in `it6505_audio_input()` leads to logic reset and false positive audio enable.
--

--- Patch [7]: [PATCH v5 07/11] drm/bridge: it6505: avoid division by zero in audio FS debug print ---
commit ebf71cd14ba5699a1da0c18fc4c670be657e122d
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: avoid division by zero in audio FS debug print

This commit adds a check for regbe > 0 before printing debug information
about the audio input frequency in it6505_enable_audio(). This prevents a
division by zero when regbe is 0 or negative due to an I2C read failure.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index fc95f8d4f4129..81d2c96445318 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1622,8 +1622,11 @@ static void it6505_enable_audio(struct it6505 *it6505)
>  	it6505_set_bits(it6505, REG_AUDIO_SRC_CTRL, AUDIO_FIFO_RESET, 0x00);
>  	it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00);
>  	regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);

[Severity: High]
This is a pre-existing issue, but since we are handling I2C read failures
for REG_AUDIO_INPUT_FREQ here, I noticed a similar problem in
it6505_audio_input().

If it6505_read() fails, it returns a negative error code. In
it6505_audio_input():

	reg05 = it6505_read(it6505, REG_RESET_CTRL);
	it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00);
	usleep_range(3000, 4000);
	regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
	it6505_write(it6505, REG_RESET_CTRL, reg05);

The negative error code in reg05 is stored without validation and passed
directly to it6505_write(). Because the regmap uses 8-bit values, the integer
is truncated (e.g., -5 becomes 0xFB). Will this inadvertently assert critical
hardware reset bits such as ALL_LOGIC_RESET and VIDEO_RESET?

Additionally, at the end of it6505_audio_input():

	return regbe != 0xFF;

Since regbe can be a negative error code if the second read fails, this check
evaluates to true. Could this falsely signal that audio should be enabled,
leading to unexpected behavior?

> -	DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz",
> -			     regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);
> +	if (regbe > 0)
> +		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);
>  }

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

[PATCH v5 08/11] drm/bridge: it6505: guard against zero channel count in audio infoframe

[Daniel Golle]

it6505->audio.channel_count stays 0 until link training or a valid
hw_params call has run, but it6505_enable_audio() can be reached
before that from the audio-FIFO-error IRQ, making
it6505_enable_audio_infoframe() index the 8-entry audio_info_ca[]
table with -1. Bail out while the channel count is not yet known.

Also fix the debug print on the invalid-channel-count path, which
logged the previously cached count instead of the rejected one.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
---
v5: no changes

v4: no changes, collected Chen-Yu Tsai's Reviewed-by

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 81d2c9644531..e9208d827ce3 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1576,6 +1576,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]);
 
@@ -3007,7 +3010,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 v5 09/11] drm/bridge: it6505: hold endpoint OF node reference while parsing it

[Daniel Golle]

it6505_parse_dt() drops the reference to each endpoint node right
after looking it up and then keeps reading its properties, racing
with the node being freed under CONFIG_OF_DYNAMIC. Put the node only
once parsing is done.

Fixes: 380d920b582d ("drm/bridge: add it6505 driver to read data-lanes and link-frequencies from dt")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v5: new patch

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

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index e9208d827ce3..6f23203d5f2a 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3364,7 +3364,6 @@ static void it6505_parse_dt(struct it6505 *it6505)
 	}
 
 	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);
@@ -3377,13 +3376,13 @@ static void it6505_parse_dt(struct it6505 *it6505)
 			*max_lane_count = MAX_LANE_COUNT;
 			dev_err(dev, "error data-lanes, use default");
 		}
+		of_node_put(ep);
 	} else {
 		*max_lane_count = MAX_LANE_COUNT;
 		dev_err(dev, "error endpoint, use default");
 	}
 
 	ep = of_graph_get_endpoint_by_regs(np, 0, 0);
-	of_node_put(ep);
 
 	if (ep) {
 		len = of_property_read_variable_u64_array(ep,
@@ -3403,6 +3402,7 @@ static void it6505_parse_dt(struct it6505 *it6505)
 			dev_err(dev, "error link frequencies, use default");
 			*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;
 		}
+		of_node_put(ep);
 	} else {
 		dev_err(dev, "error endpoint, use default");
 		*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;
-- 
2.55.0

Re: [PATCH v5 09/11] drm/bridge: it6505: hold endpoint OF node reference while parsing it

[sashiko-bot]

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

Pre-existing issues:
- [High] The local variable `link_frequencies` is used uninitialized if `of_property_read_variable_u64_array()` returns 0.
--

--- Patch [9]: [PATCH v5 09/11] drm/bridge: it6505: hold endpoint OF node reference while parsing it ---
commit a152f40f457f0c59599e70de71a80bcac957ec14
Author: Daniel Golle <daniel@makrotopia.org>
Subject: drm/bridge: it6505: hold endpoint OF node reference while parsing it

This commit fixes an issue where the device tree endpoint node reference
is dropped before parsing is complete, which can cause a use-after-free
race if the node is freed dynamically. It defers the of_node_put() calls
to execute only after all property parsing has finished.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index e9208d827ce3d..6f23203d5f2a6 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
>  	ep = of_graph_get_endpoint_by_regs(np, 0, 0);
> -	of_node_put(ep);
>  
>  	if (ep) {
>  		len = of_property_read_variable_u64_array(ep,
>  							  "link-frequencies",
>  							  &link_frequencies, 0,
>  							  1);
>  		if (len >= 0) {
>  			do_div(link_frequencies, 1000);

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue
here in it6505_parse_dt(). If of_property_read_variable_u64_array() returns
0, does this code use an uninitialized link_frequencies stack variable?

The of_property_read_variable_u64_array() function is called with a minimum
size of 0. If the link-frequencies property in the device tree is empty or
less than 8 bytes long (such as a common syntax mistake where a 32-bit
integer is provided), the function computes the size as 0. It then returns
0 without modifying the link_frequencies output variable.

Because the subsequent check is "if (len >= 0)", it evaluates to true, and
do_div() will consume whatever garbage value happens to be on the stack.
Depending on the stack memory, this could result in incorrectly rejecting
valid display modes or causing unpredictable display pipeline failures.

Should this code check "if (len > 0)" instead to guarantee the variable was
actually populated before using it?

>  			it6505->max_dpi_pixel_clock = link_frequencies;
>  		} else {
>  			dev_err(dev, "error link frequencies, use default");
>  			*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;
>  		}
> +		of_node_put(ep);
>  	} else {
>  		dev_err(dev, "error endpoint, use default");
>  		*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;

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

[PATCH v5 10/11] 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: register
an "hdmi-audio-codec" platform device from probe and wire up the
previously unused audio helpers via hdmi_codec_ops. This unblocks the
mt8186-mt6366 sound card which references it6505 as the I2S3 codec.

Audio starts out muted and is only enabled once the stream is
unmuted, since some DP-to-HDMI dongles get into a bad state if the
InfoFrame is sent without audio data. Enable/disable can race between
the FIFO-error IRQ, the delayed enable work, the hdmi-codec ops and
the HPD-low path, so the register sequences, the mute state and the
cached stream parameters are serialised with a new audio_lock mutex.
plugged_cb/codec_dev updates take mode_lock, the lock
it6505_detect() holds when calling back into the codec.

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v5: update plugged_cb/codec_dev under mode_lock

v4:
 * start out muted
 * update mute state and cached parameters under audio_lock; the
   FIFO-error IRQ probes and enables audio in one locked section
 * unregister the codec device only after quiesce on remove

v3: serialise enable/disable with audio_lock; track mute state so
    the FIFO-error IRQ cannot undo a mute

v2 (respin of Jiaxin Yu's v3 on current -next):
 * drive enable/disable from .mute_stream (hdmi_codec_ops lost
   .trigger), use it6505->dev
 * keep and unregister the codec platform_device on remove, cancel
   delayed_audio on shutdown and remove, disable audio when muting

 drivers/gpu/drm/bridge/ite-it6505.c | 163 +++++++++++++++++++++++-----
 1 file changed, 134 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 6f23203d5f2a..75b287cda801 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;
@@ -1595,7 +1598,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);
@@ -1603,13 +1606,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);
@@ -1633,6 +1644,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)
@@ -2327,19 +2346,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);
 }
@@ -2631,8 +2643,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)
@@ -2980,7 +2994,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);
@@ -2994,11 +3008,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__,
@@ -3014,8 +3029,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) {
@@ -3026,21 +3039,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",
@@ -3048,27 +3060,111 @@ 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);
+
+	/*
+	 * Delay enabling audio until the stream is unmuted; InfoFrames
+	 * without audio data upset some DP-to-HDMI dongles.
+	 */
+	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;
 
+	mutex_lock(&it6505->mode_lock);
 	it6505->plugged_cb = fn;
 	it6505->codec_dev = codec_dev;
 	it6505_plugged_status_to_codec(it6505);
+	mutex_unlock(&it6505->mode_lock);
+
+	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;
 }
@@ -3584,6 +3680,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;
@@ -3634,6 +3731,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);
@@ -3675,6 +3778,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);
 	if (it6505->extcon_state)
 		pm_runtime_put_sync(&client->dev);
 	pm_runtime_disable(&client->dev);
-- 
2.55.0

Re: [PATCH v5 10/11] drm/bridge: it6505: Add audio support

[sashiko-bot]

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

New issues:
- [Medium] Data race in it6505_variable_config() clobbers active ALSA audio parameters during hotplug events.

Pre-existing issues:
- [High] Concurrent read-modify-write on REG_RESET_CTRL across a sleeping region causes hardware state corruption.
--

--- Patch [10]: [PATCH v5 10/11] drm/bridge: it6505: Add audio support ---
commit 5131d6a60c257db17a0ee56c8c36b3d7edb7c72f
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>

drm/bridge: it6505: Add audio support

Add audio support for it6505 by bridging to the hdmi-codec: register
an "hdmi-audio-codec" platform device from probe and wire up the
previously unused audio helpers via hdmi_codec_ops. This unblocks the
mt8186-mt6366 sound card which references it6505 as the I2S3 codec.

Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 6f23203d5f2a6..75b287cda8016 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2631,8 +2643,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);
>  }

[Severity: High]
This is a pre-existing issue, but does the call to it6505_audio_input()
here risk corrupting hardware state?

Looking at it6505_audio_input(), it performs a read, sleeps, and then
blindly writes back the old register value:

it6505_audio_input() {
    reg05 = it6505_read(it6505, REG_RESET_CTRL);
    it6505_set_bits(it6505, REG_RESET_CTRL, AUDIO_RESET, 0x00);
    usleep_range(3000, 4000);
    regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
    it6505_write(it6505, REG_RESET_CTRL, reg05);
}

If another thread (such as a modeset or hotplug handler) modifies other bits
in REG_RESET_CTRL (like VIDEO_RESET) during the 3-4ms sleep window, won't
this blind write overwrite those concurrent changes with stale data?

[ ... ]

> @@ -3048,27 +3060,111 @@ 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;
>  }

[Severity: Medium]
Does this leave a data race with it6505_variable_config()?

While this code correctly takes the new audio_lock to update the audio
parameters, it6505_variable_config() appears to reset these same
parameters to default values without holding the lock:

it6505_variable_config() {
    ...
    it6505->audio.sample_rate = AUDIO_SAMPLE_RATE;
    it6505->audio.channel_count = AUDIO_CHANNEL_COUNT;
    it6505->audio.type = AUDIO_TYPE;
    ...
}

Since it6505_variable_config() is called by it6505_irq_hpd() during hotplug
events, could an external hotplug event concurrently overwrite the ALSA
parameters negotiated here, causing the audio pipeline to use corrupted
settings when enabled?

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

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

[Daniel Golle]

it6505_audio_setup_hw_params() returns -ENODEV when no encoder is
attached. With it6505 registering an hdmi-audio-codec this runs for
every stream on the I2S DAI, and on mt8186-corsola, where the rt1019
speaker amplifier shares I2S3, the error tears down the whole DPCM
backend and breaks speaker playback whenever no display is attached.

The function only caches stream parameters, which needs no encoder.
Drop the check, and apply the audio defaults once at probe instead of
in it6505_variable_config(), which would clobber the cached
parameters again on every hotplug. Actual audio output remains gated
by it6505->powered.

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

v4: apply audio defaults at probe time instead of in
    it6505_variable_config()

v3: no changes

v2: drop the encoder check entirely instead of returning 0 early

 drivers/gpu/drm/bridge/ite-it6505.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 75b287cda801..ba8db1e36ff6 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1370,16 +1370,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));
 }
@@ -3020,9 +3010,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);
@@ -3154,6 +3141,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);
 
-- 
2.55.0