From 8e0cbf356377fabac47a027dd176cd1cacc5fc01 Mon Sep 17 00:00:00 2001 From: Mark Pearson Date: Tue, 29 Dec 2020 19:18:25 -0500 Subject: [PATCH 01/13] Documentation: Add documentation for new platform_profile sysfs attribute On modern systems the platform performance, temperature, fan and other hardware related characteristics are often dynamically configurable. The profile is often automatically adjusted to the load by some automatic-mechanism (which may very well live outside the kernel). These auto platform-adjustment mechanisms often can be configured with one of several 'platform-profiles', with either a bias towards low-power consumption or towards performance (and higher power consumption and thermals). Introduce a new platform_profile sysfs API which offers a generic API for selecting the performance-profile of these automatic-mechanisms. Co-developed-by: Hans de Goede Signed-off-by: Mark Pearson Signed-off-by: Hans de Goede Signed-off-by: Rafael J. Wysocki --- .../ABI/testing/sysfs-platform_profile | 24 +++++++++++ Documentation/userspace-api/index.rst | 1 + .../userspace-api/sysfs-platform_profile.rst | 42 +++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-platform_profile create mode 100644 Documentation/userspace-api/sysfs-platform_profile.rst diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile new file mode 100644 index 000000000000..9d6b89b66cca --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform_profile @@ -0,0 +1,24 @@ +What: /sys/firmware/acpi/platform_profile_choices +Date: October 2020 +Contact: Hans de Goede +Description: This file contains a space-separated list of profiles supported for this device. + + Drivers must use the following standard profile-names: + + ============ ============================================ + low-power Low power consumption + cool Cooler operation + quiet Quieter operation + balanced Balance between low power consumption and performance + performance High performance operation + ============ ============================================ + + Userspace may expect drivers to offer more than one of these + standard profile names. + +What: /sys/firmware/acpi/platform_profile +Date: October 2020 +Contact: Hans de Goede +Description: Reading this file gives the current selected profile for this + device. Writing this file with one of the strings from + platform_profile_choices changes the profile to the new value. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index acd2cc2a538d..d29b020e5622 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -24,6 +24,7 @@ place where this information is gathered. ioctl/index iommu media/index + sysfs-platform_profile .. only:: subproject and html diff --git a/Documentation/userspace-api/sysfs-platform_profile.rst b/Documentation/userspace-api/sysfs-platform_profile.rst new file mode 100644 index 000000000000..c33a71263d9e --- /dev/null +++ b/Documentation/userspace-api/sysfs-platform_profile.rst @@ -0,0 +1,42 @@ +===================================================================== +Platform Profile Selection (e.g. /sys/firmware/acpi/platform_profile) +===================================================================== + +On modern systems the platform performance, temperature, fan and other +hardware related characteristics are often dynamically configurable. The +platform configuration is often automatically adjusted to the current +conditions by some automatic mechanism (which may very well live outside +the kernel). + +These auto platform adjustment mechanisms often can be configured with +one of several platform profiles, with either a bias towards low power +operation or towards performance. + +The purpose of the platform_profile attribute is to offer a generic sysfs +API for selecting the platform profile of these automatic mechanisms. + +Note that this API is only for selecting the platform profile, it is +NOT a goal of this API to allow monitoring the resulting performance +characteristics. Monitoring performance is best done with device/vendor +specific tools such as e.g. turbostat. + +Specifically when selecting a high performance profile the actual achieved +performance may be limited by various factors such as: the heat generated +by other components, room temperature, free air flow at the bottom of a +laptop, etc. It is explicitly NOT a goal of this API to let userspace know +about any sub-optimal conditions which are impeding reaching the requested +performance level. + +Since numbers on their own cannot represent the multiple variables that a +profile will adjust (power consumption, heat generation, etc) this API +uses strings to describe the various profiles. To make sure that userspace +gets a consistent experience the sysfs-platform_profile ABI document defines +a fixed set of profile names. Drivers *must* map their internal profile +representation onto this fixed set. + +If there is no good match when mapping then a new profile name may be +added. Drivers which wish to introduce new profile names must: + + 1. Explain why the existing profile names canot be used. + 2. Add the new profile name, along with a clear description of the + expected behaviour, to the sysfs-platform_profile ABI documentation. From a2ff95e018f1d2bc816f3078d5110a655e355f18 Mon Sep 17 00:00:00 2001 From: Mark Pearson Date: Tue, 29 Dec 2020 19:18:26 -0500 Subject: [PATCH 02/13] ACPI: platform: Add platform profile support This is the initial implementation of the platform-profile feature. It provides the details discussed and outlined in the sysfs-platform_profile document. Many modern systems have the ability to modify the operating profile to control aspects like fan speed, temperature and power levels. This module provides a common sysfs interface that platform modules can register against to control their individual profile options. Signed-off-by: Mark Pearson Reviewed-by: Hans de Goede [ rjw: Use full words in enum values names ] Signed-off-by: Rafael J. Wysocki --- drivers/acpi/Kconfig | 17 +++ drivers/acpi/Makefile | 1 + drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++ include/linux/platform_profile.h | 39 +++++++ 4 files changed, 238 insertions(+) create mode 100644 drivers/acpi/platform_profile.c create mode 100644 include/linux/platform_profile.h diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index edf1558c1105..5ddff93e38c2 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -326,6 +326,23 @@ config ACPI_THERMAL To compile this driver as a module, choose M here: the module will be called thermal. +config ACPI_PLATFORM_PROFILE + tristate "ACPI Platform Profile Driver" + default m + help + This driver adds support for platform-profiles on platforms that + support it. + + Platform-profiles can be used to control the platform behaviour. For + example whether to operate in a lower power mode, in a higher + power performance mode or between the two. + + This driver provides the sysfs interface and is used as the registration + point for platform specific drivers. + + Which profiles are supported is determined on a per-platform basis and + should be obtained from the platform specific driver. + config ACPI_CUSTOM_DSDT_FILE string "Custom DSDT Table file to include" default "" diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 076894a3330f..52b627c7f977 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -79,6 +79,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o obj-$(CONFIG_ACPI_PROCESSOR) += processor.o obj-$(CONFIG_ACPI) += container.o obj-$(CONFIG_ACPI_THERMAL) += thermal.o +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o obj-$(CONFIG_ACPI_NFIT) += nfit/ obj-$(CONFIG_ACPI_NUMA) += numa/ obj-$(CONFIG_ACPI) += acpi_memhotplug.o diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c new file mode 100644 index 000000000000..91be50a32cc8 --- /dev/null +++ b/drivers/acpi/platform_profile.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* Platform profile sysfs interface */ + +#include +#include +#include +#include +#include +#include + +static const struct platform_profile_handler *cur_profile; +static DEFINE_MUTEX(profile_lock); + +static const char * const profile_names[] = { + [PLATFORM_PROFILE_LOW_POWER] = "low-power", + [PLATFORM_PROFILE_COOL] = "cool", + [PLATFORM_PROFILE_QUIET] = "quiet", + [PLATFORM_PROFILE_BALANCED] = "balanced", + [PLATFORM_PROFILE_PERFORMANCE] = "performance", +}; +static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); + +static ssize_t platform_profile_choices_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int len = 0; + int err, i; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) { + if (len == 0) + len += sysfs_emit_at(buf, len, "%s", profile_names[i]); + else + len += sysfs_emit_at(buf, len, " %s", profile_names[i]); + } + len += sysfs_emit_at(buf, len, "\n"); + mutex_unlock(&profile_lock); + return len; +} + +static ssize_t platform_profile_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED; + int err; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + err = cur_profile->profile_get(&profile); + mutex_unlock(&profile_lock); + if (err) + return err; + + /* Check that profile is valid index */ + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) + return -EIO; + + return sysfs_emit(buf, "%s\n", profile_names[profile]); +} + +static ssize_t platform_profile_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int err, i; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + /* Scan for a matching profile */ + i = sysfs_match_string(profile_names, buf); + if (i < 0) { + mutex_unlock(&profile_lock); + return -EINVAL; + } + + /* Check that platform supports this profile choice */ + if (!test_bit(i, cur_profile->choices)) { + mutex_unlock(&profile_lock); + return -EOPNOTSUPP; + } + + err = cur_profile->profile_set(i); + mutex_unlock(&profile_lock); + if (err) + return err; + return count; +} + +static DEVICE_ATTR_RO(platform_profile_choices); +static DEVICE_ATTR_RW(platform_profile); + +static struct attribute *platform_profile_attrs[] = { + &dev_attr_platform_profile_choices.attr, + &dev_attr_platform_profile.attr, + NULL +}; + +static const struct attribute_group platform_profile_group = { + .attrs = platform_profile_attrs +}; + +void platform_profile_notify(void) +{ + if (!cur_profile) + return; + sysfs_notify(acpi_kobj, NULL, "platform_profile"); +} +EXPORT_SYMBOL_GPL(platform_profile_notify); + +int platform_profile_register(const struct platform_profile_handler *pprof) +{ + int err; + + mutex_lock(&profile_lock); + /* We can only have one active profile */ + if (cur_profile) { + mutex_unlock(&profile_lock); + return -EEXIST; + } + + /* Sanity check the profile handler field are set */ + if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) || + !pprof->profile_set || !pprof->profile_get) { + mutex_unlock(&profile_lock); + return -EINVAL; + } + + err = sysfs_create_group(acpi_kobj, &platform_profile_group); + if (err) { + mutex_unlock(&profile_lock); + return err; + } + + cur_profile = pprof; + mutex_unlock(&profile_lock); + return 0; +} +EXPORT_SYMBOL_GPL(platform_profile_register); + +int platform_profile_remove(void) +{ + mutex_lock(&profile_lock); + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + sysfs_remove_group(acpi_kobj, &platform_profile_group); + cur_profile = NULL; + mutex_unlock(&profile_lock); + return 0; +} +EXPORT_SYMBOL_GPL(platform_profile_remove); + +MODULE_AUTHOR("Mark Pearson "); +MODULE_LICENSE("GPL"); diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h new file mode 100644 index 000000000000..3623d7108421 --- /dev/null +++ b/include/linux/platform_profile.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Platform profile sysfs interface + * + * See Documentation/ABI/testing/sysfs-platform_profile.rst for more + * information. + */ + +#ifndef _PLATFORM_PROFILE_H_ +#define _PLATFORM_PROFILE_H_ + +#include + +/* + * If more options are added please update profile_names + * array in platform-profile.c and sysfs-platform-profile.rst + * documentation. + */ + +enum platform_profile_option { + PLATFORM_PROFILE_LOW_POWER, + PLATFORM_PROFILE_COOL, + PLATFORM_PROFILE_QUIET, + PLATFORM_PROFILE_BALANCED, + PLATFORM_PROFILE_PERFORMANCE, + PLATFORM_PROFILE_LAST, /*must always be last */ +}; + +struct platform_profile_handler { + unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)]; + int (*profile_get)(enum platform_profile_option *profile); + int (*profile_set)(enum platform_profile_option profile); +}; + +int platform_profile_register(const struct platform_profile_handler *pprof); +int platform_profile_remove(void); +void platform_profile_notify(void); + +#endif /*_PLATFORM_PROFILE_H_*/ From 9d56653d14cd5e545599cd9e3013daa17df50cd4 Mon Sep 17 00:00:00 2001 From: Jiaxun Yang Date: Mon, 25 Jan 2021 12:59:56 +0100 Subject: [PATCH 03/13] ACPI: platform-profile: Drop const qualifier for cur_profile Drop the const qualifier from the static global cur_profile pointer declaration. This is a preparation patch for passing the cur_profile pointer as parameter to the profile_get() and profile_set() callbacks so that drivers dynamically allocating their driver-data struct, with their platform_profile_handler struct embedded, can use this pointer to get to their driver-data. Note this also requires dropping the const from the pprof platform_profile_register() function argument. Dropping this const is not a problem, non of the queued up consumers of platform_profile_register() actually pass in a const pointer. Link: https://lore.kernel.org/linux-acpi/5e7a4d87-52ef-e487-9cc2-8e7094beaa08@redhat.com/ Link: https://lore.kernel.org/r/20210114073429.176462-2-jiaxun.yang@flygoat.com Suggested-by: Hans de Goede Signed-off-by: Jiaxun Yang [ hdegoede@redhat.com: Also remove const from platform_profile_register() ] Signed-off-by: Hans de Goede Signed-off-by: Rafael J. Wysocki --- drivers/acpi/platform_profile.c | 4 ++-- include/linux/platform_profile.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 91be50a32cc8..f65c61db7921 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -9,7 +9,7 @@ #include #include -static const struct platform_profile_handler *cur_profile; +static struct platform_profile_handler *cur_profile; static DEFINE_MUTEX(profile_lock); static const char * const profile_names[] = { @@ -132,7 +132,7 @@ void platform_profile_notify(void) } EXPORT_SYMBOL_GPL(platform_profile_notify); -int platform_profile_register(const struct platform_profile_handler *pprof) +int platform_profile_register(struct platform_profile_handler *pprof) { int err; diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index 3623d7108421..c797fdb3d91a 100644 --- a/include/linux/platform_profile.h +++ b/include/linux/platform_profile.h @@ -32,7 +32,7 @@ struct platform_profile_handler { int (*profile_set)(enum platform_profile_option profile); }; -int platform_profile_register(const struct platform_profile_handler *pprof); +int platform_profile_register(struct platform_profile_handler *pprof); int platform_profile_remove(void); void platform_profile_notify(void); From c1013ff7a5472db637c56bb6237f8343398c03a7 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 14 Jan 2021 19:46:47 +0100 Subject: [PATCH 04/13] ACPI: scan: Rearrange memory allocation in acpi_device_add() The upfront allocation of new_bus_id is done to avoid allocating memory under acpi_device_lock, but it doesn't really help, because (1) it leads to many unnecessary memory allocations for _ADR devices, (2) kstrdup_const() is run under that lock anyway and (3) it complicates the code. Rearrange acpi_device_add() to allocate memory for a new struct acpi_device_bus_id instance only when necessary, eliminate a redundant local variable from it and reduce the number of labels in there. No intentional functional impact. Signed-off-by: Rafael J. Wysocki Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 57 +++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1db063b02f63..0fb1811772b5 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -623,12 +623,23 @@ void acpi_bus_put_acpi_device(struct acpi_device *adev) put_device(&adev->dev); } +static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id) +{ + struct acpi_device_bus_id *acpi_device_bus_id; + + /* Find suitable bus_id and instance number in acpi_bus_id_list. */ + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { + if (!strcmp(acpi_device_bus_id->bus_id, dev_id)) + return acpi_device_bus_id; + } + return NULL; +} + int acpi_device_add(struct acpi_device *device, void (*release)(struct device *)) { + struct acpi_device_bus_id *acpi_device_bus_id; int result; - struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id; - int found = 0; if (device->handle) { acpi_status status; @@ -654,38 +665,26 @@ int acpi_device_add(struct acpi_device *device, INIT_LIST_HEAD(&device->del_list); mutex_init(&device->physical_node_lock); - new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL); - if (!new_bus_id) { - pr_err(PREFIX "Memory allocation error\n"); - result = -ENOMEM; - goto err_detach; - } - mutex_lock(&acpi_device_lock); - /* - * Find suitable bus_id and instance number in acpi_bus_id_list - * If failed, create one and link it into acpi_bus_id_list - */ - list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { - if (!strcmp(acpi_device_bus_id->bus_id, - acpi_device_hid(device))) { - acpi_device_bus_id->instance_no++; - found = 1; - kfree(new_bus_id); - break; + + acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); + if (acpi_device_bus_id) { + acpi_device_bus_id->instance_no++; + } else { + acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), + GFP_KERNEL); + if (!acpi_device_bus_id) { + result = -ENOMEM; + goto err_unlock; } - } - if (!found) { - acpi_device_bus_id = new_bus_id; acpi_device_bus_id->bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); if (!acpi_device_bus_id->bus_id) { - pr_err(PREFIX "Memory allocation error for bus id\n"); + kfree(acpi_device_bus_id); result = -ENOMEM; - goto err_free_new_bus_id; + goto err_unlock; } - acpi_device_bus_id->instance_no = 0; list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); } dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, acpi_device_bus_id->instance_no); @@ -720,13 +719,9 @@ int acpi_device_add(struct acpi_device *device, list_del(&device->node); list_del(&device->wakeup_list); - err_free_new_bus_id: - if (!found) - kfree(new_bus_id); - + err_unlock: mutex_unlock(&acpi_device_lock); - err_detach: acpi_detach_data(device->handle, acpi_scan_drop_device); return result; } From 5e73c5187cf4f40a5e02b6c8e4dd0fcf9686c006 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 14 Jan 2021 19:47:37 +0100 Subject: [PATCH 05/13] ACPI: scan: Adjust white space in acpi_device_add() Add empty lines in some places in acpi_device_add() to help readability and drop leading spaces before the labels in there. No functional impact. Signed-off-by: Rafael J. Wysocki Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 0fb1811772b5..1510afa7094d 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -694,10 +694,12 @@ int acpi_device_add(struct acpi_device *device, if (device->wakeup.flags.valid) list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list); + mutex_unlock(&acpi_device_lock); if (device->parent) device->dev.parent = &device->parent->dev; + device->dev.bus = &acpi_bus_type; device->dev.release = release; result = device_add(&device->dev); @@ -713,16 +715,19 @@ int acpi_device_add(struct acpi_device *device, return 0; - err: +err: mutex_lock(&acpi_device_lock); + if (device->parent) list_del(&device->node); + list_del(&device->wakeup_list); - err_unlock: +err_unlock: mutex_unlock(&acpi_device_lock); acpi_detach_data(device->handle, acpi_scan_drop_device); + return result; } From 83e2c8fc7ab89458b805e96ab37bccadf84f932b Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 18 Jan 2021 20:25:37 +0100 Subject: [PATCH 06/13] ACPI: scan: Rearrange code related to acpi_get_device_data() There are two callers of acpi_get_device_data(), acpi_bus_get_device() and acpi_bus_get_acpi_device(), but only one of them takes the int return value into account. Moreover, the latter knows that it passes a valid return pointer to acpi_get_device_data() and it properly clears that pointer upfront, so it doesn't need acpi_get_device_data() to do that. For this reason, rearrange acpi_get_device_data() to return a strct acpi_device pointer instead of an int and adapt its callers to that. While at it, rename acpi_get_device_data() to handle_to_device(), because the old name does not really reflect the functionality provided by that function. No intentional functional impact. Signed-off-by: Rafael J. Wysocki Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1510afa7094d..bcbf0fc215c8 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -578,29 +578,31 @@ static void acpi_scan_drop_device(acpi_handle handle, void *context) mutex_unlock(&acpi_device_del_lock); } -static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device, - void (*callback)(void *)) +static struct acpi_device *handle_to_device(acpi_handle handle, + void (*callback)(void *)) { + struct acpi_device *adev = NULL; acpi_status status; - if (!device) - return -EINVAL; - - *device = NULL; - status = acpi_get_data_full(handle, acpi_scan_drop_device, - (void **)device, callback); - if (ACPI_FAILURE(status) || !*device) { - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No context for object [%p]\n", - handle)); - return -ENODEV; + (void **)&adev, callback); + if (ACPI_FAILURE(status) || !adev) { + acpi_handle_debug(handle, "No context!\n"); + return NULL; } - return 0; + return adev; } int acpi_bus_get_device(acpi_handle handle, struct acpi_device **device) { - return acpi_get_device_data(handle, device, NULL); + if (!device) + return -EINVAL; + + *device = handle_to_device(handle, NULL); + if (!*device) + return -ENODEV; + + return 0; } EXPORT_SYMBOL(acpi_bus_get_device); @@ -612,10 +614,7 @@ static void get_acpi_device(void *dev) struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle) { - struct acpi_device *adev = NULL; - - acpi_get_device_data(handle, &adev, get_acpi_device); - return adev; + return handle_to_device(handle, get_acpi_device); } void acpi_bus_put_acpi_device(struct acpi_device *adev) From 84f9017c37c479c4f70456a645d24d2296ad2208 Mon Sep 17 00:00:00 2001 From: Jiaxun Yang Date: Mon, 25 Jan 2021 12:59:57 +0100 Subject: [PATCH 07/13] ACPI: platform-profile: Introduce object pointers to callbacks Add an object pointer to handler callbacks to avoid the need for drivers to have a global variable to get to their driver-data struct. Link: https://lore.kernel.org/linux-acpi/6a29f338-d9e4-150c-81dd-2ffb54f5bc35@redhat.com/ Link: https://lore.kernel.org/r/20210114073429.176462-3-jiaxun.yang@flygoat.com Signed-off-by: Jiaxun Yang Suggested-by: Hans de Goede Signed-off-by: Hans de Goede Signed-off-by: Rafael J. Wysocki --- drivers/acpi/platform_profile.c | 4 ++-- include/linux/platform_profile.h | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index f65c61db7921..80e9df427eb8 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev, return -ENODEV; } - err = cur_profile->profile_get(&profile); + err = cur_profile->profile_get(cur_profile, &profile); mutex_unlock(&profile_lock); if (err) return err; @@ -104,7 +104,7 @@ static ssize_t platform_profile_store(struct device *dev, return -EOPNOTSUPP; } - err = cur_profile->profile_set(i); + err = cur_profile->profile_set(cur_profile, i); mutex_unlock(&profile_lock); if (err) return err; diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index c797fdb3d91a..a26542d53058 100644 --- a/include/linux/platform_profile.h +++ b/include/linux/platform_profile.h @@ -28,8 +28,10 @@ enum platform_profile_option { struct platform_profile_handler { unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)]; - int (*profile_get)(enum platform_profile_option *profile); - int (*profile_set)(enum platform_profile_option profile); + int (*profile_get)(struct platform_profile_handler *pprof, + enum platform_profile_option *profile); + int (*profile_set)(struct platform_profile_handler *pprof, + enum platform_profile_option profile); }; int platform_profile_register(struct platform_profile_handler *pprof); From 041142d7d25294c17d39552ae51c1d8d89434010 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 25 Jan 2021 20:09:09 +0100 Subject: [PATCH 08/13] ACPI: platform-profile: Fix possible deadlock in platform_profile_remove() After a rmmod thinkpad_acpi, lockdep pointed out this possible deadlock: Our _show and _store sysfs attr functions get called with the kn->active lock held for the sysfs attr and then take the profile_lock. sysfs_remove_group() also takes the kn->active lock for the sysfs attr, so if we call it with the profile_lock held, then we get an ABBA deadlock. platform_profile_remove() must only be called by drivers which have first *successfully* called platform_profile_register(). Anything else is a driver bug. So the check for cur_profile being set before calling sysfs_remove_group() is not necessary and it can be dropped. It is safe to call sysfs_remove_group() without holding the profile_lock since the attr-group group cannot be re-added until after we clear cur_profile. Change platform_profile_remove() to only hold the profile_lock while clearing the cur_profile, fixing the deadlock. Signed-off-by: Hans de Goede Signed-off-by: Rafael J. Wysocki --- drivers/acpi/platform_profile.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 80e9df427eb8..4a59c5993bde 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -164,13 +164,9 @@ EXPORT_SYMBOL_GPL(platform_profile_register); int platform_profile_remove(void) { - mutex_lock(&profile_lock); - if (!cur_profile) { - mutex_unlock(&profile_lock); - return -ENODEV; - } - sysfs_remove_group(acpi_kobj, &platform_profile_group); + + mutex_lock(&profile_lock); cur_profile = NULL; mutex_unlock(&profile_lock); return 0; From e1e6bd2995ac0e1ad0c2a2d906a06f59ce2ed293 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 11 Feb 2021 19:30:01 +0100 Subject: [PATCH 09/13] ACPI: property: Fix fwnode string properties matching Property matching does not work for ACPI fwnodes if the value of the given property is not represented as a package in the _DSD package containing it. For example, the "compatible" property in the _DSD below Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"compatible", "ethernet-phy-ieee802.3-c45"} } }) will not be found by fwnode_property_match_string(), because the ACPI code handling device properties does not regard the single value as a "list" in that case. Namely, fwnode_property_match_string() invoked to match a given string property value first calls fwnode_property_read_string_array() with the last two arguments equal to NULL and 0, respectively, in order to count the items in the value of the given property, with the assumption that this value may be an array. For ACPI fwnodes, that operation is carried out by acpi_node_prop_read() which calls acpi_data_prop_read() for this purpose. However, when the return (val) pointer is NULL, that function only looks for a property whose value is a package without checking the single-value case at all. To fix that, make acpi_data_prop_read() check the single-value case if its return pointer argument is NULL and modify acpi_data_prop_read_single() handling that case to attempt to read the value of the property if the return pointer is NULL and return 1 if that succeeds. Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files") Reported-by: Calvin Johnson Cc: 4.13+ # 4.13+ Signed-off-by: Rafael J. Wysocki Reviewed-by: Sakari Ailus Reviewed-by: Mika Westerberg Reviewed-by: Andy Shevchenko --- drivers/acpi/property.c | 44 ++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 24e87b630573..16b28084c1ca 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -787,9 +787,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data, const union acpi_object *obj; int ret; - if (!val) - return -EINVAL; - if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) { ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj); if (ret) @@ -799,28 +796,43 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data, case DEV_PROP_U8: if (obj->integer.value > U8_MAX) return -EOVERFLOW; - *(u8 *)val = obj->integer.value; + + if (val) + *(u8 *)val = obj->integer.value; + break; case DEV_PROP_U16: if (obj->integer.value > U16_MAX) return -EOVERFLOW; - *(u16 *)val = obj->integer.value; + + if (val) + *(u16 *)val = obj->integer.value; + break; case DEV_PROP_U32: if (obj->integer.value > U32_MAX) return -EOVERFLOW; - *(u32 *)val = obj->integer.value; + + if (val) + *(u32 *)val = obj->integer.value; + break; default: - *(u64 *)val = obj->integer.value; + if (val) + *(u64 *)val = obj->integer.value; + break; } + + if (!val) + return 1; } else if (proptype == DEV_PROP_STRING) { ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj); if (ret) return ret; - *(char **)val = obj->string.pointer; + if (val) + *(char **)val = obj->string.pointer; return 1; } else { @@ -834,7 +846,7 @@ int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname, { int ret; - if (!adev) + if (!adev || !val) return -EINVAL; ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val); @@ -928,10 +940,20 @@ static int acpi_data_prop_read(const struct acpi_device_data *data, const union acpi_object *items; int ret; - if (val && nval == 1) { + if (nval == 1 || !val) { ret = acpi_data_prop_read_single(data, propname, proptype, val); - if (ret >= 0) + /* + * The overflow error means that the property is there and it is + * single-value, but its type does not match, so return. + */ + if (ret >= 0 || ret == -EOVERFLOW) return ret; + + /* + * Reading this property as a single-value one failed, but its + * value may still be represented as one-element array, so + * continue. + */ } ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj); From 38f3885edbef8a77b25c4d13f3de06a7b93d02de Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 12 Feb 2021 16:11:17 +0200 Subject: [PATCH 10/13] ACPI: property: Remove dead code After the commit 3a7a2ab839ad couple of functions became a dead code. Moreover, for all these years nobody used them. Remove. Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes") Signed-off-by: Andy Shevchenko Signed-off-by: Rafael J. Wysocki --- drivers/acpi/property.c | 20 -------------------- include/linux/acpi.h | 21 --------------------- 2 files changed, 41 deletions(-) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 16b28084c1ca..22ccab4e7c6d 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -841,20 +841,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data, return ret; } -int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname, - enum dev_prop_type proptype, void *val) -{ - int ret; - - if (!adev || !val) - return -EINVAL; - - ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val); - if (ret < 0 || proptype != ACPI_TYPE_STRING) - return ret; - return 0; -} - static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val, size_t nval) { @@ -995,12 +981,6 @@ static int acpi_data_prop_read(const struct acpi_device_data *data, return ret; } -int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname, - enum dev_prop_type proptype, void *val, size_t nval) -{ - return adev ? acpi_data_prop_read(&adev->data, propname, proptype, val, nval) : -EINVAL; -} - /** * acpi_node_prop_read - retrieve the value of an ACPI property with given name. * @fwnode: Firmware node to get the property from. diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 053bf05fb1f7..a47eaf448131 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1114,14 +1114,9 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid, int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname, void **valptr); -int acpi_dev_prop_read_single(struct acpi_device *adev, - const char *propname, enum dev_prop_type proptype, - void *val); int acpi_node_prop_read(const struct fwnode_handle *fwnode, const char *propname, enum dev_prop_type proptype, void *val, size_t nval); -int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname, - enum dev_prop_type proptype, void *val, size_t nval); struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode, struct fwnode_handle *child); @@ -1223,14 +1218,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode, return -ENXIO; } -static inline int acpi_dev_prop_read_single(const struct acpi_device *adev, - const char *propname, - enum dev_prop_type proptype, - void *val) -{ - return -ENXIO; -} - static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode, const char *propname, enum dev_prop_type proptype, @@ -1239,14 +1226,6 @@ static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode, return -ENXIO; } -static inline int acpi_dev_prop_read(const struct acpi_device *adev, - const char *propname, - enum dev_prop_type proptype, - void *val, size_t nval) -{ - return -ENXIO; -} - static inline struct fwnode_handle * acpi_get_next_subnode(const struct fwnode_handle *fwnode, struct fwnode_handle *child) From 325aa816143228a0b3472074ffb50d55ac3f04fe Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 12 Feb 2021 16:11:18 +0200 Subject: [PATCH 11/13] ACPI: property: Make acpi_node_prop_read() static There is no users outside of property.c. No need to export acpi_node_prop_read(), hence make it static. Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files") Signed-off-by: Andy Shevchenko Signed-off-by: Rafael J. Wysocki --- drivers/acpi/property.c | 6 +++--- include/linux/acpi.h | 11 ----------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 22ccab4e7c6d..2b65ad9b4c0d 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -993,9 +993,9 @@ static int acpi_data_prop_read(const struct acpi_device_data *data, * of the property. Otherwise, read at most @nval values to the array at the * location pointed to by @val. */ -int acpi_node_prop_read(const struct fwnode_handle *fwnode, - const char *propname, enum dev_prop_type proptype, - void *val, size_t nval) +static int acpi_node_prop_read(const struct fwnode_handle *fwnode, + const char *propname, enum dev_prop_type proptype, + void *val, size_t nval) { return acpi_data_prop_read(acpi_device_data_of_node(fwnode), propname, proptype, val, nval); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a47eaf448131..dc6e1f39dc6f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1114,9 +1114,6 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid, int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname, void **valptr); -int acpi_node_prop_read(const struct fwnode_handle *fwnode, - const char *propname, enum dev_prop_type proptype, - void *val, size_t nval); struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode, struct fwnode_handle *child); @@ -1218,14 +1215,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode, return -ENXIO; } -static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode, - const char *propname, - enum dev_prop_type proptype, - void *val, size_t nval) -{ - return -ENXIO; -} - static inline struct fwnode_handle * acpi_get_next_subnode(const struct fwnode_handle *fwnode, struct fwnode_handle *child) From c82ff99eaab83df6b962ce83521c456ba9cf44c2 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 12 Feb 2021 16:11:19 +0200 Subject: [PATCH 12/13] ACPI: property: Satisfy kernel doc validator (part 1) CHECK drivers/acpi/property.c warning: Function parameter or member 'data' not described in 'acpi_data_get_property_array' warning: Excess function parameter 'adev' description in 'acpi_data_get_property_array' Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes") Signed-off-by: Andy Shevchenko Signed-off-by: Rafael J. Wysocki --- drivers/acpi/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 2b65ad9b4c0d..ab4d7c734b0d 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -564,7 +564,7 @@ int acpi_node_prop_get(const struct fwnode_handle *fwnode, /** * acpi_data_get_property_array - return an ACPI array property with given name - * @adev: ACPI data object to get the property from + * @data: ACPI data object to get the property from * @name: Name of the property * @type: Expected type of array elements * @obj: Location to store a pointer to the property value (if not NULL) From 1de359d82576e57963f0d8b2d89cbdb2c9f4f2aa Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 12 Feb 2021 16:11:20 +0200 Subject: [PATCH 13/13] ACPI: property: Satisfy kernel doc validator (part 2) CHECK drivers/acpi/property.c warning: Function parameter or member '__fwnode' not described in 'acpi_graph_get_remote_endpoint' warning: Excess function parameter 'fwnode' description in 'acpi_graph_get_remote_endpoint' warning: Excess function parameter 'endpoint' description in 'acpi_graph_get_remote_endpoint' Fixes: 0ef7478639c5 ("ACPI: property: Make the ACPI graph API private") Signed-off-by: Andy Shevchenko Signed-off-by: Rafael J. Wysocki --- drivers/acpi/property.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index ab4d7c734b0d..e312ebaed8db 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -1212,8 +1212,7 @@ static struct fwnode_handle *acpi_graph_get_child_prop_value( /** * acpi_graph_get_remote_endpoint - Parses and returns remote end of an endpoint - * @fwnode: Endpoint firmware node pointing to a remote device - * @endpoint: Firmware node of remote endpoint is filled here if not %NULL + * @__fwnode: Endpoint firmware node pointing to a remote device * * Returns the remote endpoint corresponding to @__fwnode. NULL on error. */