On Sat, Apr 10, 2021 at 3:29 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> The macro requires to call acpi_dev_put() on each iteration.
> Due to this it doesn't tolerate sudden disappearence of the devices.
>
> Document all these nuances to prevent users blindly call it without
> understanding the possible issues.
>
> While at it, add the note to the acpi_dev_get_next_match_dev() and
> advertise acpi_dev_put() instead of put_device() in the whole family
> of the helper functions.
>
> Fixes: bf263f64e804 ("media: ACPI / bus: Add acpi_dev_get_next_match_dev() and helper macro")
> Cc: Daniel Scally <djrscally@xxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> ---
> drivers/acpi/utils.c | 12 ++++++++----
> include/acpi/acpi_bus.h | 13 +++++++++++++
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index f1aff4dab476..3f3171e9aef5 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -811,7 +811,7 @@ static int acpi_dev_match_cb(struct device *dev, const void *data)
> * Note that if the device is pluggable, it may since have disappeared.
> *
> * Note that unlike acpi_dev_found() this function checks the status
> - * of the device. So for devices which are present in the dsdt, but
> + * of the device. So for devices which are present in the DSDT, but
> * which are disabled (their _STA callback returns 0) this function
> * will return false.
> *
> @@ -838,7 +838,7 @@ EXPORT_SYMBOL(acpi_dev_present);
>
> /**
> * acpi_dev_get_next_match_dev - Return the next match of ACPI device
> - * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv
> + * @adev: Pointer to the previous ACPI device matching this @hid, @uid and @hrv
> * @hid: Hardware ID of the device.
> * @uid: Unique ID of the device, pass NULL to not check _UID
> * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
The two cleanups above are not related to the subject of the patch.
Please separate them.
> @@ -846,7 +846,11 @@ EXPORT_SYMBOL(acpi_dev_present);
> * Return the next match of ACPI device if another matching device was present
> * at the moment of invocation, or NULL otherwise.
> *
> - * The caller is responsible to call put_device() on the returned device.
> + * Note, the function does not tolerate the sudden disappearance of @adev, e.g.
> + * in the case of hotplug event.
"of a hotplug event"
> That said, caller should ensure that this will
"the caller"
> + * never happen.
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.
"responsible for"
And I would say "responsible for invoking".
> *
> * See additional information in acpi_dev_present() as well.
> */
> @@ -875,7 +879,7 @@ EXPORT_SYMBOL(acpi_dev_get_next_match_dev);
> * Return the first match of ACPI device if a matching device was present
> * at the moment of invocation, or NULL otherwise.
> *
> - * The caller is responsible to call put_device() on the returned device.
> + * The caller is responsible to call acpi_dev_put() on the returned device.
> *
> * See additional information in acpi_dev_present() as well.
> */
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index f28b097c658f..834b7a1f7405 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -689,6 +689,19 @@ acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const cha
> struct acpi_device *
> acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
>
> +/**
> + * for_each_acpi_dev_match - iterate over ACPI devices that matching the criteria
> + * @adev: pointer to the matching ACPI device, NULL at the end of the loop
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * The caller is responsible to call acpi_dev_put() on the returned device.
As per the above.
> + *
> + * Due to above requirement there is a window that may invalidate @adev and
> + * next iteration will use a dangling pointer, e.g. in the case of hotplug
> + * event. That said, caller should ensure that this will never happen.
> + */
> #define for_each_acpi_dev_match(adev, hid, uid, hrv) \
> for (adev = acpi_dev_get_first_match_dev(hid, uid, hrv); \
> adev; \
> --
> 2.31.1
>
