On Mon, Jul 24, 2023 at 11:06:02AM +0000, Lu, Brent wrote:
+/* helper function to get the number of specific codec */
...and leak a lot of reference counts...
+static int get_num_codecs(const char *hid) {
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);Humm, I am a bit worried about reference counts.
See https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9 16, it's not clear to me where the get() is done.
Adding Andy to make sure this is done right.
Thank you for Cc'ing.
Yes, the above code is problematic. One has to use the respective for_each macro (defined nearby the used API).
if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to release the adev from previous call. And the last call returns NULL. It seems to me the reference count should be fine. Is my understanding correct?
Ah, right. sorry for the confusion. That's why we have a macro to not think about these details :-)
I saw the macro for_each_acpi_dev_match and re-write the function as follow. Thanks for suggesting using the macro.
/* helper function to get the number of specific codec */ static int get_num_codecs(const char *hid) { struct acpi_device *adev;
int dev_num = 0;
size_t here or at least unsigned int is more correct.
for_each_acpi_dev_match(adev, hid, NULL, -1) dev_num++;
return dev_num; }
Otherwise, yes, that's what I have in mind.
Will test it in next few days.