[PATCH v2 0/3] ASoC: hdac_hda: Fix module unbind/bind via sysfs

[PATCH v2 0/3] ASoC: hdac_hda: Fix module unbind/bind via sysfs

[Peter Ujfalusi]

Hi,

Changes since v1:
- Add pm_runtime_set_active() after snd_hda_codec_device_new() in
  hdac_hda_codec_probe() function as reported and suggested by
  Sashiko Bot.

Expanded test plan run on the device to cover the previously missed
scenario.

While looking at the reconfig lockup reported in
https://bugzilla.kernel.org/show_bug.cgi?id=222182
I checked whether the same can be done by unbinding and binding the HDA
codec driver via sysfs:

  echo -n ehdaudio0D0 > /sys/bus/hdaudio/drivers/snd_hda_codec_alc882/unbind
  echo -n ehdaudio0D0 > /sys/bus/hdaudio/drivers/snd_hda_codec_alc882/bind

The unbind works and the SOF card is removed, but the codec cannot be
bound again. This has been broken for a long time with hdac_hda; the
reconfig lockup itself is addressed by a separate patch.

Patch 1 and 2 fix the bind: the codec private data was lost on unbind
and the codec device type was left as HDA_DEV_ASOC, so no HDA codec
driver could match it anymore. Patch 2 depends on patch 1, otherwise
the bind would dereference NULL drvdata instead of failing.

Patch 3 fixes an unbalanced runtime PM disable in the component remove,
which leaves the codec with runtime PM disabled after the card is
re-created. This is also visible with machine driver unbind/bind.

Tested on PTL (ALC888 + iDisp): repeated codec and machine driver
unbind/bind, playback/capture, runtime suspend and module reload.

Regards,
Peter
---
Peter Ujfalusi (3):
  ASoC: hdac_hda: Allocate the private data at codec driver bind
  ASoC: hdac_hda: Restore the codec device type on unbind
  ASoC: hdac_hda: Fix codec runtime PM across remove and probe

 sound/soc/codecs/hdac_hda.c     | 36 ++++++++++++++++++++++++++++++++-
 sound/soc/sof/intel/hda-codec.c | 23 ++++-----------------
 2 files changed, 39 insertions(+), 20 deletions(-)

-- 
2.56.0

[PATCH v2 1/3] ASoC: hdac_hda: Allocate the private data at codec driver bind

[Peter Ujfalusi]

The SOF HDA code allocates struct hdac_hda_priv and stores it as drvdata
of the codec device before the codec driver is bound. The driver core
clears drvdata on unbind, so after unbinding the codec driver via sysfs
a subsequent bind dereferences a NULL pointer in hdac_hda_dev_probe().

All information in the private data can be derived from the codec
device itself, so allocate and set it up in hdac_hda_dev_probe() as a
device managed resource, which is released on unbind.

Remove the allocation from SOF, keeping the check for iDisp codecs
without display audio component.

