All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling
@ 2021-06-16 14:21 Rafael J. Wysocki
  2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:21 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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!




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

* [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb()
  2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
@ 2021-06-16 14:21 ` Rafael J. Wysocki
  2021-06-16 14:36   ` Hans de Goede
  2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:21 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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)




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

* [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()
  2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
  2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
@ 2021-06-16 14:22 ` Rafael J. Wysocki
  2021-06-16 14:41   ` Hans de Goede
  2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:22 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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




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

* [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()
  2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
  2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
  2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki
@ 2021-06-16 14:23 ` Rafael J. Wysocki
  2021-06-16 14:48   ` Hans de Goede
  2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
  4 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:23 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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);




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

* [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add()
  2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki
@ 2021-06-16 14:24 ` Rafael J. Wysocki
  2021-06-16 14:49   ` Hans de Goede
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
  4 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:24 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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




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

* [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
                   ` (3 preceding siblings ...)
  2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki
@ 2021-06-16 14:25 ` Rafael J. Wysocki
  2021-06-16 14:55   ` Hans de Goede
                     ` (4 more replies)
  4 siblings, 5 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 14:25 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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;




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

* Re: [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb()
  2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
@ 2021-06-16 14:36   ` Hans de Goede
  0 siblings, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 14:36 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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


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

* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()
  2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki
@ 2021-06-16 14:41   ` Hans de Goede
  2021-06-16 15:11     ` Rafael J. Wysocki
  0 siblings, 1 reply; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 14:41 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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


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

* Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()
  2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki
@ 2021-06-16 14:48   ` Hans de Goede
  2021-06-16 15:12     ` Rafael J. Wysocki
  0 siblings, 1 reply; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 14:48 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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);
> 
> 
> 


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

* Re: [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add()
  2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki
@ 2021-06-16 14:49   ` Hans de Goede
  0 siblings, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 14:49 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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


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

* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
@ 2021-06-16 14:55   ` Hans de Goede
  2021-06-16 15:19     ` Rafael J. Wysocki
  2021-06-16 18:00   ` [PATCH v2 " Rafael J. Wysocki
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 14:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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


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

* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()
  2021-06-16 14:41   ` Hans de Goede
@ 2021-06-16 15:11     ` Rafael J. Wysocki
  2021-06-16 15:25       ` Rafael J. Wysocki
  2021-06-16 15:28       ` Hans de Goede
  0 siblings, 2 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 15:11 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML

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!

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

* Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep()
  2021-06-16 14:48   ` Hans de Goede
@ 2021-06-16 15:12     ` Rafael J. Wysocki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 15:12 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML

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!

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

* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:55   ` Hans de Goede
@ 2021-06-16 15:19     ` Rafael J. Wysocki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 15:19 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML

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!

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

* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()
  2021-06-16 15:11     ` Rafael J. Wysocki
@ 2021-06-16 15:25       ` Rafael J. Wysocki
  2021-06-16 15:28       ` Hans de Goede
  1 sibling, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 15:25 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML

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!

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

* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list()
  2021-06-16 15:11     ` Rafael J. Wysocki
  2021-06-16 15:25       ` Rafael J. Wysocki
@ 2021-06-16 15:28       ` Hans de Goede
  1 sibling, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 15:28 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux ACPI, LKML

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


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

* [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
  2021-06-16 14:55   ` Hans de Goede
@ 2021-06-16 18:00   ` Rafael J. Wysocki
  2021-06-16 18:04     ` Hans de Goede
  2021-06-17  0:25   ` [PATCH " kernel test robot
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-06-16 18:00 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Hans de Goede

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;




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

* Re: [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 18:00   ` [PATCH v2 " Rafael J. Wysocki
@ 2021-06-16 18:04     ` Hans de Goede
  0 siblings, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2021-06-16 18:04 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML

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


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

* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
  2021-06-16 14:55   ` Hans de Goede
  2021-06-16 18:00   ` [PATCH v2 " Rafael J. Wysocki
@ 2021-06-17  0:25   ` kernel test robot
  2021-06-17  0:41   ` kernel test robot
  2021-06-17  0:42   ` [RFC PATCH] ACPI: scan: __acpi_device_add() can be static kernel test robot
  4 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-06-17  0:25 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI
  Cc: kbuild-all, clang-built-linux, LKML, Hans de Goede

[-- 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 --]

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

* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
                     ` (2 preceding siblings ...)
  2021-06-17  0:25   ` [PATCH " kernel test robot
@ 2021-06-17  0:41   ` kernel test robot
  2021-06-17  0:42   ` [RFC PATCH] ACPI: scan: __acpi_device_add() can be static kernel test robot
  4 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-06-17  0:41 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: kbuild-all, LKML, Hans de Goede

[-- 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 --]

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

* [RFC PATCH] ACPI: scan: __acpi_device_add() can be static
  2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
                     ` (3 preceding siblings ...)
  2021-06-17  0:41   ` kernel test robot
@ 2021-06-17  0:42   ` kernel test robot
  4 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-06-17  0:42 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI; +Cc: kbuild-all, LKML, Hans de Goede

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;

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

end of thread, other threads:[~2021-06-17  0:42 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
2021-06-16 14:36   ` Hans de Goede
2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki
2021-06-16 14:41   ` Hans de Goede
2021-06-16 15:11     ` Rafael J. Wysocki
2021-06-16 15:25       ` Rafael J. Wysocki
2021-06-16 15:28       ` Hans de Goede
2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki
2021-06-16 14:48   ` Hans de Goede
2021-06-16 15:12     ` Rafael J. Wysocki
2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki
2021-06-16 14:49   ` Hans de Goede
2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
2021-06-16 14:55   ` Hans de Goede
2021-06-16 15:19     ` Rafael J. Wysocki
2021-06-16 18:00   ` [PATCH v2 " Rafael J. Wysocki
2021-06-16 18:04     ` Hans de Goede
2021-06-17  0:25   ` [PATCH " kernel test robot
2021-06-17  0:41   ` kernel test robot
2021-06-17  0:42   ` [RFC PATCH] ACPI: scan: __acpi_device_add() can be static kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.