Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro
@ 2021-04-10 13:13 Andy Shevchenko
  2021-04-12 17:27 ` Rafael J. Wysocki
  0 siblings, 1 reply; 3+ messages in thread
From: Andy Shevchenko @ 2021-04-10 13:13 UTC (permalink / raw)
  To: Rafael J. Wysocki, linux-acpi, linux-kernel, devel
  Cc: Rafael J. Wysocki, Len Brown, Robert Moore, Erik Kaneda,
	Andy Shevchenko, Daniel Scally

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@gmail.com>
Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 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
@@ -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. That said, caller should ensure that this will
+ * never happen.
+ *
+ * The caller is responsible to call acpi_dev_put() on the returned device.
  *
  * 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.
+ *
+ * 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


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

* Re: [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro
  2021-04-10 13:13 [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro Andy Shevchenko
@ 2021-04-12 17:27 ` Rafael J. Wysocki
  2021-04-12 17:50   ` Andy Shevchenko
  0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2021-04-12 17:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Rafael J. Wysocki, Len Brown, Robert Moore, Erik Kaneda,
	Daniel Scally

On Sat, Apr 10, 2021 at 3:29 PM Andy Shevchenko
<andy.shevchenko@gmail.com> 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@gmail.com>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  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
>

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

* Re: [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro
  2021-04-12 17:27 ` Rafael J. Wysocki
@ 2021-04-12 17:50   ` Andy Shevchenko
  0 siblings, 0 replies; 3+ messages in thread
From: Andy Shevchenko @ 2021-04-12 17:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List,
	open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
	Rafael J. Wysocki, Len Brown, Robert Moore, Erik Kaneda,
	Daniel Scally

On Mon, Apr 12, 2021 at 8:27 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Apr 10, 2021 at 3:29 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > The macro requires to call acpi_dev_put() on each iteration.
> > Due to this it doesn't tolerate sudden disappearence of the devices.

And should be "disappearance" :-)

> >
> > 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.

Thanks for reviewing this, I'll address all in v2.

-- 
With Best Regards,
Andy Shevchenko

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-10 13:13 [PATCH v1 1/1] ACPI: utils: Document for_each_acpi_dev_match() macro Andy Shevchenko
2021-04-12 17:27 ` Rafael J. Wysocki
2021-04-12 17:50   ` Andy Shevchenko

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git