Fixes: 3fd63658caed ("ASoC: Intel: Drop hdac_ext usage for codec device creation")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/hdac_hda.c     | 13 ++++++++++++-
 sound/soc/sof/intel/hda-codec.c | 23 ++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 7414be08d3d5..46617fc88277 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -619,10 +619,21 @@ static const struct snd_soc_component_driver hdac_hda_hdmi_codec = {
 
 static int hdac_hda_dev_probe(struct hdac_device *hdev)
 {
-	struct hdac_hda_priv *hda_pvt = dev_get_drvdata(&hdev->dev);
+	struct hdac_hda_priv *hda_pvt;
 	struct hdac_ext_link *hlink;
 	int ret;
 
+	hda_pvt = devm_kzalloc(&hdev->dev, sizeof(*hda_pvt), GFP_KERNEL);
+	if (!hda_pvt)
+		return -ENOMEM;
+
+	hda_pvt->codec = dev_to_hda_codec(&hdev->dev);
+	hda_pvt->dev_index = hdev->addr;
+	/* Intel iDisp (HDMI/DP) codec */
+	if (((hdev->vendor_id >> 16) & 0xFFFF) == 0x8086)
+		hda_pvt->need_display_power = true;
+	dev_set_drvdata(&hdev->dev, hda_pvt);
+
 	/* hold the ref while we probe */
 	hlink = snd_hdac_ext_bus_get_hlink_by_name(hdev->bus, dev_name(&hdev->dev));
 	if (!hlink) {
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 91f7ca22a4a7..a2b2ecf1e7e6 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -15,8 +15,6 @@
 #include "hda.h"
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
-#include "../../codecs/hdac_hda.h"
-
 #define CODEC_PROBE_RETRIES	3
 
 #define IDISP_VID_INTEL	0x80860000
@@ -148,7 +146,6 @@ static struct hda_codec *hda_codec_device_init(struct hdac_bus *bus, int addr, i
 /* probe individual codec */
 static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
 {
-	struct hdac_hda_priv *hda_priv;
 	struct hda_bus *hbus = sof_to_hbus(sdev);
 	struct hda_codec *codec;
 	u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) |
@@ -168,27 +165,15 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address)
 	dev_dbg(sdev->dev, "HDA codec #%d probed OK: response: %x\n",
 		address, resp);
 
-	hda_priv = devm_kzalloc(sdev->dev, sizeof(*hda_priv), GFP_KERNEL);
-	if (!hda_priv)
-		return -ENOMEM;
-
 	codec = hda_codec_device_init(&hbus->core, address, HDA_DEV_LEGACY);
 	ret = PTR_ERR_OR_ZERO(codec);
 	if (ret < 0)
 		return ret;
 
-	hda_priv->codec = codec;
-	hda_priv->dev_index = address;
-	dev_set_drvdata(&codec->core.dev, hda_priv);
-
-	if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) {
-		if (!hbus->core.audio_component) {
-			dev_dbg(sdev->dev,
-				"iDisp hw present but no driver\n");
-			ret = -ENOENT;
-			goto out;
-		}
-		hda_priv->need_display_power = true;
+	if ((resp & 0xFFFF0000) == IDISP_VID_INTEL && !hbus->core.audio_component) {
+		dev_dbg(sdev->dev, "iDisp hw present but no driver\n");
+		ret = -ENOENT;
+		goto out;
 	}
 
 	if (is_generic_config(hbus))
-- 
2.56.0

[PATCH v2 2/3] ASoC: hdac_hda: Restore the codec device type on unbind

[Peter Ujfalusi]

hdac_hda_codec_probe() changes the type of the codec device to
HDA_DEV_ASOC to prevent snd_hda_codec_unregister() from unregistering
the device when the card is freed.

The type is never restored, and hda_bus_match() requires the device and
driver types to be equal. After unbinding the codec driver via sysfs the
HDA_DEV_LEGACY codec drivers no longer match the device and bind fails
with -ENODEV.

Restore HDA_DEV_LEGACY with a device managed action registered before
the component, so it runs after the component and the card are gone.

Fixes: b60ee2e281b6 ("ASoC: hdac_hda: overwrite hdev type to HDA_DEV_ASOC")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/hdac_hda.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 46617fc88277..cae5711e979c 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -617,6 +617,13 @@ static const struct snd_soc_component_driver hdac_hda_hdmi_codec = {
 	.endianness		= 1,
 };
 
+static void hdac_hda_restore_dev_type(void *data)
+{
+	struct hdac_device *hdev = data;
+
+	hdev->type = HDA_DEV_LEGACY;
+}
+
 static int hdac_hda_dev_probe(struct hdac_device *hdev)
 {
 	struct hdac_hda_priv *hda_pvt;
@@ -634,6 +641,11 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 		hda_pvt->need_display_power = true;
 	dev_set_drvdata(&hdev->dev, hda_pvt);
 
+	/* Allow the legacy codec driver to match again after unbind */
+	ret = devm_add_action(&hdev->dev, hdac_hda_restore_dev_type, hdev);
+	if (ret)
+		return ret;
+
 	/* hold the ref while we probe */
 	hlink = snd_hdac_ext_bus_get_hlink_by_name(hdev->bus, dev_name(&hdev->dev));
 	if (!hlink) {
-- 
2.56.0

[PATCH v2 3/3] ASoC: hdac_hda: Fix codec runtime PM across remove and probe

[Peter Ujfalusi]

hdac_hda_codec_remove() disables runtime PM of the codec device before
releasing the HDA link and calling the codec driver's remove callback,
so that no runtime resume can race with the teardown and access the
link or the already freed codec driver data.
snd_hda_codec_cleanup_for_unbind() then disables runtime PM again.

On the next probe snd_hda_codec_register() enables runtime PM only once,
leaving it disabled after the ASoC card or the codec driver has been
unbound and bound again: the codec stays powered and runtime_status
reads 'unsupported'.

Re-enable runtime PM at the end of hdac_hda_codec_remove() to balance
its own disable. The codec remains disabled until it is registered again.

With runtime PM enabled again the codec still does not suspend: the
runtime status is RPM_SUSPENDED after the remove, while
snd_hda_codec_device_new() powers the codec to D0 on probe. The
pm_runtime_put()/snd_hda_power_down() calls see an already suspended
device and the codec, its HDA link and the display power reference
stay powered, preventing the controller from runtime suspending.
Mark the device active after the codec has been powered up.

Fixes: c3ec8ac82105 ("ASoC: hdac_hda: fix memleak on module unload")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/hdac_hda.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index cae5711e979c..fe5796cad1ff 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -435,6 +435,13 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
 		goto error_no_pm;
 	}
 
+	/* The codec is in D0, the status is RPM_SUSPENDED after a previous unbind */
+	ret = pm_runtime_set_active(&hdev->dev);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "%s: failed to set runtime PM active %d\n", __func__, ret);
+		goto error_no_pm;
+	}
+
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (loadable_patch[hda_pvt->dev_index] && *loadable_patch[hda_pvt->dev_index]) {
 		const struct firmware *fw __free(firmware) = NULL;
@@ -554,6 +561,7 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
 		return;
 	}
 
+	/* prevent runtime PM callbacks while the link and the codec are torn down */
 	pm_runtime_disable(&hdev->dev);
 	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
 
@@ -561,6 +569,9 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
 		driver->ops->remove(codec);
 
 	snd_hda_codec_cleanup_for_unbind(codec);
+
+	/* Balances the disable above, cleanup_for_unbind keeps runtime PM disabled */
+	pm_runtime_enable(&hdev->dev);
 }
 
 static const struct snd_soc_dapm_route hdac_hda_dapm_routes[] = {
-- 
2.56.0

Re: [PATCH v2 3/3] ASoC: hdac_hda: Fix codec runtime PM across remove and probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: hdac_hda: desynchronized runtime PM and link power across unbind
- [High] ASoC: hdac_hda: incomplete cleanup on hdac_hda_codec_probe() error paths
--

commit 96608fa793f58585604bebad10178e1f2248d765
Author: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: hdac_hda: Fix codec runtime PM across remove and probe

This commit balances runtime PM disable and enable calls in
hdac_hda_codec_remove() and calls pm_runtime_set_active() in
hdac_hda_codec_probe() so that the codec runtime PM status reflects its
powered state after unbind and rebind.

> diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
> index cae5711e979c3..fe5796cad1fff 100644
> --- a/sound/soc/codecs/hdac_hda.c
> +++ b/sound/soc/codecs/hdac_hda.c
> @@ -435,6 +435,13 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
>  		goto error_no_pm;
>  	}
>  
> +	/* The codec is in D0, the status is RPM_SUSPENDED after a previous unbind */
> +	ret = pm_runtime_set_active(&hdev->dev);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "%s: failed to set runtime PM active %d\n", __func__, ret);
> +		goto error_no_pm;
> +	}

