[PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling

[PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling

[Rafael J. Wysocki]

Hi,

The following patches address a few issues and do a few cleanups related to
the handling of the _DEP dependencies list.

Please refer to the patch changelogs for details.

Thanks!

[PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make acpi_dev_get_first_consumer_dev_cb() a bit more straightforward
and rewrite the comment in it.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2107,13 +2107,12 @@ static int acpi_dev_get_first_consumer_d
 	struct acpi_device *adev;
 
 	adev = acpi_bus_get_acpi_device(dep->consumer);
-	if (!adev)
-		/* If we don't find an adev then we want to continue parsing */
-		return 0;
-
-	*(struct acpi_device **)data = adev;
-
-	return 1;
+	if (adev) {
+		*(struct acpi_device **)data = adev;
+		return 1;
+	}
+	/* Continue parsing if the device object is not present. */
+	return 0;
 }
 
 static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)

[PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because acpi_walk_dep_device_list() is only called by the code in the
file in which it is defined, make it static, drop the export of it
and drop its header from acpi.h.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c  |    7 +++----
 include/linux/acpi.h |    3 ---
 2 files changed, 3 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2145,9 +2145,9 @@ static int acpi_scan_clear_dep(struct ac
  * negative value is returned by the callback then the loop is broken and that
  * value is returned as the final error.
  */
-int acpi_walk_dep_device_list(acpi_handle handle,
-			      int (*callback)(struct acpi_dep_data *, void *),
-			      void *data)
+static int acpi_walk_dep_device_list(acpi_handle handle,
+				int (*callback)(struct acpi_dep_data *, void *),
+				void *data)
 {
 	struct acpi_dep_data *dep, *tmp;
 	int ret = 0;
@@ -2164,7 +2164,6 @@ int acpi_walk_dep_device_list(acpi_handl
 
 	return ret > 0 ? 0 : ret;
 }
-EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
 
 /**
  * acpi_dev_clear_dependencies - Inform consumers that the device is now active
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -668,9 +668,6 @@ extern bool acpi_driver_match_device(str
 				     const struct device_driver *drv);
 int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
 int acpi_device_modalias(struct device *, char *, int);
-int acpi_walk_dep_device_list(acpi_handle handle,
-			      int (*callback)(struct acpi_dep_data *, void *),
-			      void *data);
 
 struct platform_device *acpi_create_platform_device(struct acpi_device *,
 						    struct property_entry *);

[PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In general, acpi_bus_attach() can only be run safely under
acpi_scan_lock, but that lock cannot be acquired under
acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred
execution of acpi_bus_attach() under acpi_scan_lock instead of
calling it directly.

This also fixes a possible race between acpi_scan_clear_dep() and
device removal that might cause a device object that went away to
be accessed, because acpi_scan_clear_dep() is changed to acquire
a reference on the consumer device object.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d
 	return 0;
 }
 
-static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
-{
+struct acpi_scan_clear_dep_work {
+	struct work_struct work;
 	struct acpi_device *adev;
+};
+
+static void acpi_scan_clear_dep_fn(struct work_struct *work)
+{
+	struct acpi_scan_clear_dep_work *cdw;
+
+	cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
 
-	acpi_bus_get_device(dep->consumer, &adev);
+	acpi_scan_lock_acquire();
+	acpi_bus_attach(cdw->adev, true);
+	acpi_scan_lock_release();
+
+	acpi_dev_put(cdw->adev);
+	kfree(cdw);
+}
+
+static bool acpi_scan_clear_dep_queue(struct acpi_device *adev)
+{
+	struct acpi_scan_clear_dep_work *cdw;
+
+	if (adev->dep_unmet)
+		return false;
+
+	cdw = kmalloc(sizeof(*cdw), GFP_KERNEL);
+	if (!cdw)
+		return false;
+
+	cdw->adev = adev;
+	INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn);
+	/*
+	 * Since the work function may block on the lock until the entire
+	 * initial enumeration of devices is complete, put it into the unbound
+	 * workqueue.
+	 */
+	queue_work(system_unbound_wq, &cdw->work);
+
+	return true;
+}
+
+static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
+{
+	struct acpi_device *adev = acpi_bus_get_acpi_device(dep->consumer);
 
 	if (adev) {
 		adev->dep_unmet--;
-		if (!adev->dep_unmet)
-			acpi_bus_attach(adev, true);
+		if (!acpi_scan_clear_dep_queue(adev))
+			acpi_dev_put(adev);
 	}
 
 	list_del(&dep->node);

[PATCH 4/5] ACPI: scan: Reorganize acpi_device_add()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the invocation of acpi_attach_data() in acpi_device_add()
into a separate function.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -640,23 +640,32 @@ static int acpi_device_set_name(struct a
 	return 0;
 }
 
+static int acpi_tie_acpi_dev(struct acpi_device *adev)
+{
+	acpi_handle handle = adev->handle;
+	acpi_status status;
+
+	if (!handle)
+		return 0;
+
+	status = acpi_attach_data(handle, acpi_scan_drop_device, adev);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_err(handle, "Unable to attach device data\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;
 
-	if (device->handle) {
-		acpi_status status;
-
-		status = acpi_attach_data(device->handle, acpi_scan_drop_device,
-					  device);
-		if (ACPI_FAILURE(status)) {
-			acpi_handle_err(device->handle,
-					"Unable to attach device data\n");
-			return -ENODEV;
-		}
-	}
+	result = acpi_tie_acpi_dev(device);
+	if (result)
+		return result;
 
 	/*
 	 * Linkage

[PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_add_single_object() runs concurrently with respect to
acpi_scan_clear_dep() which deletes a dependencies list entry where
the device being added is the consumer, the device's dep_unmet
counter may not be updated to reflect that change.

Namely, if the dependencies list entry is deleted right after
calling acpi_scan_dep_init() and before calling acpi_device_add(),
acpi_scan_clear_dep() will not find the device object corresponding
to the consumer device ACPI handle and it will not update its
dep_unmet counter to reflect the deletion of the list entry.
Consequently, the dep_unmet counter of the device will never
become zero going forward which may prevent it from being
completely enumerated.

To address this problem, modify acpi_add_single_object() to run
acpi_tie_acpi_dev(), to attach the ACPI device object created by it
to the corresponding ACPI namespace node, under acpi_dep_list_lock
along with acpi_scan_dep_init() whenever the latter is called.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi
 	return 0;
 }
 
-int acpi_device_add(struct acpi_device *device,
-		    void (*release)(struct device *))
+int __acpi_device_add(struct acpi_device *device,
+		      void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;
 
-	result = acpi_tie_acpi_dev(device);
-	if (result)
-		return result;
-
 	/*
 	 * Linkage
 	 * -------
@@ -755,6 +751,17 @@ err_unlock:
 	return result;
 }
 
+int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
+{
+	int ret;
+
+	ret = acpi_tie_acpi_dev(adev);
+	if (ret)
+		return ret;
+
+	return __acpi_device_add(adev, release);
+}
+
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
@@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac
 {
 	struct acpi_dep_data *dep;
 
-	mutex_lock(&acpi_dep_list_lock);
-
 	list_for_each_entry(dep, &acpi_dep_list, node) {
 		if (dep->consumer == adev->handle)
 			adev->dep_unmet++;
 	}
-
-	mutex_unlock(&acpi_dep_list_lock);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct
 				  acpi_handle handle, int type, bool dep_init)
 {
 	struct acpi_device *device;
+	bool release_dep_lock = false;
 	int result;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
@@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct
 	 * this must be done before the get power-/wakeup_dev-flags calls.
 	 */
 	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
-		if (dep_init)
+		if (dep_init) {
+			mutex_lock(&acpi_dep_list_lock);
+			/*
+			 * Hold the lock until the acpi_tie_acpi_dev() call
+			 * below to prevent concurrent acpi_scan_clear_dep()
+			 * from deleting a dependency list entry without
+			 * updating dep_unmet for the device.
+			 */
+			release_dep_lock = true;
 			acpi_scan_dep_init(device);
-
+		}
 		acpi_scan_init_status(device);
 	}
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
-	result = acpi_device_add(device, acpi_device_release);
+	result = acpi_tie_acpi_dev(device);
+
+	if (release_dep_lock)
+		mutex_unlock(&acpi_dep_list_lock);
+
+	if (result)
+		return result;
+
+	result = __acpi_device_add(device, acpi_device_release);
 	if (result) {
 		acpi_device_release(&device->dev);
 		return result;

Re: [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb()

[Hans de Goede]

Hi,

On 6/16/21 4:21 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make acpi_dev_get_first_consumer_dev_cb() a bit more straightforward
> and rewrite the comment in it.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/acpi/scan.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2107,13 +2107,12 @@ static int acpi_dev_get_first_consumer_d
>  	struct acpi_device *adev;
>  
>  	adev = acpi_bus_get_acpi_device(dep->consumer);
> -	if (!adev)
> -		/* If we don't find an adev then we want to continue parsing */
> -		return 0;
> -
> -	*(struct acpi_device **)data = adev;
> -
> -	return 1;
> +	if (adev) {
> +		*(struct acpi_device **)data = adev;
> +		return 1;
> +	}
> +	/* Continue parsing if the device object is not present. */
> +	return 0;
>  }
>  
>  static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
> 
> 
> 

Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()

[Hans de Goede]

Hi,

On 6/16/21 4:22 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because acpi_walk_dep_device_list() is only called by the code in the
> file in which it is defined, make it static, drop the export of it
> and drop its header from acpi.h.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Actually, acpi_walk_dep_device_list() was split out as a
helper function used to implement acpi_dev_clear_dependencies()
because it will be used outside of drivers/acpi.
Specifically it will be used in the new intel_skl_int3472 driver:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/

Which I plan to merge into pdx86/for-next today, I've just merged
your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list()
as preparation for this.

Regards,

Hans


> ---
>  drivers/acpi/scan.c  |    7 +++----
>  include/linux/acpi.h |    3 ---
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2145,9 +2145,9 @@ static int acpi_scan_clear_dep(struct ac
>   * negative value is returned by the callback then the loop is broken and that
>   * value is returned as the final error.
>   */
> -int acpi_walk_dep_device_list(acpi_handle handle,
> -			      int (*callback)(struct acpi_dep_data *, void *),
> -			      void *data)
> +static int acpi_walk_dep_device_list(acpi_handle handle,
> +				int (*callback)(struct acpi_dep_data *, void *),
> +				void *data)
>  {
>  	struct acpi_dep_data *dep, *tmp;
>  	int ret = 0;
> @@ -2164,7 +2164,6 @@ int acpi_walk_dep_device_list(acpi_handl
>  
>  	return ret > 0 ? 0 : ret;
>  }
> -EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
>  
>  /**
>   * acpi_dev_clear_dependencies - Inform consumers that the device is now active
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -668,9 +668,6 @@ extern bool acpi_driver_match_device(str
>  				     const struct device_driver *drv);
>  int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
>  int acpi_device_modalias(struct device *, char *, int);
> -int acpi_walk_dep_device_list(acpi_handle handle,
> -			      int (*callback)(struct acpi_dep_data *, void *),
> -			      void *data);
>  
>  struct platform_device *acpi_create_platform_device(struct acpi_device *,
>  						    struct property_entry *);
> 
> 
> 

Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()

[Hans de Goede]

Hi,

On 6/16/21 4:23 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In general, acpi_bus_attach() can only be run safely under
> acpi_scan_lock, but that lock cannot be acquired under
> acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred
> execution of acpi_bus_attach() under acpi_scan_lock instead of
> calling it directly.
> 
> This also fixes a possible race between acpi_scan_clear_dep() and
> device removal that might cause a device object that went away to
> be accessed, because acpi_scan_clear_dep() is changed to acquire
> a reference on the consumer device object.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d
>  	return 0;
>  }
>  
> -static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
> -{
> +struct acpi_scan_clear_dep_work {
> +	struct work_struct work;
>  	struct acpi_device *adev;
> +};
> +
> +static void acpi_scan_clear_dep_fn(struct work_struct *work)
> +{
> +	struct acpi_scan_clear_dep_work *cdw;
> +
> +	cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
>  
> -	acpi_bus_get_device(dep->consumer, &adev);
> +	acpi_scan_lock_acquire();
> +	acpi_bus_attach(cdw->adev, true);
> +	acpi_scan_lock_release();
> +
> +	acpi_dev_put(cdw->adev);
> +	kfree(cdw);
> +}
> +
> +static bool acpi_scan_clear_dep_queue(struct acpi_device *adev)
> +{
> +	struct acpi_scan_clear_dep_work *cdw;
> +
> +	if (adev->dep_unmet)
> +		return false;
> +
> +	cdw = kmalloc(sizeof(*cdw), GFP_KERNEL);
> +	if (!cdw)
> +		return false;
> +
> +	cdw->adev = adev;
> +	INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn);
> +	/*
> +	 * Since the work function may block on the lock until the entire
> +	 * initial enumeration of devices is complete, put it into the unbound
> +	 * workqueue.
> +	 */
> +	queue_work(system_unbound_wq, &cdw->work);

Hmm, I'm a bit worried about this. Even with the system_unbound_wq
some code may expect at least some progress being made with processing
works during the initial enumeration. OTOH this does run pretty early on.

Still I wonder if it would not be better to create + use our own workqueue
for this ?

I guess we can always do this if we run into issues later...

With that said / otherwise the patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> +
> +	return true;
> +}
> +
> +static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
> +{
> +	struct acpi_device *adev = acpi_bus_get_acpi_device(dep->consumer);
>  
>  	if (adev) {
>  		adev->dep_unmet--;
> -		if (!adev->dep_unmet)
> -			acpi_bus_attach(adev, true);
> +		if (!acpi_scan_clear_dep_queue(adev))
> +			acpi_dev_put(adev);
>  	}
>  
>  	list_del(&dep->node);
> 
> 
> 

Re: [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add()

[Hans de Goede]

Hi,

On 6/16/21 4:24 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move the invocation of acpi_attach_data() in acpi_device_add()
> into a separate function.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/acpi/scan.c |   31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -640,23 +640,32 @@ static int acpi_device_set_name(struct a
>  	return 0;
>  }
>  
> +static int acpi_tie_acpi_dev(struct acpi_device *adev)
> +{
> +	acpi_handle handle = adev->handle;
> +	acpi_status status;
> +
> +	if (!handle)
> +		return 0;
> +
> +	status = acpi_attach_data(handle, acpi_scan_drop_device, adev);
> +	if (ACPI_FAILURE(status)) {
> +		acpi_handle_err(handle, "Unable to attach device data\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  int acpi_device_add(struct acpi_device *device,
>  		    void (*release)(struct device *))
>  {
>  	struct acpi_device_bus_id *acpi_device_bus_id;
>  	int result;
>  
> -	if (device->handle) {
> -		acpi_status status;
> -
> -		status = acpi_attach_data(device->handle, acpi_scan_drop_device,
> -					  device);
> -		if (ACPI_FAILURE(status)) {
> -			acpi_handle_err(device->handle,
> -					"Unable to attach device data\n");
> -			return -ENODEV;
> -		}
> -	}
> +	result = acpi_tie_acpi_dev(device);
> +	if (result)
> +		return result;
>  
>  	/*
>  	 * Linkage
> 
> 
> 

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[Hans de Goede]

Hi,

On 6/16/21 4:25 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If acpi_add_single_object() runs concurrently with respect to
> acpi_scan_clear_dep() which deletes a dependencies list entry where
> the device being added is the consumer, the device's dep_unmet
> counter may not be updated to reflect that change.
> 
> Namely, if the dependencies list entry is deleted right after
> calling acpi_scan_dep_init() and before calling acpi_device_add(),
> acpi_scan_clear_dep() will not find the device object corresponding
> to the consumer device ACPI handle and it will not update its
> dep_unmet counter to reflect the deletion of the list entry.
> Consequently, the dep_unmet counter of the device will never
> become zero going forward which may prevent it from being
> completely enumerated.
> 
> To address this problem, modify acpi_add_single_object() to run
> acpi_tie_acpi_dev(), to attach the ACPI device object created by it
> to the corresponding ACPI namespace node, under acpi_dep_list_lock
> along with acpi_scan_dep_init() whenever the latter is called.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi
>  	return 0;
>  }
>  
> -int acpi_device_add(struct acpi_device *device,
> -		    void (*release)(struct device *))
> +int __acpi_device_add(struct acpi_device *device,
> +		      void (*release)(struct device *))
>  {
>  	struct acpi_device_bus_id *acpi_device_bus_id;
>  	int result;
>  
> -	result = acpi_tie_acpi_dev(device);
> -	if (result)
> -		return result;
> -
>  	/*
>  	 * Linkage
>  	 * -------
> @@ -755,6 +751,17 @@ err_unlock:
>  	return result;
>  }
>  
> +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
> +{
> +	int ret;
> +
> +	ret = acpi_tie_acpi_dev(adev);
> +	if (ret)
> +		return ret;
> +
> +	return __acpi_device_add(adev, release);
> +}
> +
>  /* --------------------------------------------------------------------------
>                                   Device Enumeration
>     -------------------------------------------------------------------------- */
> @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac
>  {
>  	struct acpi_dep_data *dep;
>  
> -	mutex_lock(&acpi_dep_list_lock);
> -
>  	list_for_each_entry(dep, &acpi_dep_list, node) {
>  		if (dep->consumer == adev->handle)
>  			adev->dep_unmet++;
>  	}
> -
> -	mutex_unlock(&acpi_dep_list_lock);
>  }
>  
>  void acpi_device_add_finalize(struct acpi_device *device)
> @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct
>  				  acpi_handle handle, int type, bool dep_init)
>  {
>  	struct acpi_device *device;
> +	bool release_dep_lock = false;
>  	int result;
>  
>  	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct
>  	 * this must be done before the get power-/wakeup_dev-flags calls.
>  	 */
>  	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
> -		if (dep_init)
> +		if (dep_init) {
> +			mutex_lock(&acpi_dep_list_lock);
> +			/*
> +			 * Hold the lock until the acpi_tie_acpi_dev() call
> +			 * below to prevent concurrent acpi_scan_clear_dep()
> +			 * from deleting a dependency list entry without
> +			 * updating dep_unmet for the device.
> +			 */
> +			release_dep_lock = true;
>  			acpi_scan_dep_init(device);
> -
> +		}
>  		acpi_scan_init_status(device);
>  	}
>  
>  	acpi_bus_get_power_flags(device);
>  	acpi_bus_get_wakeup_device_flags(device);
>  
> -	result = acpi_device_add(device, acpi_device_release);
> +	result = acpi_tie_acpi_dev(device);
> +
> +	if (release_dep_lock)
> +		mutex_unlock(&acpi_dep_list_lock);
> +
> +	if (result)

AFAICT you are missing a "acpi_device_release(&device->dev);"
call in the error-exit path here, causing a mem-leak.

Otherwise this looks good, with the above fixed this is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> +		return result;
> +
> +	result = __acpi_device_add(device, acpi_device_release);
>  	if (result) {
>  		acpi_device_release(&device->dev);
>  		return result;
> 
> 
> 

Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()

[Rafael J. Wysocki]

On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 6/16/21 4:22 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Because acpi_walk_dep_device_list() is only called by the code in the
> > file in which it is defined, make it static, drop the export of it
> > and drop its header from acpi.h.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Actually, acpi_walk_dep_device_list() was split out as a
> helper function used to implement acpi_dev_clear_dependencies()
> because it will be used outside of drivers/acpi.

Not exactly.

> Specifically it will be used in the new intel_skl_int3472 driver:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/

That driver will use acpi_dev_get_first_consumer_dev() which is based
on acpi_walk_dep_device_list(), but still defined in
drivers/acpi/scan.c.

> Which I plan to merge into pdx86/for-next today, I've just merged
> your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list()
> as preparation for this.

No, the acpi_walk_dep_device_list() is a leftover there AFAICS.

If it needs to be exported in the future, that still can be done.  ATM
the export isn't necessary.

Thanks!

Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()

[Rafael J. Wysocki]

On Wed, Jun 16, 2021 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 6/16/21 4:23 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In general, acpi_bus_attach() can only be run safely under
> > acpi_scan_lock, but that lock cannot be acquired under
> > acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred
> > execution of acpi_bus_attach() under acpi_scan_lock instead of
> > calling it directly.
> >
> > This also fixes a possible race between acpi_scan_clear_dep() and
> > device removal that might cause a device object that went away to
> > be accessed, because acpi_scan_clear_dep() is changed to acquire
> > a reference on the consumer device object.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/scan.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 45 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d
> >       return 0;
> >  }
> >
> > -static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
> > -{
> > +struct acpi_scan_clear_dep_work {
> > +     struct work_struct work;
> >       struct acpi_device *adev;
> > +};
> > +
> > +static void acpi_scan_clear_dep_fn(struct work_struct *work)
> > +{
> > +     struct acpi_scan_clear_dep_work *cdw;
> > +
> > +     cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
> >
> > -     acpi_bus_get_device(dep->consumer, &adev);
> > +     acpi_scan_lock_acquire();
> > +     acpi_bus_attach(cdw->adev, true);
> > +     acpi_scan_lock_release();
> > +
> > +     acpi_dev_put(cdw->adev);
> > +     kfree(cdw);
> > +}
> > +
> > +static bool acpi_scan_clear_dep_queue(struct acpi_device *adev)
> > +{
> > +     struct acpi_scan_clear_dep_work *cdw;
> > +
> > +     if (adev->dep_unmet)
> > +             return false;
> > +
> > +     cdw = kmalloc(sizeof(*cdw), GFP_KERNEL);
> > +     if (!cdw)
> > +             return false;
> > +
> > +     cdw->adev = adev;
> > +     INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn);
> > +     /*
> > +      * Since the work function may block on the lock until the entire
> > +      * initial enumeration of devices is complete, put it into the unbound
> > +      * workqueue.
> > +      */
> > +     queue_work(system_unbound_wq, &cdw->work);
>
> Hmm, I'm a bit worried about this. Even with the system_unbound_wq
> some code may expect at least some progress being made with processing
> works during the initial enumeration. OTOH this does run pretty early on.
>
> Still I wonder if it would not be better to create + use our own workqueue
> for this ?
>
> I guess we can always do this if we run into issues later...

Exactly my thought.

> With that said / otherwise the patch looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[Rafael J. Wysocki]

On Wed, Jun 16, 2021 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 6/16/21 4:25 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If acpi_add_single_object() runs concurrently with respect to
> > acpi_scan_clear_dep() which deletes a dependencies list entry where
> > the device being added is the consumer, the device's dep_unmet
> > counter may not be updated to reflect that change.
> >
> > Namely, if the dependencies list entry is deleted right after
> > calling acpi_scan_dep_init() and before calling acpi_device_add(),
> > acpi_scan_clear_dep() will not find the device object corresponding
> > to the consumer device ACPI handle and it will not update its
> > dep_unmet counter to reflect the deletion of the list entry.
> > Consequently, the dep_unmet counter of the device will never
> > become zero going forward which may prevent it from being
> > completely enumerated.
> >
> > To address this problem, modify acpi_add_single_object() to run
> > acpi_tie_acpi_dev(), to attach the ACPI device object created by it
> > to the corresponding ACPI namespace node, under acpi_dep_list_lock
> > along with acpi_scan_dep_init() whenever the latter is called.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/scan.c |   46 +++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 33 insertions(+), 13 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi
> >       return 0;
> >  }
> >
> > -int acpi_device_add(struct acpi_device *device,
> > -                 void (*release)(struct device *))
> > +int __acpi_device_add(struct acpi_device *device,
> > +                   void (*release)(struct device *))
> >  {
> >       struct acpi_device_bus_id *acpi_device_bus_id;
> >       int result;
> >
> > -     result = acpi_tie_acpi_dev(device);
> > -     if (result)
> > -             return result;
> > -
> >       /*
> >        * Linkage
> >        * -------
> > @@ -755,6 +751,17 @@ err_unlock:
> >       return result;
> >  }
> >
> > +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
> > +{
> > +     int ret;
> > +
> > +     ret = acpi_tie_acpi_dev(adev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return __acpi_device_add(adev, release);
> > +}
> > +
> >  /* --------------------------------------------------------------------------
> >                                   Device Enumeration
> >     -------------------------------------------------------------------------- */
> > @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac
> >  {
> >       struct acpi_dep_data *dep;
> >
> > -     mutex_lock(&acpi_dep_list_lock);
> > -
> >       list_for_each_entry(dep, &acpi_dep_list, node) {
> >               if (dep->consumer == adev->handle)
> >                       adev->dep_unmet++;
> >       }
> > -
> > -     mutex_unlock(&acpi_dep_list_lock);
> >  }
> >
> >  void acpi_device_add_finalize(struct acpi_device *device)
> > @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct
> >                                 acpi_handle handle, int type, bool dep_init)
> >  {
> >       struct acpi_device *device;
> > +     bool release_dep_lock = false;
> >       int result;
> >
> >       device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> > @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct
> >        * this must be done before the get power-/wakeup_dev-flags calls.
> >        */
> >       if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
> > -             if (dep_init)
> > +             if (dep_init) {
> > +                     mutex_lock(&acpi_dep_list_lock);
> > +                     /*
> > +                      * Hold the lock until the acpi_tie_acpi_dev() call
> > +                      * below to prevent concurrent acpi_scan_clear_dep()
> > +                      * from deleting a dependency list entry without
> > +                      * updating dep_unmet for the device.
> > +                      */
> > +                     release_dep_lock = true;
> >                       acpi_scan_dep_init(device);
> > -
> > +             }
> >               acpi_scan_init_status(device);
> >       }
> >
> >       acpi_bus_get_power_flags(device);
> >       acpi_bus_get_wakeup_device_flags(device);
> >
> > -     result = acpi_device_add(device, acpi_device_release);
> > +     result = acpi_tie_acpi_dev(device);
> > +
> > +     if (release_dep_lock)
> > +             mutex_unlock(&acpi_dep_list_lock);
> > +
> > +     if (result)
>
> AFAICT you are missing a "acpi_device_release(&device->dev);"
> call in the error-exit path here, causing a mem-leak.

Indeed.

I'll send a v2 of this patch alone to fix this issue.

> Otherwise this looks good, with the above fixed this is:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()

[Rafael J. Wysocki]

On Wed, Jun 16, 2021 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 6/16/21 4:22 PM, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Because acpi_walk_dep_device_list() is only called by the code in the
> > > file in which it is defined, make it static, drop the export of it
> > > and drop its header from acpi.h.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Actually, acpi_walk_dep_device_list() was split out as a
> > helper function used to implement acpi_dev_clear_dependencies()
> > because it will be used outside of drivers/acpi.
>
> Not exactly.
>
> > Specifically it will be used in the new intel_skl_int3472 driver:
> > https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/
>
> That driver will use acpi_dev_get_first_consumer_dev() which is based
> on acpi_walk_dep_device_list(), but still defined in
> drivers/acpi/scan.c.
>
> > Which I plan to merge into pdx86/for-next today, I've just merged
> > your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list()
> > as preparation for this.
>
> No, the acpi_walk_dep_device_list() is a leftover there AFAICS.

I mean the export of it.

> If it needs to be exported in the future, that still can be done.  ATM
> the export isn't necessary.
>
> Thanks!

Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()

[Hans de Goede]

Hi,

On 6/16/21 5:11 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 6/16/21 4:22 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Because acpi_walk_dep_device_list() is only called by the code in the
>>> file in which it is defined, make it static, drop the export of it
>>> and drop its header from acpi.h.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Actually, acpi_walk_dep_device_list() was split out as a
>> helper function used to implement acpi_dev_clear_dependencies()
>> because it will be used outside of drivers/acpi.
> 
> Not exactly.
> 
>> Specifically it will be used in the new intel_skl_int3472 driver:
>> https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/
> 
> That driver will use acpi_dev_get_first_consumer_dev() which is based
> on acpi_walk_dep_device_list(), but still defined in
> drivers/acpi/scan.c.
> 
>> Which I plan to merge into pdx86/for-next today, I've just merged
>> your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list()
>> as preparation for this.
> 
> No, the acpi_walk_dep_device_list() is a leftover there AFAICS.

You are right, my bad.

So with this resolved, this patch is fine too:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

[PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_add_single_object() runs concurrently with respect to
acpi_scan_clear_dep() which deletes a dependencies list entry where
the device being added is the consumer, the device's dep_unmet
counter may not be updated to reflect that change.

Namely, if the dependencies list entry is deleted right after
calling acpi_scan_dep_init() and before calling acpi_device_add(),
acpi_scan_clear_dep() will not find the device object corresponding
to the consumer device ACPI handle and it will not update its
dep_unmet counter to reflect the deletion of the list entry.
Consequently, the dep_unmet counter of the device will never
become zero going forward which may prevent it from being
completely enumerated.

To address this problem, modify acpi_add_single_object() to run
acpi_tie_acpi_dev(), to attach the ACPI device object created by it
to the corresponding ACPI namespace node, under acpi_dep_list_lock
along with acpi_scan_dep_init() whenever the latter is called.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---

-> v2:
   * Fix memory leak spotted by Hans.
   * Add the R-by tag from Hans.

---
 drivers/acpi/scan.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -657,16 +652,12 @@ static int acpi_tie_acpi_dev(struct acpi
 	return 0;
 }
 
-int acpi_device_add(struct acpi_device *device,
-		    void (*release)(struct device *))
+int __acpi_device_add(struct acpi_device *device,
+		      void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;
 
-	result = acpi_tie_acpi_dev(device);
-	if (result)
-		return result;
-
 	/*
 	 * Linkage
 	 * -------
@@ -755,6 +746,17 @@ err_unlock:
 	return result;
 }
 
+int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
+{
+	int ret;
+
+	ret = acpi_tie_acpi_dev(adev);
+	if (ret)
+		return ret;
+
+	return __acpi_device_add(adev, release);
+}
+
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
@@ -1681,14 +1683,10 @@ static void acpi_scan_dep_init(struct ac
 {
 	struct acpi_dep_data *dep;
 
-	mutex_lock(&acpi_dep_list_lock);
-
 	list_for_each_entry(dep, &acpi_dep_list, node) {
 		if (dep->consumer == adev->handle)
 			adev->dep_unmet++;
 	}
-
-	mutex_unlock(&acpi_dep_list_lock);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1707,6 +1705,7 @@ static int acpi_add_single_object(struct
 				  acpi_handle handle, int type, bool dep_init)
 {
 	struct acpi_device *device;
+	bool release_dep_lock = false;
 	int result;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
@@ -1720,16 +1719,31 @@ static int acpi_add_single_object(struct
 	 * this must be done before the get power-/wakeup_dev-flags calls.
 	 */
 	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
-		if (dep_init)
+		if (dep_init) {
+			mutex_lock(&acpi_dep_list_lock);
+			/*
+			 * Hold the lock until the acpi_tie_acpi_dev() call
+			 * below to prevent concurrent acpi_scan_clear_dep()
+			 * from deleting a dependency list entry without
+			 * updating dep_unmet for the device.
+			 */
+			release_dep_lock = true;
 			acpi_scan_dep_init(device);
-
+		}
 		acpi_scan_init_status(device);
 	}
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
-	result = acpi_device_add(device, acpi_device_release);
+	result = acpi_tie_acpi_dev(device);
+
+	if (release_dep_lock)
+		mutex_unlock(&acpi_dep_list_lock);
+
+	if (!result)
+		result = __acpi_device_add(device, acpi_device_release);
+
 	if (result) {
 		acpi_device_release(&device->dev);
 		return result;

Re: [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies

[Hans de Goede]

Hi,

On 6/16/21 8:00 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If acpi_add_single_object() runs concurrently with respect to
> acpi_scan_clear_dep() which deletes a dependencies list entry where
> the device being added is the consumer, the device's dep_unmet
> counter may not be updated to reflect that change.
> 
> Namely, if the dependencies list entry is deleted right after
> calling acpi_scan_dep_init() and before calling acpi_device_add(),
> acpi_scan_clear_dep() will not find the device object corresponding
> to the consumer device ACPI handle and it will not update its
> dep_unmet counter to reflect the deletion of the list entry.
> Consequently, the dep_unmet counter of the device will never
> become zero going forward which may prevent it from being
> completely enumerated.
> 
> To address this problem, modify acpi_add_single_object() to run
> acpi_tie_acpi_dev(), to attach the ACPI device object created by it
> to the corresponding ACPI namespace node, under acpi_dep_list_lock
> along with acpi_scan_dep_init() whenever the latter is called.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

FWIW this looks good to me now.

Regards,

Hans


> ---
> 
> -> v2:
>    * Fix memory leak spotted by Hans.
>    * Add the R-by tag from Hans.
> 
> ---
>  drivers/acpi/scan.c |   50 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -657,16 +652,12 @@ static int acpi_tie_acpi_dev(struct acpi
>  	return 0;
>  }
>  
> -int acpi_device_add(struct acpi_device *device,
> -		    void (*release)(struct device *))
> +int __acpi_device_add(struct acpi_device *device,
> +		      void (*release)(struct device *))
>  {
>  	struct acpi_device_bus_id *acpi_device_bus_id;
>  	int result;
>  
> -	result = acpi_tie_acpi_dev(device);
> -	if (result)
> -		return result;
> -
>  	/*
>  	 * Linkage
>  	 * -------
> @@ -755,6 +746,17 @@ err_unlock:
>  	return result;
>  }
>  
> +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
> +{
> +	int ret;
> +
> +	ret = acpi_tie_acpi_dev(adev);
> +	if (ret)
> +		return ret;
> +
> +	return __acpi_device_add(adev, release);
> +}
> +
>  /* --------------------------------------------------------------------------
>                                   Device Enumeration
>     -------------------------------------------------------------------------- */
> @@ -1681,14 +1683,10 @@ static void acpi_scan_dep_init(struct ac
>  {
>  	struct acpi_dep_data *dep;
>  
> -	mutex_lock(&acpi_dep_list_lock);
> -
>  	list_for_each_entry(dep, &acpi_dep_list, node) {
>  		if (dep->consumer == adev->handle)
>  			adev->dep_unmet++;
>  	}
> -
> -	mutex_unlock(&acpi_dep_list_lock);
>  }
>  
>  void acpi_device_add_finalize(struct acpi_device *device)
> @@ -1707,6 +1705,7 @@ static int acpi_add_single_object(struct
>  				  acpi_handle handle, int type, bool dep_init)
>  {
>  	struct acpi_device *device;
> +	bool release_dep_lock = false;
>  	int result;
>  
>  	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> @@ -1720,16 +1719,31 @@ static int acpi_add_single_object(struct
>  	 * this must be done before the get power-/wakeup_dev-flags calls.
>  	 */
>  	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
> -		if (dep_init)
> +		if (dep_init) {
> +			mutex_lock(&acpi_dep_list_lock);
> +			/*
> +			 * Hold the lock until the acpi_tie_acpi_dev() call
> +			 * below to prevent concurrent acpi_scan_clear_dep()
> +			 * from deleting a dependency list entry without
> +			 * updating dep_unmet for the device.
> +			 */
> +			release_dep_lock = true;
>  			acpi_scan_dep_init(device);
> -
> +		}
>  		acpi_scan_init_status(device);
>  	}
>  
>  	acpi_bus_get_power_flags(device);
>  	acpi_bus_get_wakeup_device_flags(device);
>  
> -	result = acpi_device_add(device, acpi_device_release);
> +	result = acpi_tie_acpi_dev(device);
> +
> +	if (release_dep_lock)
> +		mutex_unlock(&acpi_dep_list_lock);
> +
> +	if (!result)
> +		result = __acpi_device_add(device, acpi_device_release);
> +
>  	if (result) {
>  		acpi_device_release(&device->dev);
>  		return result;
> 
> 
> 

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5204 bytes --]

Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-b001-20210615 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/acpi/scan.c:660:5: warning: no previous prototype for function '__acpi_device_add' [-Wmissing-prototypes]
   int __acpi_device_add(struct acpi_device *device,
       ^
   drivers/acpi/scan.c:660:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __acpi_device_add(struct acpi_device *device,
   ^
   static 
   1 warning generated.


vim +/__acpi_device_add +660 drivers/acpi/scan.c

   659	
 > 660	int __acpi_device_add(struct acpi_device *device,
   661			      void (*release)(struct device *))
   662	{
   663		struct acpi_device_bus_id *acpi_device_bus_id;
   664		int result;
   665	
   666		/*
   667		 * Linkage
   668		 * -------
   669		 * Link this device to its parent and siblings.
   670		 */
   671		INIT_LIST_HEAD(&device->children);
   672		INIT_LIST_HEAD(&device->node);
   673		INIT_LIST_HEAD(&device->wakeup_list);
   674		INIT_LIST_HEAD(&device->physical_node_list);
   675		INIT_LIST_HEAD(&device->del_list);
   676		mutex_init(&device->physical_node_lock);
   677	
   678		mutex_lock(&acpi_device_lock);
   679	
   680		acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device));
   681		if (acpi_device_bus_id) {
   682			result = acpi_device_set_name(device, acpi_device_bus_id);
   683			if (result)
   684				goto err_unlock;
   685		} else {
   686			acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id),
   687						     GFP_KERNEL);
   688			if (!acpi_device_bus_id) {
   689				result = -ENOMEM;
   690				goto err_unlock;
   691			}
   692			acpi_device_bus_id->bus_id =
   693				kstrdup_const(acpi_device_hid(device), GFP_KERNEL);
   694			if (!acpi_device_bus_id->bus_id) {
   695				kfree(acpi_device_bus_id);
   696				result = -ENOMEM;
   697				goto err_unlock;
   698			}
   699	
   700			ida_init(&acpi_device_bus_id->instance_ida);
   701	
   702			result = acpi_device_set_name(device, acpi_device_bus_id);
   703			if (result) {
   704				kfree_const(acpi_device_bus_id->bus_id);
   705				kfree(acpi_device_bus_id);
   706				goto err_unlock;
   707			}
   708	
   709			list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
   710		}
   711	
   712		if (device->parent)
   713			list_add_tail(&device->node, &device->parent->children);
   714	
   715		if (device->wakeup.flags.valid)
   716			list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
   717	
   718		mutex_unlock(&acpi_device_lock);
   719	
   720		if (device->parent)
   721			device->dev.parent = &device->parent->dev;
   722	
   723		device->dev.bus = &acpi_bus_type;
   724		device->dev.release = release;
   725		result = device_add(&device->dev);
   726		if (result) {
   727			dev_err(&device->dev, "Error registering device\n");
   728			goto err;
   729		}
   730	
   731		result = acpi_device_setup_files(device);
   732		if (result)
   733			pr_err("Error creating sysfs interface for device %s\n",
   734			       dev_name(&device->dev));
   735	
   736		return 0;
   737	
   738	err:
   739		mutex_lock(&acpi_device_lock);
   740	
   741		if (device->parent)
   742			list_del(&device->node);
   743	
   744		list_del(&device->wakeup_list);
   745	
   746	err_unlock:
   747		mutex_unlock(&acpi_device_lock);
   748	
   749		acpi_detach_data(device->handle, acpi_scan_drop_device);
   750	
   751		return result;
   752	}
   753	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30253 bytes --]

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5349 bytes --]

Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-b001-20210615 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/acpi/scan.c:660:5: warning: no previous prototype for function '__acpi_device_add' [-Wmissing-prototypes]
   int __acpi_device_add(struct acpi_device *device,
       ^
   drivers/acpi/scan.c:660:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __acpi_device_add(struct acpi_device *device,
   ^
   static 
   1 warning generated.


vim +/__acpi_device_add +660 drivers/acpi/scan.c

   659	
 > 660	int __acpi_device_add(struct acpi_device *device,
   661			      void (*release)(struct device *))
   662	{
   663		struct acpi_device_bus_id *acpi_device_bus_id;
   664		int result;
   665	
   666		/*
   667		 * Linkage
   668		 * -------
   669		 * Link this device to its parent and siblings.
   670		 */
   671		INIT_LIST_HEAD(&device->children);
   672		INIT_LIST_HEAD(&device->node);
   673		INIT_LIST_HEAD(&device->wakeup_list);
   674		INIT_LIST_HEAD(&device->physical_node_list);
   675		INIT_LIST_HEAD(&device->del_list);
   676		mutex_init(&device->physical_node_lock);
   677	
   678		mutex_lock(&acpi_device_lock);
   679	
   680		acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device));
   681		if (acpi_device_bus_id) {
   682			result = acpi_device_set_name(device, acpi_device_bus_id);
   683			if (result)
   684				goto err_unlock;
   685		} else {
   686			acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id),
   687						     GFP_KERNEL);
   688			if (!acpi_device_bus_id) {
   689				result = -ENOMEM;
   690				goto err_unlock;
   691			}
   692			acpi_device_bus_id->bus_id =
   693				kstrdup_const(acpi_device_hid(device), GFP_KERNEL);
   694			if (!acpi_device_bus_id->bus_id) {
   695				kfree(acpi_device_bus_id);
   696				result = -ENOMEM;
   697				goto err_unlock;
   698			}
   699	
   700			ida_init(&acpi_device_bus_id->instance_ida);
   701	
   702			result = acpi_device_set_name(device, acpi_device_bus_id);
   703			if (result) {
   704				kfree_const(acpi_device_bus_id->bus_id);
   705				kfree(acpi_device_bus_id);
   706				goto err_unlock;
   707			}
   708	
   709			list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
   710		}
   711	
   712		if (device->parent)
   713			list_add_tail(&device->node, &device->parent->children);
   714	
   715		if (device->wakeup.flags.valid)
   716			list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
   717	
   718		mutex_unlock(&acpi_device_lock);
   719	
   720		if (device->parent)
   721			device->dev.parent = &device->parent->dev;
   722	
   723		device->dev.bus = &acpi_bus_type;
   724		device->dev.release = release;
   725		result = device_add(&device->dev);
   726		if (result) {
   727			dev_err(&device->dev, "Error registering device\n");
   728			goto err;
   729		}
   730	
   731		result = acpi_device_setup_files(device);
   732		if (result)
   733			pr_err("Error creating sysfs interface for device %s\n",
   734			       dev_name(&device->dev));
   735	
   736		return 0;
   737	
   738	err:
   739		mutex_lock(&acpi_device_lock);
   740	
   741		if (device->parent)
   742			list_del(&device->node);
   743	
   744		list_del(&device->wakeup_list);
   745	
   746	err_unlock:
   747		mutex_unlock(&acpi_device_lock);
   748	
   749		acpi_detach_data(device->handle, acpi_scan_drop_device);
   750	
   751		return result;
   752	}
   753	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30253 bytes --]

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s001-20210615 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/acpi/scan.c:660:5: sparse: sparse: symbol '__acpi_device_add' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41825 bytes --]

Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

Hi "Rafael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-s001-20210615 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528
        git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/acpi/scan.c:660:5: sparse: sparse: symbol '__acpi_device_add' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 41825 bytes --]

[RFC PATCH] ACPI: scan: __acpi_device_add() can be static

[kernel test robot]

drivers/acpi/scan.c:660:5: warning: symbol '__acpi_device_add' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index be8b149244220..f30ab5a55a9a4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -657,8 +657,8 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev)
 	return 0;
 }
 
-int __acpi_device_add(struct acpi_device *device,
-		      void (*release)(struct device *))
+static int __acpi_device_add(struct acpi_device *device,
+			     void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;

[RFC PATCH] ACPI: scan: __acpi_device_add() can be static

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

drivers/acpi/scan.c:660:5: warning: symbol '__acpi_device_add' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index be8b149244220..f30ab5a55a9a4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -657,8 +657,8 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev)
 	return 0;
 }
 
-int __acpi_device_add(struct acpi_device *device,
-		      void (*release)(struct device *))
+static int __acpi_device_add(struct acpi_device *device,
+			     void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;