All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks
@ 2026-04-28  7:44 Shuhao Fu
  2026-04-28  8:01 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Shuhao Fu @ 2026-04-28  7:44 UTC (permalink / raw)
  To: David Rhodes, Richard Fitzgerald
  Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, patches, linux-kernel

This series fixes two missing acpi_dev_put() cases in Cirrus HDA side-codec
ACPI lookup paths.

Patch 1 drops the lookup reference after synthesizing the companion in
cs35l56_hda_read_acpi().

Patch 2 drops the lookup reference on the early !physdev error path in
cs35l41_hda_read_acpi().

Shuhao Fu (2):
  ALSA: hda: cs35l56: Put ACPI device after setting companion
  ALSA: hda: cs35l41: Fix ACPI device leak on missing physical node

 sound/hda/codecs/side-codecs/cs35l41_hda.c | 4 +++-
 sound/hda/codecs/side-codecs/cs35l56_hda.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion
  2026-04-28  7:44 [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Shuhao Fu
@ 2026-04-28  8:01 ` Shuhao Fu
  2026-04-28  9:05   ` Richard Fitzgerald
  2026-04-28  8:12 ` [PATCH 2/2] ALSA: hda: cs35l41: Put ACPI device on missing physical node Shuhao Fu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Shuhao Fu @ 2026-04-28  8:01 UTC (permalink / raw)
  To: David Rhodes, Richard Fitzgerald
  Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, patches, linux-kernel

acpi_dev_get_first_match_dev() returns a refcounted ACPI device and
callers are expected to balance it with acpi_dev_put().

When no companion is already attached, cs35l56_hda_read_acpi() looks
up an ACPI device and sets it with ACPI_COMPANION_SET(), but leaves
the lookup reference held.

ACPI_COMPANION_SET() does not take ownership of that reference, so
drop it with acpi_dev_put() after attaching the companion.

Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier")
Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
---
 sound/hda/codecs/side-codecs/cs35l56_hda.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
index 1ace4beef50857b..16a5a40899b2873 100644
--- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
+++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
@@ -1032,6 +1032,7 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
 			return -ENODEV;
 		}
 		ACPI_COMPANION_SET(cs35l56->base.dev, adev);
+		acpi_dev_put(adev);
 	}
 
 	/* Initialize things that could be overwritten by a fixup */

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] ALSA: hda: cs35l41: Put ACPI device on missing physical node
  2026-04-28  7:44 [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Shuhao Fu
  2026-04-28  8:01 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
@ 2026-04-28  8:12 ` Shuhao Fu
  2026-04-28 10:40 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
  2026-05-07 14:32 ` [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Takashi Iwai
  3 siblings, 0 replies; 7+ messages in thread
From: Shuhao Fu @ 2026-04-28  8:12 UTC (permalink / raw)
  To: David Rhodes, Richard Fitzgerald
  Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, patches, linux-kernel

acpi_dev_get_first_match_dev() returns a refcounted ACPI device and
callers must balance it with acpi_dev_put().

cs35l41_hda_read_acpi() stores the returned ACPI device in
cs35l41->dacpi. That reference is normally released by the later
probe cleanup or the remove path, but the NULL-check on
physdev exits before either of those paths can run.

Drop the lookup reference before returning -ENODEV.

Fixes: c34b04cc6178 ("ALSA: hda: cs35l41: Fix NULL pointer dereference in cs35l41_hda_read_acpi()")
Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
---
 sound/hda/codecs/side-codecs/cs35l41_hda.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.c b/sound/hda/codecs/side-codecs/cs35l41_hda.c
index b64890006bb7019..acfccc848f82d82 100644
--- a/sound/hda/codecs/side-codecs/cs35l41_hda.c
+++ b/sound/hda/codecs/side-codecs/cs35l41_hda.c
@@ -1896,8 +1896,10 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i
 
 	cs35l41->dacpi = adev;
 	physdev = get_device(acpi_get_first_physical_node(adev));
-	if (!physdev)
+	if (!physdev) {
+		acpi_dev_put(adev);
 		return -ENODEV;
+	}
 
 	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
 	if (IS_ERR(sub))

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion
  2026-04-28  8:01 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
@ 2026-04-28  9:05   ` Richard Fitzgerald
  0 siblings, 0 replies; 7+ messages in thread
From: Richard Fitzgerald @ 2026-04-28  9:05 UTC (permalink / raw)
  To: Shuhao Fu, David Rhodes
  Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, patches, linux-kernel

On 28/04/2026 9:01 am, Shuhao Fu wrote:
> acpi_dev_get_first_match_dev() returns a refcounted ACPI device and
> callers are expected to balance it with acpi_dev_put().
> 
> When no companion is already attached, cs35l56_hda_read_acpi() looks
> up an ACPI device and sets it with ACPI_COMPANION_SET(), but leaves
> the lookup reference held.
> 
> ACPI_COMPANION_SET() does not take ownership of that reference, so
> drop it with acpi_dev_put() after attaching the companion.

Are you sure about this?
I remember when I wrote this code I checked the driver core and saw that
if there is a companion it puts it when the driver is removed.
That is why I didn't put the reference here, it would have caused a
double put.

> 
> Fixes: 73cfbfa9caea ("ALSA: hda/cs35l56: Add driver for Cirrus Logic CS35L56 amplifier")
> Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> ---
>   sound/hda/codecs/side-codecs/cs35l56_hda.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> index 1ace4beef50857b..16a5a40899b2873 100644
> --- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
> +++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> @@ -1032,6 +1032,7 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
>   			return -ENODEV;
>   		}
>   		ACPI_COMPANION_SET(cs35l56->base.dev, adev);
> +		acpi_dev_put(adev);
>   	}
>   
>   	/* Initialize things that could be overwritten by a fixup */


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion
  2026-04-28  7:44 [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Shuhao Fu
  2026-04-28  8:01 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
  2026-04-28  8:12 ` [PATCH 2/2] ALSA: hda: cs35l41: Put ACPI device on missing physical node Shuhao Fu
@ 2026-04-28 10:40 ` Shuhao Fu
  2026-05-06 14:37   ` Simon Trimmer
  2026-05-07 14:32 ` [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Takashi Iwai
  3 siblings, 1 reply; 7+ messages in thread
From: Shuhao Fu @ 2026-04-28 10:40 UTC (permalink / raw)
  To: David Rhodes, Richard Fitzgerald
  Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, patches, linux-kernel

Hi Richard,

> Are you sure about this?
> I remember when I wrote this code I checked the driver core and saw that
> if there is a companion it puts it when the driver is removed.
> That is why I didn't put the reference here, it would have caused a
> double put.

I may well be missing something here. But from my reading of the current
code, it does not seem to cause a double put.

The place where I do seem to find ACPI companion cleanup is when the device 
object itself is deleted/unregistered:

  `device_del()`
    -> `device_platform_notify_remove()`
    -> `acpi_device_notify_remove()`
    -> `acpi_unbind_one()`

What makes me think this is not the matching put for
`acpi_dev_get_first_match_dev()` is that `acpi_unbind_one()` only calls
`acpi_dev_put()` after it finds a matching entry for the device in
`acpi_dev->physical_node_list`.

As far as I can tell, that list entry is created by `acpi_bind_one()`, which
also takes its own extra reference with `acpi_dev_get(acpi_dev)`. So the put
in `acpi_unbind_one()` looks to me like it is paired with that
`acpi_bind_one()` reference, rather than with the earlier
`acpi_dev_get_first_match_dev()` lookup.

If that reading is right, then I think the ownership looks like this:

- `ACPI_COMPANION_SET()` only attaches the companion pointer/fwnode
- the lookup reference from `acpi_dev_get_first_match_dev()` is still with
  the caller
- `acpi_dev_put(adev)` after `ACPI_COMPANION_SET()` balances only that
  lookup reference
- the later `acpi_unbind_one()` path would not be putting the same
  reference again, because that put is for the separate ref taken by
  `acpi_bind_one()`

Part of why I leaned that way is that I found a couple of in-tree examples
that seem to follow the same pattern:

- `drivers/platform/x86/x86-android-tablets/core.c`
  does `acpi_dev_get_first_match_dev()`, `ACPI_COMPANION_SET()`, then
  `acpi_dev_put()`

- `drivers/acpi/arm64/mpam.c`
  does `acpi_dev_get_first_match_dev()`, `ACPI_COMPANION_SET()`, then
  `acpi_dev_put()`

So from my own understanding, those examples also seem to treat
`ACPI_COMPANION_SET()` as not consuming the reference returned by
`acpi_dev_get_first_match_dev()`.

But this is only my reading of the current ownership flow, so if I am
overlooking some rule around manually assigned companions I am happy to
re-check.

Best regards,
Shuhao

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion
  2026-04-28 10:40 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
@ 2026-05-06 14:37   ` Simon Trimmer
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Trimmer @ 2026-05-06 14:37 UTC (permalink / raw)
  To: 'Shuhao Fu', 'David Rhodes',
	'Richard Fitzgerald'
  Cc: 'Jaroslav Kysela', 'Takashi Iwai', linux-sound,
	patches, linux-kernel

On 28/04/2026 11:41 am, Shuhao Fu wrote:
> Hi Richard,
> 
> > Are you sure about this?
> > I remember when I wrote this code I checked the driver core and saw that
> > if there is a companion it puts it when the driver is removed.
> > That is why I didn't put the reference here, it would have caused a
> > double put.
> 
> I may well be missing something here. But from my reading of the current
> code, it does not seem to cause a double put.
> 
> The place where I do seem to find ACPI companion cleanup is when the
> device
> object itself is deleted/unregistered:
> 
>   `device_del()`
>     -> `device_platform_notify_remove()`
>     -> `acpi_device_notify_remove()`
>     -> `acpi_unbind_one()`
> 
> What makes me think this is not the matching put for
> `acpi_dev_get_first_match_dev()` is that `acpi_unbind_one()` only calls
> `acpi_dev_put()` after it finds a matching entry for the device in
> `acpi_dev->physical_node_list`.
> 
> As far as I can tell, that list entry is created by `acpi_bind_one()`,
which
> also takes its own extra reference with `acpi_dev_get(acpi_dev)`. So the
put
> in `acpi_unbind_one()` looks to me like it is paired with that
> `acpi_bind_one()` reference, rather than with the earlier
> `acpi_dev_get_first_match_dev()` lookup.
> 
> If that reading is right, then I think the ownership looks like this:
> 
> - `ACPI_COMPANION_SET()` only attaches the companion pointer/fwnode
> - the lookup reference from `acpi_dev_get_first_match_dev()` is still with
>   the caller
> - `acpi_dev_put(adev)` after `ACPI_COMPANION_SET()` balances only that
>   lookup reference
> - the later `acpi_unbind_one()` path would not be putting the same
>   reference again, because that put is for the separate ref taken by
>   `acpi_bind_one()`
> 
> Part of why I leaned that way is that I found a couple of in-tree examples
> that seem to follow the same pattern:
> 
> - `drivers/platform/x86/x86-android-tablets/core.c`
>   does `acpi_dev_get_first_match_dev()`, `ACPI_COMPANION_SET()`, then
>   `acpi_dev_put()`
> 
> - `drivers/acpi/arm64/mpam.c`
>   does `acpi_dev_get_first_match_dev()`, `ACPI_COMPANION_SET()`, then
>   `acpi_dev_put()`
> 
> So from my own understanding, those examples also seem to treat
> `ACPI_COMPANION_SET()` as not consuming the reference returned by
> `acpi_dev_get_first_match_dev()`.
> 
> But this is only my reading of the current ownership flow, so if I am
> overlooking some rule around manually assigned companions I am happy to
> re-check.
> 
> Best regards,
> Shuhao

Tested-by: Simon Trimmer <simont@opensource.cirrus.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks
  2026-04-28  7:44 [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Shuhao Fu
                   ` (2 preceding siblings ...)
  2026-04-28 10:40 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
@ 2026-05-07 14:32 ` Takashi Iwai
  3 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2026-05-07 14:32 UTC (permalink / raw)
  To: Shuhao Fu
  Cc: David Rhodes, Richard Fitzgerald, Jaroslav Kysela, Takashi Iwai,
	linux-sound, patches, linux-kernel

On Tue, 28 Apr 2026 09:44:15 +0200,
Shuhao Fu wrote:
> 
> This series fixes two missing acpi_dev_put() cases in Cirrus HDA side-codec
> ACPI lookup paths.
> 
> Patch 1 drops the lookup reference after synthesizing the companion in
> cs35l56_hda_read_acpi().
> 
> Patch 2 drops the lookup reference on the early !physdev error path in
> cs35l41_hda_read_acpi().
> 
> Shuhao Fu (2):
>   ALSA: hda: cs35l56: Put ACPI device after setting companion
>   ALSA: hda: cs35l41: Fix ACPI device leak on missing physical node

Assuming that the logic is correct, I applied both now.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-05-07 14:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-28  7:44 [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Shuhao Fu
2026-04-28  8:01 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
2026-04-28  9:05   ` Richard Fitzgerald
2026-04-28  8:12 ` [PATCH 2/2] ALSA: hda: cs35l41: Put ACPI device on missing physical node Shuhao Fu
2026-04-28 10:40 ` [PATCH 1/2] ALSA: hda: cs35l56: Put ACPI device after setting companion Shuhao Fu
2026-05-06 14:37   ` Simon Trimmer
2026-05-07 14:32 ` [PATCH 0/2] ALSA: hda: Fix Cirrus ACPI device reference leaks Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.