[Severity: High]
If hdac_hda_codec_probe() fails after pm_runtime_set_active(&hdev->dev)
succeeds, can this leak the parent controller runtime PM child_count and
leave the codec runtime PM state inconsistent?

In sound/soc/codecs/hdac_hda.c:hdac_hda_codec_probe(), after a previous
unbind left runtime PM disabled and suspended, pm_runtime_set_active()
marks &hdev->dev active and increments parent->power.child_count. If a
subsequent step fails (such as request_firmware(), snd_hda_load_patch(),
snd_hda_codec_set_name(), snd_hdac_regmap_init(), driver->ops->probe(),
snd_hda_codec_parse_pcms(), or snd_hda_codec_build_controls()), control
jumps to the error labels at the bottom of hdac_hda_codec_probe():

sound/soc/codecs/hdac_hda.c:hdac_hda_codec_probe() {
	...
error_patch:
	if (driver->ops->remove)
		driver->ops->remove(hcodec);
error_regmap:
	snd_hdac_regmap_exit(hdev);
error_pm:
	pm_runtime_put(&hdev->dev);
error_no_pm:
	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
	return ret;
}

Because hdac_hda_codec_probe() returns an error, snd_soc_component_probe()
does not mark the component as probed and hdac_hda_codec_remove() is never
called. None of the error paths call pm_runtime_set_suspended(&hdev->dev),
undo the pm_runtime_forbid(&hdev->dev) done by snd_hda_codec_device_new(),
or release the display power enabled earlier in hdac_hda_codec_probe().

