[PATCH v1 0/7] ACPI: scan: A few ad-hoc cleanups

[PATCH v1 0/7] ACPI: scan: A few ad-hoc cleanups

[Andy Shevchenko]

While looking for something else in the scan.c I noticed the room
of improvement. Hence this series. Also bus.c patches, which some
how were related to my research, but I think are independent from
the scan.c improvements.

Andy Shevchenko (7):
  ACPI: bus: Make container_of() no-op where it makes sense
  ACPI: bus: Don't use "proxy" headers
  ACPI: scan: Replace infinite for-loop with finite while-loop
  ACPI: scan: Use list_first_entry_or_null() in acpi_device_hid()
  ACPI: scan: Move misleading comment to acpi_dma_configure_id()
  ACPI: scan: Use standard error checking pattern
  ACPI: scan: Introduce typedef:s for struct acpi_hotplug_context
    members

 drivers/acpi/dock.c     | 48 +++++++++++++++--------------------------
 drivers/acpi/scan.c     | 42 +++++++++++++++++-------------------
 include/acpi/acpi_bus.h | 28 +++++++++++++++---------
 3 files changed, 55 insertions(+), 63 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/7] ACPI: bus: Make container_of() no-op where it makes sense

[Andy Shevchenko]

Move list head node to be the first member in a few data structures
in order to make container_of() no-op at compile time. On x86_64 with
a custom (default + a few dozens of drivers enabled) configuration:

  add/remove: 0/0 grow/shrink: 5/12 up/down: 21/-124 (-103)
  ...
  Total: Before=39924675, After=39924572, chg -0.00%

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/acpi/acpi_bus.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 5de954e2b18a..cdee38ef9ba5 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -124,8 +124,8 @@ static inline struct acpi_hotplug_profile *to_acpi_hotplug_profile(
 }
 
 struct acpi_scan_handler {
-	const struct acpi_device_id *ids;
 	struct list_head list_node;
+	const struct acpi_device_id *ids;
 	bool (*match)(const char *idstr, const struct acpi_device_id **matchid);
 	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
 	void (*detach)(struct acpi_device *dev);
@@ -269,6 +269,7 @@ struct acpi_device_power_flags {
 };
 
 struct acpi_device_power_state {
+	struct list_head resources;	/* Power resources referenced */
 	struct {
 		u8 valid:1;
 		u8 explicit_set:1;	/* _PSx present? */
@@ -276,7 +277,6 @@ struct acpi_device_power_state {
 	} flags;
 	int power;		/* % Power (compared to D0) */
 	int latency;		/* Dx->D0 time (microseconds) */
-	struct list_head resources;	/* Power resources referenced */
 };
 
 struct acpi_device_power {
@@ -342,16 +342,16 @@ struct acpi_device_wakeup {
 };
 
 struct acpi_device_physical_node {
-	unsigned int node_id;
 	struct list_head node;
+	unsigned int node_id;
 	struct device *dev;
 	bool put_online:1;
 };
 
 struct acpi_device_properties {
+	struct list_head list;
 	const guid_t *guid;
 	union acpi_object *properties;
-	struct list_head list;
 	void **bufs;
 };
 
@@ -488,12 +488,12 @@ struct acpi_device {
 
 /* Non-device subnode */
 struct acpi_data_node {
+	struct list_head sibling;
 	const char *name;
 	acpi_handle handle;
 	struct fwnode_handle fwnode;
 	struct fwnode_handle *parent;
 	struct acpi_device_data data;
-	struct list_head sibling;
 	struct kobject kobj;
 	struct completion kobj_done;
 };
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 2/7] ACPI: bus: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/acpi/acpi_bus.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index cdee38ef9ba5..acb62d1d3306 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -9,8 +9,13 @@
 #ifndef __ACPI_BUS_H__
 #define __ACPI_BUS_H__
 
+#include <linux/completion.h>
+#include <linux/container_of.h>
 #include <linux/device.h>
+#include <linux/kobject.h>
+#include <linux/mutex.h>
 #include <linux/property.h>
+#include <linux/types.h>
 
 struct acpi_handle_list {
 	u32 count;
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 3/7] ACPI: scan: Replace infinite for-loop with finite while-loop

[Andy Shevchenko]

The infinite loops is harder to understand (as one has to go
over the body in order to find main exit conditional) and it's
more verbose than usual approach with a while-loop.

Note, we may not use list_for_each_entry_safe() as there is locking
involved and the saved pointer may become invalid behind our back.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7c157bf92695..5e4118970285 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -530,15 +530,10 @@ static DEFINE_MUTEX(acpi_device_del_lock);
 
 static void acpi_device_del_work_fn(struct work_struct *work_not_used)
 {
-	for (;;) {
-		struct acpi_device *adev;
+	struct acpi_device *adev;
 
-		mutex_lock(&acpi_device_del_lock);
-
-		if (list_empty(&acpi_device_del_list)) {
-			mutex_unlock(&acpi_device_del_lock);
-			break;
-		}
+	mutex_lock(&acpi_device_del_lock);
+	while (!list_empty(&acpi_device_del_list)) {
 		adev = list_first_entry(&acpi_device_del_list,
 					struct acpi_device, del_list);
 		list_del(&adev->del_list);
@@ -555,7 +550,10 @@ static void acpi_device_del_work_fn(struct work_struct *work_not_used)
 		 */
 		acpi_power_transition(adev, ACPI_STATE_D3_COLD);
 		acpi_dev_put(adev);
+
+		mutex_lock(&acpi_device_del_lock);
 	}
+	mutex_unlock(&acpi_device_del_lock);
 }
 
 /**
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 3/7] ACPI: scan: Replace infinite for-loop with finite while-loop

[Rafael J. Wysocki]

On Mon, Mar 25, 2024 at 1:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The infinite loops is harder to understand (as one has to go
> over the body in order to find main exit conditional) and it's
> more verbose than usual approach with a while-loop.
>
> Note, we may not use list_for_each_entry_safe() as there is locking
> involved and the saved pointer may become invalid behind our back.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/scan.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7c157bf92695..5e4118970285 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -530,15 +530,10 @@ static DEFINE_MUTEX(acpi_device_del_lock);
>
>  static void acpi_device_del_work_fn(struct work_struct *work_not_used)
>  {
> -       for (;;) {
> -               struct acpi_device *adev;
> +       struct acpi_device *adev;
>
> -               mutex_lock(&acpi_device_del_lock);
> -
> -               if (list_empty(&acpi_device_del_list)) {
> -                       mutex_unlock(&acpi_device_del_lock);
> -                       break;
> -               }
> +       mutex_lock(&acpi_device_del_lock);
> +       while (!list_empty(&acpi_device_del_list)) {
>                 adev = list_first_entry(&acpi_device_del_list,
>                                         struct acpi_device, del_list);
>                 list_del(&adev->del_list);
> @@ -555,7 +550,10 @@ static void acpi_device_del_work_fn(struct work_struct *work_not_used)
>                  */
>                 acpi_power_transition(adev, ACPI_STATE_D3_COLD);
>                 acpi_dev_put(adev);
> +
> +               mutex_lock(&acpi_device_del_lock);
>         }
> +       mutex_unlock(&acpi_device_del_lock);
>  }
>
>  /**
> --

I don't quite agree with this one, sorry.

The rest of the series has been applied as 6.10 material.

Thanks!

Re: [PATCH v1 3/7] ACPI: scan: Replace infinite for-loop with finite while-loop

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 09:22:29PM +0200, Rafael J. Wysocki wrote:
> On Mon, Mar 25, 2024 at 1:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> I don't quite agree with this one, sorry.

No problem.

> The rest of the series has been applied as 6.10 material.

Thank you!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/7] ACPI: scan: Use list_first_entry_or_null() in acpi_device_hid()

[Andy Shevchenko]

To replace list_empty() + list_first_entry() pair to simplify code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5e4118970285..f5581d3701f1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1296,10 +1296,10 @@ const char *acpi_device_hid(struct acpi_device *device)
 {
 	struct acpi_hardware_id *hid;
 
-	if (list_empty(&device->pnp.ids))
+	hid = list_first_entry_or_null(&device->pnp.ids, struct acpi_hardware_id, list);
+	if (!hid)
 		return dummy_hid;
 
-	hid = list_first_entry(&device->pnp.ids, struct acpi_hardware_id, list);
 	return hid->id;
 }
 EXPORT_SYMBOL(acpi_device_hid);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 5/7] ACPI: scan: Move misleading comment to acpi_dma_configure_id()

[Andy Shevchenko]

The acpi_iommu_configure_id() implementation has a misleading comment
since after it the flow does something different to what it states.
Move the commit to the caller and with that unshadow the error code
inside acpi_iommu_configure_id().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f5581d3701f1..b2785a036a68 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1623,12 +1623,11 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 	if (!err && dev->bus)
 		err = iommu_probe_device(dev);
 
-	/* Ignore all other errors apart from EPROBE_DEFER */
-	if (err == -EPROBE_DEFER) {
+	if (err == -EPROBE_DEFER)
 		return err;
-	} else if (err) {
+	if (err) {
 		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
-		return -ENODEV;
+		return err;
 	}
 	if (!acpi_iommu_fwspec_ops(dev))
 		return -ENODEV;
@@ -1669,13 +1668,14 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 
 	acpi_arch_dma_setup(dev);
 
+	/* Ignore all other errors apart from EPROBE_DEFER */
 	ret = acpi_iommu_configure_id(dev, input_id);
 	if (ret == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
 	/*
 	 * Historically this routine doesn't fail driver probing due to errors
-	 * in acpi_iommu_configure_id()
+	 * in acpi_iommu_configure_id().
 	 */
 
 	arch_setup_dma_ops(dev, 0, U64_MAX, attr == DEV_DMA_COHERENT);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 6/7] ACPI: scan: Use standard error checking pattern

[Andy Shevchenko]

Check for an error and return it as it's the usual way to handle this.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/scan.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index b2785a036a68..45f7ec3b6548 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1579,12 +1579,13 @@ int acpi_iommu_fwspec_init(struct device *dev, u32 id,
 			   struct fwnode_handle *fwnode,
 			   const struct iommu_ops *ops)
 {
-	int ret = iommu_fwspec_init(dev, fwnode, ops);
+	int ret;
 
-	if (!ret)
-		ret = iommu_fwspec_add_ids(dev, &id, 1);
+	ret = iommu_fwspec_init(dev, fwnode, ops);
+	if (ret)
+		return ret;
 
-	return ret;
+	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
 static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 7/7] ACPI: scan: Introduce typedef:s for struct acpi_hotplug_context members

[Andy Shevchenko]

Follow the struct acpi_device_ops approach and introduce typedef:s
for the members. It makes code less verbose and more particular on
what parameters we take or types we use.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/dock.c     | 48 +++++++++++++++--------------------------
 drivers/acpi/scan.c     |  5 ++---
 include/acpi/acpi_bus.h | 13 ++++++-----
 3 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index a7c00ef78086..34affbda295e 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -88,43 +88,29 @@ static void dock_hotplug_event(struct dock_dependent_device *dd, u32 event,
 			       enum dock_callback_type cb_type)
 {
 	struct acpi_device *adev = dd->adev;
+	acpi_hp_fixup fixup = NULL;
+	acpi_hp_uevent uevent = NULL;
+	acpi_hp_notify notify = NULL;
 
 	acpi_lock_hp_context();
 
-	if (!adev->hp)
-		goto out;
-
-	if (cb_type == DOCK_CALL_FIXUP) {
-		void (*fixup)(struct acpi_device *);
-
-		fixup = adev->hp->fixup;
-		if (fixup) {
-			acpi_unlock_hp_context();
-			fixup(adev);
-			return;
-		}
-	} else if (cb_type == DOCK_CALL_UEVENT) {
-		void (*uevent)(struct acpi_device *, u32);
-
-		uevent = adev->hp->uevent;
-		if (uevent) {
-			acpi_unlock_hp_context();
-			uevent(adev, event);
-			return;
-		}
-	} else {
-		int (*notify)(struct acpi_device *, u32);
-
-		notify = adev->hp->notify;
-		if (notify) {
-			acpi_unlock_hp_context();
-			notify(adev, event);
-			return;
-		}
+	if (adev->hp) {
+		if (cb_type == DOCK_CALL_FIXUP)
+			fixup = adev->hp->fixup;
+		else if (cb_type == DOCK_CALL_UEVENT)
+			uevent = adev->hp->uevent;
+		else
+			notify = adev->hp->notify;
 	}
 
- out:
 	acpi_unlock_hp_context();
+
+	if (fixup)
+		fixup(adev);
+	else if (uevent)
+		uevent(adev, event);
+	else if (notify)
+		notify(adev, event);
 }
 
 static struct dock_station *find_dock_station(acpi_handle handle)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 45f7ec3b6548..534cb15351f5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -73,8 +73,7 @@ void acpi_unlock_hp_context(void)
 
 void acpi_initialize_hp_context(struct acpi_device *adev,
 				struct acpi_hotplug_context *hp,
-				int (*notify)(struct acpi_device *, u32),
-				void (*uevent)(struct acpi_device *, u32))
+				acpi_hp_notify notify, acpi_hp_uevent uevent)
 {
 	acpi_lock_hp_context();
 	hp->notify = notify;
@@ -428,7 +427,7 @@ void acpi_device_hotplug(struct acpi_device *adev, u32 src)
 	} else if (adev->flags.hotplug_notify) {
 		error = acpi_generic_hotplug_event(adev, src);
 	} else {
-		int (*notify)(struct acpi_device *, u32);
+		acpi_hp_notify notify;
 
 		acpi_lock_hp_context();
 		notify = adev->hp ? adev->hp->notify : NULL;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index acb62d1d3306..6aa0d8a1de79 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -144,11 +144,15 @@ struct acpi_scan_handler {
  * --------------------
  */
 
+typedef int (*acpi_hp_notify) (struct acpi_device *, u32);
+typedef void (*acpi_hp_uevent) (struct acpi_device *, u32);
+typedef void (*acpi_hp_fixup) (struct acpi_device *);
+
 struct acpi_hotplug_context {
 	struct acpi_device *self;
-	int (*notify)(struct acpi_device *, u32);
-	void (*uevent)(struct acpi_device *, u32);
-	void (*fixup)(struct acpi_device *);
+	acpi_hp_notify notify;
+	acpi_hp_uevent uevent;
+	acpi_hp_fixup fixup;
 };
 
 /*
@@ -583,8 +587,7 @@ static inline void acpi_set_hp_context(struct acpi_device *adev,
 
 void acpi_initialize_hp_context(struct acpi_device *adev,
 				struct acpi_hotplug_context *hp,
-				int (*notify)(struct acpi_device *, u32),
-				void (*uevent)(struct acpi_device *, u32));
+				acpi_hp_notify notify, acpi_hp_uevent uevent);
 
 /* acpi_device.dev.bus == &acpi_bus_type */
 extern const struct bus_type acpi_bus_type;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 0/7] ACPI: scan: A few ad-hoc cleanups

[Kuppuswamy Sathyanarayanan]

On 3/25/24 5:32 AM, Andy Shevchenko wrote:
> While looking for something else in the scan.c I noticed the room
> of improvement. Hence this series. Also bus.c patches, which some
> how were related to my research, but I think are independent from
> the scan.c improvements.

Changes looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Andy Shevchenko (7):
>   ACPI: bus: Make container_of() no-op where it makes sense
>   ACPI: bus: Don't use "proxy" headers
>   ACPI: scan: Replace infinite for-loop with finite while-loop
>   ACPI: scan: Use list_first_entry_or_null() in acpi_device_hid()
>   ACPI: scan: Move misleading comment to acpi_dma_configure_id()
>   ACPI: scan: Use standard error checking pattern
>   ACPI: scan: Introduce typedef:s for struct acpi_hotplug_context
>     members
>
>  drivers/acpi/dock.c     | 48 +++++++++++++++--------------------------
>  drivers/acpi/scan.c     | 42 +++++++++++++++++-------------------
>  include/acpi/acpi_bus.h | 28 +++++++++++++++---------
>  3 files changed, 55 insertions(+), 63 deletions(-)
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer