Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 24 Jul 2023 14:16:22 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI |
| |
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.
-- With Best Regards, Andy Shevchenko
| ![\](/images/icornerr.gif) |