In addition, if pm_runtime_set_active() or the firmware patch loader fails
and jumps to error_no_pm before hdev->type is overwritten in
hdac_hda_codec_probe(), codec->core.type remains set to the legacy type
assigned by snd_hda_codec_device_new(). When card cleanup runs
snd_card_free() -> snd_hda_codec_dev_free() -> snd_hda_codec_unregister():

sound/hda/common/codec.c:snd_hda_codec_unregister() {
	...
	if (codec->core.type == HDA_DEV_LEGACY)
		snd_hdac_device_unregister(&codec->core);
	snd_hda_codec_display_power(codec, false);

	if (codec->core.type == HDA_DEV_LEGACY)
		put_device(hda_codec_dev(codec));
}

this unregisters the bus-managed codec device and deadlocks on
client_mutex via device_del() -> snd_soc_unregister_component_by_driver().

Should the error paths in hdac_hda_codec_probe() reset the runtime PM
status to suspended, allow runtime PM, release display power, and ensure
hdev->type is set before jumping to cleanup?

> @@ -554,6 +561,7 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
>  		return;
>  	}
>  
> +	/* prevent runtime PM callbacks while the link and the codec are torn down */
>  	pm_runtime_disable(&hdev->dev);
>  	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
>  
> @@ -561,6 +569,9 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
>  		driver->ops->remove(codec);
>  
>  	snd_hda_codec_cleanup_for_unbind(codec);
> +
> +	/* Balances the disable above, cleanup_for_unbind keeps runtime PM disabled */
> +	pm_runtime_enable(&hdev->dev);
>  }

[Severity: High]
Does this sequence in hdac_hda_codec_remove() and hdac_hda_codec_probe()
desynchronize bus->codec_powered and hlink->ref_count depending on whether
the codec is runtime active or suspended when hdac_hda_codec_remove()
runs?

There appear to be two cases that get out of sync:

1. If the codec is runtime active when hdac_hda_codec_remove() runs (for
example, when unbinding the codec driver via sysfs while autosuspend is
enabled by skl_set_hda_codec_autosuspend_delay() ->
snd_hda_set_power_save()):

drivers/base/dd.c:__device_release_driver() {
	pm_runtime_get_sync(dev);
	...
	pm_runtime_put_sync(dev);
	device_remove(dev);
	...
	device_unbind_cleanup(dev);
}

Here pm_runtime_get_sync() resumes the codec via
hda_codec_runtime_resume() -> snd_hdac_codec_link_up() ->
snd_hdac_ext_bus_link_power(codec, true), which sets the codec bit in
bus->codec_powered and increments hlink->ref_count. Because autosuspend is
enabled, pm_runtime_put_sync() only starts the autosuspend timer and
leaves &hdev->dev active when device_unbind_cleanup() invokes
hdac_hda_codec_remove().

