* [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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ 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] 24+ 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; 24+ 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] 24+ 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-17 0:25 ` kernel test robot
2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ 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] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
@ 2021-06-17 0:25 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-06-17 0:25 UTC (permalink / raw)
To: kbuild-all
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ 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-17 0:41 ` kernel test robot
2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ 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] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies
@ 2021-06-17 0:41 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-06-17 0:41 UTC (permalink / raw)
To: kbuild-all
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ 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
@ 2021-06-17 0:42 ` kernel test robot
2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki
` (3 subsequent siblings)
4 siblings, 0 replies; 24+ 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 related [flat|nested] 24+ messages in thread
* [RFC PATCH] ACPI: scan: __acpi_device_add() can be static
@ 2021-06-17 0:42 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-06-17 0:42 UTC (permalink / raw)
To: kbuild-all
[-- 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;
^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-06-17 0:42 UTC | newest]
Thread overview: 24+ 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:25 ` kernel test robot
2021-06-17 0:41 ` 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
2021-06-17 0:42 ` 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.