ALSA: hda: Fix Cirrus ACPI device reference leaks

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(-)

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.

---
 sound/hda/codecs/side-codecs/cs35l56_hda.c | 1 +
 1 file changed, 1 insertion(+)

index 1ace4beef50857b..16a5a40899b2873 100644
@@ -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 */

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.

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.

---
 sound/hda/codecs/side-codecs/cs35l41_hda.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index b64890006bb7019..acfccc848f82d82 100644
@@ -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))

Hi Richard,

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

which

put

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

thanks,

Takashi