diff mbox series

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

Message ID 20240325123444.3031851-4-andriy.shevchenko@linux.intel.com (mailing list archive)
State Rejected, archived
Headers show
Series ACPI: scan: A few ad-hoc cleanups | expand

Commit Message

Andy Shevchenko March 25, 2024, 12:32 p.m. UTC
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(-)

Comments

Rafael J. Wysocki April 4, 2024, 7:22 p.m. UTC | #1
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!
Andy Shevchenko April 4, 2024, 7:26 p.m. UTC | #2
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!
diff mbox series

Patch

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);
 }
 
 /**