Hi All,
There are still opportunities to clean up the ACPI support code and
this series is part of the effort in that direction.
It makes changes without functional impact (AFAICS) to the core ACPI
code related to devices and to some of its users.
Please refer to the patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Instead of having acpi_device_add() defined as a wrapper around
__acpi_device_add(), export acpi_tie_acpi_dev() so it can be called
directly by acpi_add_power_resource(), fold acpi_device_add() into the
latter and rename __acpi_device_add() to acpi_device_add().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/internal.h | 1 +
drivers/acpi/power.c | 6 +++++-
drivers/acpi/scan.c | 17 +++--------------
3 files changed, 9 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -104,6 +104,7 @@ struct acpi_device_bus_id {
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
int type, void (*release)(struct device *));
+int acpi_tie_acpi_dev(struct acpi_device *adev);
int acpi_device_add(struct acpi_device *device);
int acpi_device_setup_files(struct acpi_device *dev);
void acpi_device_remove_files(struct acpi_device *dev);
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -643,7 +643,7 @@ static int acpi_device_set_name(struct a
return 0;
}
-static int acpi_tie_acpi_dev(struct acpi_device *adev)
+int acpi_tie_acpi_dev(struct acpi_device *adev)
{
acpi_handle handle = adev->handle;
acpi_status status;
@@ -673,7 +673,7 @@ static void acpi_store_pld_crc(struct ac
ACPI_FREE(pld);
}
-static int __acpi_device_add(struct acpi_device *device)
+int acpi_device_add(struct acpi_device *device)
{
struct acpi_device_bus_id *acpi_device_bus_id;
int result;
@@ -755,17 +755,6 @@ err_unlock:
return result;
}
-int acpi_device_add(struct acpi_device *adev)
-{
- int ret;
-
- ret = acpi_tie_acpi_dev(adev);
- if (ret)
- return ret;
-
- return __acpi_device_add(adev);
-}
-
/* --------------------------------------------------------------------------
Device Enumeration
-------------------------------------------------------------------------- */
@@ -1866,7 +1855,7 @@ static int acpi_add_single_object(struct
mutex_unlock(&acpi_dep_list_lock);
if (!result)
- result = __acpi_device_add(device);
+ result = acpi_device_add(device);
if (result) {
acpi_device_release(&device->dev);
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -952,6 +952,7 @@ struct acpi_device *acpi_add_power_resou
strcpy(acpi_device_name(device), ACPI_POWER_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_POWER_CLASS);
device->power.state = ACPI_STATE_UNKNOWN;
+ device->flags.match_driver = true;
/* Evaluate the object to get the system level and resource order. */
status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
@@ -968,7 +969,10 @@ struct acpi_device *acpi_add_power_resou
pr_info("%s [%s]\n", acpi_device_name(device), acpi_device_bid(device));
- device->flags.match_driver = true;
+ result = acpi_tie_acpi_dev(device);
+ if (result)
+ goto err;
+
result = acpi_device_add(device);
if (result)
goto err;
From: Rafael J. Wysocki <[email protected]>
Because acpi_bus_get_acpi_device() is completely analogous to
acpi_fetch_acpi_dev(), rename it to acpi_get_acpi_dev() and
add a kerneldoc comment to it.
Accordingly, rename acpi_bus_put_acpi_device() to acpi_put_acpi_dev()
and update all of the users of these two functions.
While at it, move the acpi_fetch_acpi_dev() header next to the
acpi_get_acpi_dev() header in the header file holding them.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/bus.c | 6 +++---
drivers/acpi/device_pm.c | 4 ++--
drivers/acpi/irq.c | 4 ++--
drivers/acpi/scan.c | 21 ++++++++++++++++-----
drivers/hwmon/acpi_power_meter.c | 2 +-
include/acpi/acpi_bus.h | 6 +++---
6 files changed, 27 insertions(+), 16 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -511,7 +511,6 @@ extern int unregister_acpi_notifier(stru
* External Functions
*/
-struct acpi_device *acpi_fetch_acpi_dev(acpi_handle handle);
acpi_status acpi_bus_get_status_handle(acpi_handle handle,
unsigned long long *sta);
int acpi_bus_get_status(struct acpi_device *device);
@@ -766,9 +765,10 @@ static inline void acpi_dev_put(struct a
put_device(&adev->dev);
}
-struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle);
+struct acpi_device *acpi_fetch_acpi_dev(acpi_handle handle);
+struct acpi_device *acpi_get_acpi_dev(acpi_handle handle);
-static inline void acpi_bus_put_acpi_device(struct acpi_device *adev)
+static inline void acpi_put_acpi_dev(struct acpi_device *adev)
{
acpi_dev_put(adev);
}
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -429,7 +429,7 @@ void acpi_device_hotplug(struct acpi_dev
acpi_evaluate_ost(adev->handle, src, ost_code, NULL);
out:
- acpi_bus_put_acpi_device(adev);
+ acpi_put_acpi_dev(adev);
mutex_unlock(&acpi_scan_lock);
unlock_device_hotplug();
}
@@ -599,11 +599,22 @@ static void get_acpi_device(void *dev)
acpi_dev_get(dev);
}
-struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle)
+/**
+ * acpi_get_acpi_dev - Retrieve ACPI device object and reference count it.
+ * @handle: ACPI handle associated with the requested ACPI device object.
+ *
+ * Return a pointer to the ACPI device object associated with @handle and bump
+ * up that object's reference counter (under the ACPI Namespace lock), if
+ * present, or return NULL otherwise.
+ *
+ * The ACPI device object reference acquired by this function needs to be
+ * dropped via acpi_dev_put().
+ */
+struct acpi_device *acpi_get_acpi_dev(acpi_handle handle)
{
return handle_to_device(handle, get_acpi_device);
}
-EXPORT_SYMBOL_GPL(acpi_bus_get_acpi_device);
+EXPORT_SYMBOL_GPL(acpi_get_acpi_dev);
static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id)
{
@@ -2238,7 +2249,7 @@ static int acpi_dev_get_first_consumer_d
{
struct acpi_device *adev;
- adev = acpi_bus_get_acpi_device(dep->consumer);
+ adev = acpi_get_acpi_dev(dep->consumer);
if (adev) {
*(struct acpi_device **)data = adev;
return 1;
@@ -2291,7 +2302,7 @@ static bool acpi_scan_clear_dep_queue(st
static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data)
{
- struct acpi_device *adev = acpi_bus_get_acpi_device(dep->consumer);
+ struct acpi_device *adev = acpi_get_acpi_dev(dep->consumer);
if (adev) {
adev->dep_unmet--;
Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -511,7 +511,7 @@ static void acpi_bus_notify(acpi_handle
break;
}
- adev = acpi_bus_get_acpi_device(handle);
+ adev = acpi_get_acpi_dev(handle);
if (!adev)
goto err;
@@ -524,14 +524,14 @@ static void acpi_bus_notify(acpi_handle
}
if (!hotplug_event) {
- acpi_bus_put_acpi_device(adev);
+ acpi_put_acpi_dev(adev);
return;
}
if (ACPI_SUCCESS(acpi_hotplug_schedule(adev, type)))
return;
- acpi_bus_put_acpi_device(adev);
+ acpi_put_acpi_dev(adev);
err:
acpi_evaluate_ost(handle, type, ost_code, NULL);
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -497,7 +497,7 @@ static void acpi_pm_notify_handler(acpi_
acpi_handle_debug(handle, "Wake notify\n");
- adev = acpi_bus_get_acpi_device(handle);
+ adev = acpi_get_acpi_dev(handle);
if (!adev)
return;
@@ -515,7 +515,7 @@ static void acpi_pm_notify_handler(acpi_
mutex_unlock(&acpi_pm_notifier_lock);
- acpi_bus_put_acpi_device(adev);
+ acpi_put_acpi_dev(adev);
}
/**
Index: linux-pm/drivers/acpi/irq.c
===================================================================
--- linux-pm.orig/drivers/acpi/irq.c
+++ linux-pm/drivers/acpi/irq.c
@@ -111,12 +111,12 @@ acpi_get_irq_source_fwhandle(const struc
if (WARN_ON(ACPI_FAILURE(status)))
return NULL;
- device = acpi_bus_get_acpi_device(handle);
+ device = acpi_get_acpi_dev(handle);
if (WARN_ON(!device))
return NULL;
result = &device->fwnode;
- acpi_bus_put_acpi_device(device);
+ acpi_put_acpi_dev(device);
return result;
}
Index: linux-pm/drivers/hwmon/acpi_power_meter.c
===================================================================
--- linux-pm.orig/drivers/hwmon/acpi_power_meter.c
+++ linux-pm/drivers/hwmon/acpi_power_meter.c
@@ -598,7 +598,7 @@ static int read_domain_devices(struct ac
continue;
/* Create a symlink to domain objects */
- obj = acpi_bus_get_acpi_device(element->reference.handle);
+ obj = acpi_get_acpi_dev(element->reference.handle);
resource->domain_devices[i] = obj;
if (!obj)
continue;
Hi Rafael,
"Rafael J. Wysocki" <[email protected]> writes:
> Hi All,
>
> There are still opportunities to clean up the ACPI support code and
> this series is part of the effort in that direction.
>
> It makes changes without functional impact (AFAICS) to the core ACPI
> code related to devices and to some of its users.
>
> Please refer to the patch changelogs for details.
Other than the single typo I noticed in Patch 2, the changes look good!
If it helps,
Reviewed-by: Punit Agrawal <[email protected]>
Thanks,
Punit
[...]
On Fri, Aug 12, 2022 at 3:11 PM Punit Agrawal
<[email protected]> wrote:
>
> Hi Rafael,
>
> "Rafael J. Wysocki" <[email protected]> writes:
>
> > Hi All,
> >
> > There are still opportunities to clean up the ACPI support code and
> > this series is part of the effort in that direction.
> >
> > It makes changes without functional impact (AFAICS) to the core ACPI
> > code related to devices and to some of its users.
> >
> > Please refer to the patch changelogs for details.
>
> Other than the single typo I noticed in Patch 2,
I've fixed that one while applying the patch.
> the changes look good!
>
> If it helps,
>
> Reviewed-by: Punit Agrawal <[email protected]>
Thank you!
From: Rafael J. Wysocki <[email protected]>
In principle, it should be valid to pass NULL as the ACPI device
pointer to acpi_device_get_power() and acpi_device_set_power() and they
both are expected to return -EINVAL in that case, but that has been
broken recently by commit 62fcb99bdf10 ("ACPI: Drop parent field from
struct acpi_device") which has caused the ACPI device pointer to be
dereferenced in these functions before the NULL check.
Fix that and while at it make acpi_device_set_power() only use the
parent field if the target ACPI device object's ignore_parent flag
in not set.
Fixes: 62fcb99bdf10 ("ACPI: Drop parent field from struct acpi_device")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
linux-next material.
---
drivers/acpi/device_pm.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -74,13 +74,15 @@ static int acpi_dev_pm_explicit_get(stru
*/
int acpi_device_get_power(struct acpi_device *device, int *state)
{
- struct acpi_device *parent = acpi_dev_parent(device);
int result = ACPI_STATE_UNKNOWN;
+ struct acpi_device *parent;
int error;
if (!device || !state)
return -EINVAL;
+ parent = acpi_dev_parent(device);
+
if (!device->flags.power_manageable) {
/* TBD: Non-recursive algorithm for walking up hierarchy. */
*state = parent ? parent->power.state : ACPI_STATE_D0;
@@ -159,7 +161,6 @@ static int acpi_dev_pm_explicit_set(stru
*/
int acpi_device_set_power(struct acpi_device *device, int state)
{
- struct acpi_device *parent = acpi_dev_parent(device);
int target_state = state;
int result = 0;
@@ -192,13 +193,17 @@ int acpi_device_set_power(struct acpi_de
return -ENODEV;
}
- if (!device->power.flags.ignore_parent && parent &&
- state < parent->power.state) {
- acpi_handle_debug(device->handle,
- "Cannot transition to %s for parent in %s\n",
- acpi_power_state_string(state),
- acpi_power_state_string(parent->power.state));
- return -ENODEV;
+ if (!device->power.flags.ignore_parent) {
+ struct acpi_device *parent;
+
+ parent = acpi_dev_parent(device);
+ if (parent && state < parent->power.state) {
+ acpi_handle_debug(device->handle,
+ "Cannot transition to %s for parent in %s\n",
+ acpi_power_state_string(state),
+ acpi_power_state_string(parent->power.state));
+ return -ENODEV;
+ }
}
/*
From: Rafael J. Wysocki <[email protected]>
Because acpi_dev_parent() is defined as static inline, the extra
header of it in acpi_bus.h is redundant, so drop it.
Fixes: 62fcb99bdf10 ("ACPI: Drop parent field from struct acpi_device")
Reported-by: Hanjun Guo <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
linux-next material
---
include/acpi/acpi_bus.h | 1 -
1 file changed, 1 deletion(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -485,7 +485,6 @@ void acpi_initialize_hp_context(struct a
/* acpi_device.dev.bus == &acpi_bus_type */
extern struct bus_type acpi_bus_type;
-struct acpi_device *acpi_dev_parent(struct acpi_device *adev);
int acpi_bus_for_each_dev(int (*fn)(struct device *, void *), void *data);
int acpi_dev_for_each_child(struct acpi_device *adev,
int (*fn)(struct acpi_device *, void *), void *data);
On 2022/8/29 23:53, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Because acpi_dev_parent() is defined as static inline, the extra
> header of it in acpi_bus.h is redundant, so drop it.
>
> Fixes: 62fcb99bdf10 ("ACPI: Drop parent field from struct acpi_device")
> Reported-by: Hanjun Guo <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Hanjun Guo <[email protected]>
Thanks
Hanjun