In hdac_hda_codec_remove(), pm_runtime_disable(&hdev->dev) cancels the
pending autosuspend timer without calling hda_codec_runtime_suspend(), and
snd_hdac_ext_bus_link_put() only drops the link reference from
hdac_hda_codec_probe(). That leaves the codec powered, the link reference
from hda_codec_runtime_resume() leaked, and the codec bit set in
bus->codec_powered, which causes sound/soc/sof/intel/hda-dsp.c:
hda_dsp_runtime_idle() to return -EBUSY and block controller runtime
suspend:

sound/soc/sof/intel/hda-dsp.c:hda_dsp_runtime_idle() {
	if (hbus->codec_powered) {
		dev_dbg(sdev->dev, "some codecs still powered (%08X), not idle\n",
			(unsigned int)hbus->codec_powered);
		return -EBUSY;
	}
	...
}

2. Conversely, if the codec is already runtime suspended when
hdac_hda_codec_remove() runs, hda_codec_runtime_suspend() has already
called snd_hdac_codec_link_down() ->
snd_hdac_ext_bus_link_power(codec, false), clearing the codec bit in
bus->codec_powered and dropping hlink->ref_count.
hdac_hda_codec_remove() then calls snd_hdac_ext_bus_link_put() a second
time.

On the next hdac_hda_codec_probe(), pm_runtime_set_active(&hdev->dev)
marks the device runtime active without calling snd_hdac_codec_link_up(),
so the codec bit in bus->codec_powered remains cleared while the codec is
active. When hda_codec_runtime_suspend() later runs,
snd_hdac_ext_bus_link_power(codec, false) sees enable == oldstate (both
false) and returns early:

sound/hda/core/ext/controller.c:snd_hdac_ext_bus_link_power() {
	struct hdac_bus *bus = codec->bus;
	bool oldstate = test_bit(codec->addr, &bus->codec_powered);

	if (enable == oldstate)
		return;
	...
}

skipping hdac_ext_codec_link_down(), controller display power release, and
wake enable configuration in sof_hda_bus_link_power().

Should hdac_hda_codec_probe() and hdac_hda_codec_remove() ensure the codec
link power state and runtime PM state transitions go through the runtime
PM callbacks (such as pm_runtime_resume() in probe and
pm_runtime_put_sync_suspend() before teardown) rather than manipulating
the link refcount and runtime status directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027125448.11018-1-peter.ujfalusi@linux.intel.com?part=3

Re: [PATCH v2 0/3] ASoC: hdac_hda: Fix module unbind/bind via sysfs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Tue, Oct 27, 2026 at 02:54:45PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v1:
> - Add pm_runtime_set_active() after snd_hda_codec_device_new() in
>   hdac_hda_codec_probe() function as reported and suggested by
>   Sashiko Bot.

Please send an incremental change, I already put this into CI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] ASoC: hdac_hda: Fix module unbind/bind via sysfs

[Péter Ujfalusi]

On 27/10/2026 15:34, Mark Brown wrote:
> On Tue, Oct 27, 2026 at 02:54:45PM +0200, Peter Ujfalusi wrote:
>> Hi,
>>
>> Changes since v1:
>> - Add pm_runtime_set_active() after snd_hda_codec_device_new() in
>>   hdac_hda_codec_probe() function as reported and suggested by
>>   Sashiko Bot.
> 
> Please send an incremental change, I already put this into CI.

I don't see them in for-7.4, for-linus or for-next branches, the
followup should have a Fixes tag to be backported alongside this patch.

I can use the same commit id on the other had for this as well:
Fixes: c3ec8ac82105 ("ASoC: hdac_hda: fix memleak on module unload")

-- 
Péter

Re: [PATCH v2 0/3] ASoC: hdac_hda: Fix module unbind/bind via sysfs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Tue, Oct 27, 2026 at 04:25:00PM +0200, Péter Ujfalusi wrote:
> On 27/10/2026 15:34, Mark Brown wrote:

> > Please send an incremental change, I already put this into CI.

> I don't see them in for-7.4, for-linus or for-next branches, the
> followup should have a Fixes tag to be backported alongside this patch.

CI means I am testing them (and other stuff), I don't want to have to
redo all the testing for a rebase.

> I can use the same commit id on the other had for this as well:
> Fixes: c3ec8ac82105 ("ASoC: hdac_hda: fix memleak on module unload")

That's probably fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]