This patch set enables custom ACPI board configuration by adding
mechanisms in the Linux kernel for loading user defined SSDTs.
In order to support ACPI open-ended hardware configurations we need a
way to augment the ACPI configuration provided by the firmware
image. A common example is connecting sensors on I2C / SPI buses on
development boards.
Although this can be accomplished by creating a kernel platform driver
or recompiling the firmware image with updated ACPI tables, neither is
practical: the former proliferates board specific kernel code while
the latter requires access to firmware tools which are often not
publicly available.
Because ACPI supports external references in AML code a more practical
way to augment firmware ACPI configuration is by dynamically loading
user defined SSDT tables that contain the board specific information.
This patch sets provides three methods for loading custom SSDTs:
* From an EFI variable
This is the preferred method, when EFI is supported on the platform,
because it allows a persistent, OS independent way of storing and
updating the user defined SSDTs. There is also work underway to
implement EFI support for loading user defined SSDTs and using this
method will make it easier to convert to the EFI loading mechanism
when that will arrive.
* From the first uncompressed initrd (similar with the override
functionality)
This is useful when EFI is not supported on the platform and when it
is not possible to defer the loading to userspace.
* From userspace via configfs
This is useful when we want to defer the operation to userspace for
platform detection, loading the SSDTs from a custom partition, etc.
Octavian Purdila (10):
kernel: add TAINT_OVERLAY_ACPI_TABLE
acpi: install SSDT tables from initrd
acpi: add support for ACPI reconfiguration notifiers
acpi: fix enumeration (visited) flags for bus rescans
i2c: add support for ACPI reconfigure notifications
spi: add support for ACPI reconfigure notifications
efi: load SSTDs from EFI variables
configfs: fix CONFIGFS_BIN_ATTR_[RW]O definitions
acpi: add support for configfs
acpi: add support for loading SSDTs via configfs
Documentation/ABI/testing/configfs-acpi | 23 +++++
Documentation/acpi/ssdt-overlays.txt | 174 ++++++++++++++++++++++++++++++++
Documentation/kernel-parameters.txt | 7 ++
Documentation/oops-tracing.txt | 2 +
Documentation/sysctl/kernel.txt | 1 +
MAINTAINERS | 1 +
drivers/acpi/Kconfig | 9 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/bus.c | 72 +++++++++++++
drivers/acpi/configfs.c | 143 ++++++++++++++++++++++++++
drivers/acpi/internal.h | 3 +
drivers/acpi/scan.c | 73 +++++++++++++-
drivers/acpi/sysfs.c | 6 +-
drivers/firmware/efi/efi.c | 107 ++++++++++++++++++++
drivers/i2c/i2c-core.c | 38 ++++++-
drivers/spi/spi.c | 36 ++++++-
include/acpi/acpi_bus.h | 8 ++
include/linux/configfs.h | 4 +-
include/linux/kernel.h | 1 +
kernel/panic.c | 2 +
20 files changed, 699 insertions(+), 12 deletions(-)
create mode 100644 Documentation/ABI/testing/configfs-acpi
create mode 100644 Documentation/acpi/ssdt-overlays.txt
create mode 100644 drivers/acpi/configfs.c
--
1.9.1
Add a new tain flag that indicates wheather the user has loaded ACPI
SSDT overlays. This will provide a clean indication in bug reports that
the user has added new information to the ACPI tables.
Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/oops-tracing.txt | 2 ++
Documentation/sysctl/kernel.txt | 1 +
include/linux/kernel.h | 1 +
kernel/panic.c | 2 ++
4 files changed, 6 insertions(+)
diff --git a/Documentation/oops-tracing.txt b/Documentation/oops-tracing.txt
index f3ac05c..40e1117 100644
--- a/Documentation/oops-tracing.txt
+++ b/Documentation/oops-tracing.txt
@@ -272,6 +272,8 @@ characters, each representing a particular tainted value.
16: 'K' if the kernel has been live patched.
+ 17: 'N' if ACPI SSDT overlays have been loaded.
+
The primary reason for the 'Tainted: ' string is to tell kernel
debuggers if this is a clean kernel or if anything unusual has
occurred. Tainting is permanent: even if an offending module is
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index a93b414..547173e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -895,6 +895,7 @@ can be ORed together:
signature.
16384 - A soft lockup has previously occurred on the system.
32768 - The kernel has been live patched.
+65536 - ACPI SSDT overlays have been loaded.
==============================================================
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c..18ff9c7 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -519,6 +519,7 @@ extern enum system_states {
#define TAINT_UNSIGNED_MODULE 13
#define TAINT_SOFTLOCKUP 14
#define TAINT_LIVEPATCH 15
+#define TAINT_OVERLAY_ACPI_TABLE 16
extern const char hex_asc[];
#define hex_asc_lo(x) hex_asc[((x) & 0x0f)]
diff --git a/kernel/panic.c b/kernel/panic.c
index d96469d..bf4b6d0 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -263,6 +263,7 @@ static const struct tnt tnts[] = {
{ TAINT_UNSIGNED_MODULE, 'E', ' ' },
{ TAINT_SOFTLOCKUP, 'L', ' ' },
{ TAINT_LIVEPATCH, 'K', ' ' },
+ { TAINT_OVERLAY_ACPI_TABLE, 'N', ' ' },
};
/**
@@ -284,6 +285,7 @@ static const struct tnt tnts[] = {
* 'E' - Unsigned module has been loaded.
* 'L' - A soft lockup has previously occurred.
* 'K' - Kernel has been live patched.
+ * 'N' - ACPI SSDT overlays have been loaded.
*
* The string is overwritten by the next call to print_tainted().
*/
--
1.9.1
If the ACPI tables changes as a result of a dinamically loaded table and
a bus rescan is required the enumeration/visited flag are not
consistent.
I2C/SPI are not directly enumerated in acpi_bus_attach(), however the
visited flag is set. This makes it impossible to check if an ACPI device
has already been enumerated by the I2C and SPI subsystems. To fix this
issue we only set the visited flags if the device is not I2C or SPI.
With this change we also need to remove setting visited to false from
acpi_bus_attach(), otherwise if we rescan already enumerated I2C/SPI
devices we try to re-enumerate them.
Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/acpi/scan.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a7e476c..8f3fd37 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1670,7 +1670,7 @@ static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
return -1;
}
-static void acpi_default_enumeration(struct acpi_device *device)
+static bool acpi_default_enumeration(struct acpi_device *device)
{
struct list_head resource_list;
bool is_spi_i2c_slave = false;
@@ -1685,6 +1685,7 @@ static void acpi_default_enumeration(struct acpi_device *device)
acpi_dev_free_resource_list(&resource_list);
if (!is_spi_i2c_slave)
acpi_create_platform_device(device);
+ return !is_spi_i2c_slave;
}
static const struct acpi_device_id generic_device_ids[] = {
@@ -1744,6 +1745,7 @@ static void acpi_bus_attach(struct acpi_device *device)
struct acpi_device *child;
acpi_handle ejd;
int ret;
+ bool enumerated = true;
if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
register_dock_dependent_device(device, ejd);
@@ -1766,7 +1768,7 @@ static void acpi_bus_attach(struct acpi_device *device)
device->flags.initialized = true;
}
- device->flags.visited = false;
+
ret = acpi_scan_attach_handler(device);
if (ret < 0)
return;
@@ -1778,9 +1780,10 @@ static void acpi_bus_attach(struct acpi_device *device)
return;
if (!ret && device->pnp.type.platform_id)
- acpi_default_enumeration(device);
+ enumerated = acpi_default_enumeration(device);
}
- device->flags.visited = true;
+ if (enumerated)
+ device->flags.visited = true;
ok:
list_for_each_entry(child, &device->children, node)
--
1.9.1
Add support for ACPI reconfiguration notifiers to allow subsystems to
react to changes in the ACPI tables that happen after the initial
enumeration. This is similar with the way dynamic device tree
notifications work.
Right now only two reconfigure notifications are supported: table load
and table unload operations. This could be extended with finer grained
notification like ACPI device added or removed in the future.
Since ACPICA allows only one table notification handler, this patch
makes the table notifier function generic and moves it out of the
sysfs specific code.
Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/acpi/bus.c | 9 +++++++
drivers/acpi/internal.h | 3 +++
drivers/acpi/scan.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/sysfs.c | 6 ++---
include/acpi/acpi_bus.h | 8 +++++++
5 files changed, 84 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 5e0d076..09134ed 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1037,6 +1037,13 @@ void __init acpi_subsystem_init(void)
}
}
+static acpi_status acpi_bus_table_handler(u32 event, void *table, void *context)
+{
+ acpi_scan_table_handler(event, table, context);
+
+ return acpi_sysfs_table_handler(event, table, context);
+}
+
static int __init acpi_bus_init(void)
{
int result;
@@ -1078,6 +1085,8 @@ static int __init acpi_bus_init(void)
* _PDC control method may load dynamic SSDT tables,
* and we need to install the table handler before that.
*/
+ status = acpi_install_table_handler(acpi_bus_table_handler, NULL);
+
acpi_sysfs_init();
acpi_early_processor_set_pdc();
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 1e6833a..cd4724f 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -80,6 +80,9 @@ bool acpi_queue_hotplug_work(struct work_struct *work);
void acpi_device_hotplug(struct acpi_device *adev, u32 src);
bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent);
+acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context);
+void acpi_scan_table_handler(u32 event, void *table, void *context);
+
/* --------------------------------------------------------------------------
Device Node Initialization / Removal
-------------------------------------------------------------------------- */
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 407a376..a7e476c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1916,6 +1916,8 @@ static int acpi_bus_scan_fixed(void)
return result < 0 ? result : 0;
}
+static bool acpi_scan_initialized;
+
int __init acpi_scan_init(void)
{
int result;
@@ -1959,6 +1961,8 @@ int __init acpi_scan_init(void)
acpi_update_all_gpes();
+ acpi_scan_initialized = true;
+
out:
mutex_unlock(&acpi_scan_lock);
return result;
@@ -2002,3 +2006,61 @@ int __init __acpi_probe_device_table(struct acpi_probe_entry *ap_head, int nr)
return count;
}
+
+static BLOCKING_NOTIFIER_HEAD(acpi_reconfig_chain);
+
+struct acpi_table_events_work {
+ struct work_struct work;
+ void *table;
+ u32 event;
+};
+
+void acpi_table_events_fn(struct work_struct *work)
+{
+ struct acpi_table_events_work *tew;
+ enum acpi_reconfig_event event;
+
+ tew = container_of(work, struct acpi_table_events_work, work);
+
+ acpi_scan_lock_acquire();
+ acpi_bus_scan(ACPI_ROOT_OBJECT);
+ acpi_scan_lock_release();
+
+ if (tew->event == ACPI_TABLE_EVENT_LOAD)
+ event = ACPI_RECONFIG_TABLE_LOAD;
+ else
+ event = ACPI_RECONFIG_TABLE_UNLOAD;
+
+ blocking_notifier_call_chain(&acpi_reconfig_chain, event, tew->table);
+ kfree(tew);
+}
+
+void acpi_scan_table_handler(u32 event, void *table, void *context)
+{
+ struct acpi_table_events_work *tew;
+
+ if (!acpi_scan_initialized)
+ return;
+
+ tew = kmalloc(sizeof(*tew), GFP_KERNEL);
+ if (!tew)
+ return;
+
+ INIT_WORK(&tew->work, acpi_table_events_fn);
+ tew->table = table;
+ tew->event = event;
+
+ schedule_work(&tew->work);
+}
+
+int acpi_reconfig_notifier_register(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&acpi_reconfig_chain, nb);
+}
+EXPORT_SYMBOL(acpi_reconfig_notifier_register);
+
+int acpi_reconfig_notifier_unregister(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&acpi_reconfig_chain, nb);
+}
+EXPORT_SYMBOL(acpi_reconfig_notifier_unregister);
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 0243d37..d132d66 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -378,8 +378,7 @@ static void acpi_table_attr_init(struct acpi_table_attr *table_attr,
return;
}
-static acpi_status
-acpi_sysfs_table_handler(u32 event, void *table, void *context)
+acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
{
struct acpi_table_attr *table_attr;
@@ -452,9 +451,8 @@ static int acpi_tables_sysfs_init(void)
kobject_uevent(tables_kobj, KOBJ_ADD);
kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
- status = acpi_install_table_handler(acpi_sysfs_table_handler, NULL);
- return ACPI_FAILURE(status) ? -EINVAL : 0;
+ return 0;
err_dynamic_tables:
kobject_put(tables_kobj);
err:
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 14362a8..2faf183 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -518,6 +518,14 @@ int acpi_match_device_ids(struct acpi_device *device,
int acpi_create_dir(struct acpi_device *);
void acpi_remove_dir(struct acpi_device *);
+enum acpi_reconfig_event {
+ ACPI_RECONFIG_TABLE_LOAD = 0,
+ ACPI_RECONFIG_TABLE_UNLOAD,
+};
+
+int acpi_reconfig_notifier_register(struct notifier_block *nb);
+int acpi_reconfig_notifier_unregister(struct notifier_block *nb);
+
static inline bool acpi_device_enumerated(struct acpi_device *adev)
{
return adev && adev->flags.initialized && adev->flags.visited;
--
1.9.1
This allows the SPI core to enumerate devices from ACPI tables that
are dynamically loaded after the SPI master device has been probed.
Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/spi/spi.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 47eff80..8751f02 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1619,7 +1619,8 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
if (acpi_bus_get_device(handle, &adev))
return AE_OK;
- if (acpi_bus_get_status(adev) || !adev->status.present)
+ if (acpi_bus_get_status(adev) || !adev->status.present ||
+ acpi_device_enumerated(adev))
return AE_OK;
spi = spi_alloc_device(master);
@@ -1645,6 +1646,8 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
if (spi->irq < 0)
spi->irq = acpi_dev_gpio_irq_get(adev, 0);
+ adev->flags.visited = true;
+
adev->power.flags.ignore_parent = true;
strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
if (spi_add_device(spi)) {
@@ -2698,6 +2701,35 @@ static struct notifier_block spi_of_notifier = {
extern struct notifier_block spi_of_notifier;
#endif /* IS_ENABLED(CONFIG_OF_DYNAMIC) */
+#if IS_ENABLED(CONFIG_ACPI)
+static int acpi_spi_table_load(struct device *dev, const void *data)
+{
+ struct spi_master *master = container_of(dev, struct spi_master, dev);
+
+ acpi_register_spi_devices(master);
+ return 0;
+}
+
+static int acpi_spi_notify(struct notifier_block *nb, unsigned long value,
+ void *arg)
+{
+ switch (value) {
+ case ACPI_RECONFIG_TABLE_LOAD:
+ class_find_device(&spi_master_class, NULL, NULL,
+ acpi_spi_table_load);
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block spi_acpi_notifier = {
+ .notifier_call = acpi_spi_notify,
+};
+#else
+extern struct notifier_block spi_acpi_notifier;
+#endif
+
static int __init spi_init(void)
{
int status;
@@ -2718,6 +2750,8 @@ static int __init spi_init(void)
if (IS_ENABLED(CONFIG_OF_DYNAMIC))
WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
+ if (IS_ENABLED(CONFIG_ACPI))
+ WARN_ON(acpi_reconfig_notifier_register(&spi_acpi_notifier));
return 0;
--
1.9.1
The type should be struct configfs_bin_attribute and not struct
configfs_attribute.
Signed-off-by: Octavian Purdila <[email protected]>
---
include/linux/configfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index f8165c1..ada0613 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -181,7 +181,7 @@ static struct configfs_bin_attribute _pfx##attr_##_name = { \
}
#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_attribute _pfx##attr_##_name = { \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
.cb_attr = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IRUGO, \
@@ -193,7 +193,7 @@ static struct configfs_attribute _pfx##attr_##_name = { \
}
#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_attribute _pfx##attr_##_name = { \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
.cb_attr = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IWUSR, \
--
1.9.1
Add support for acpi_user_table configfs items that allows the user to
load new tables. The data attributes contains the table data and once it
is filled from userspace the table is loaded and ACPI devices are
enumerated.
Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/ABI/testing/configfs-acpi | 16 +++++
Documentation/acpi/ssdt-overlays.txt | 14 +++++
drivers/acpi/configfs.c | 104 ++++++++++++++++++++++++++++++++
3 files changed, 134 insertions(+)
diff --git a/Documentation/ABI/testing/configfs-acpi b/Documentation/ABI/testing/configfs-acpi
index 0c806aa..34a205e 100644
--- a/Documentation/ABI/testing/configfs-acpi
+++ b/Documentation/ABI/testing/configfs-acpi
@@ -5,3 +5,19 @@ Contact: [email protected]
Description:
This represents the ACPI subsystem entry point directory. It
contains sub-groups corresponding to ACPI configurable options.
+
+What: /config/acpi/table
+Date: April 2016
+KernelVersion: 4.6
+Description:
+
+ This group contains the configuration for user defined ACPI
+ tables. The attributes of a user define table are:
+
+ data - a binary write only attribute that the user can use to
+ fill in the ACPI aml definitions. Once the aml data is
+ written to this file and the file is closed the table
+ will be loaded and ACPI device will be enumerated. To
+ check if the operation is successful the user must check
+ the error code for close(). If the operation is
+ successful, subsequent writes to this attribute will fail.
diff --git a/Documentation/acpi/ssdt-overlays.txt b/Documentation/acpi/ssdt-overlays.txt
index 7c588be..a88a2ce 100644
--- a/Documentation/acpi/ssdt-overlays.txt
+++ b/Documentation/acpi/ssdt-overlays.txt
@@ -158,3 +158,17 @@ tmp=$(mktemp)
/bin/echo -ne "\007\000\000\000" | cat - $filename > $tmp
dd if=$tmp of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp)
rm $tmp
+
+== Loading ACPI SSDTs from configfs ==
+
+This option allows loading of user defined SSDTs from userspace via the configfs
+interface. The CONFIG_ACPI_CONFIGFS option must be select and configfs must be
+mounted. In the following examples, we assume that configfs has been mounted in
+/config.
+
+New tables can be loading by creating new directories in /config/acpi/table/ and
+writing the SSDT aml code in the data attribute:
+
+cd /config/acpi/table
+mkdir my_ssdt
+cat ~/ssdt.aml > my_ssdt/data
diff --git a/drivers/acpi/configfs.c b/drivers/acpi/configfs.c
index 96aa3d8..3a194806 100644
--- a/drivers/acpi/configfs.c
+++ b/drivers/acpi/configfs.c
@@ -13,6 +13,104 @@
#include <linux/configfs.h>
#include <linux/acpi.h>
+static struct config_group *acpi_table_group;
+
+struct acpi_user_table {
+ struct config_item cfg;
+ struct acpi_table_header *table;
+};
+
+static ssize_t acpi_table_data_write(struct config_item *cfg,
+ const void *data, size_t size)
+{
+ struct acpi_table_header *header = (struct acpi_table_header *)data;
+ struct acpi_user_table *table;
+ int ret;
+
+ table = container_of(cfg, struct acpi_user_table, cfg);
+
+ if (table->table) {
+ pr_err("ACPI configfs table: table already loaded\n");
+ return -EBUSY;
+ }
+
+ if (header->length != size) {
+ pr_err("ACPI configfs table: invalid table length\n");
+ return -EINVAL;
+ }
+
+ if (memcmp(header->signature, ACPI_SIG_SSDT, 4)) {
+ pr_err("ACPI configfs table: invalid table signature\n");
+ return -EINVAL;
+ }
+
+ table = container_of(cfg, struct acpi_user_table, cfg);
+
+ table->table = kmemdup(header, header->length, GFP_KERNEL);
+ if (!table->table)
+ return -ENOMEM;
+
+ ret = acpi_load_table(table->table);
+ if (ret) {
+ kfree(table->table);
+ table->table = NULL;
+ }
+
+ add_taint(TAINT_OVERLAY_ACPI_TABLE, LOCKDEP_STILL_OK);
+
+ return ret;
+}
+
+#define MAX_ACPI_TABLE_SIZE (128 * 1024)
+
+CONFIGFS_BIN_ATTR_WO(acpi_table_, data, NULL, MAX_ACPI_TABLE_SIZE);
+
+struct configfs_bin_attribute *acpi_table_bin_attrs[] = {
+ &acpi_table_attr_data,
+ NULL,
+};
+
+static struct config_item_type acpi_table_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_bin_attrs = acpi_table_bin_attrs,
+};
+
+static struct config_item *acpi_table_make_item(struct config_group *group,
+ const char *name)
+{
+ struct acpi_user_table *table;
+
+ table = kzalloc(sizeof(*table), GFP_KERNEL);
+ if (!table)
+ return ERR_PTR(-ENOMEM);
+
+ config_item_init_type_name(&table->cfg, name, &acpi_table_type);
+ return &table->cfg;
+}
+
+struct configfs_group_operations acpi_table_group_ops = {
+ .make_item = acpi_table_make_item,
+};
+
+static struct config_item_type acpi_tables_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_group_ops = &acpi_table_group_ops,
+};
+
+static struct config_item_type acpi_root_group_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem acpi_configfs = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = "acpi",
+ .ci_type = &acpi_root_group_type,
+ },
+ },
+ .su_mutex = __MUTEX_INITIALIZER(acpi_configfs.su_mutex),
+};
+
static int __init acpi_configfs_init(void)
{
int ret;
@@ -24,12 +122,18 @@ static int __init acpi_configfs_init(void)
if (ret)
return ret;
+ acpi_table_group = configfs_register_default_group(root, "table",
+ &acpi_tables_type);
+ if (IS_ERR(acpi_table_group))
+ return PTR_ERR(acpi_table_group);
+
return 0;
}
module_init(acpi_configfs_init);
static void __exit acpi_configfs_exit(void)
{
+ configfs_unregister_default_group(acpi_table_group);
configfs_unregister_subsystem(&acpi_configfs);
}
module_exit(acpi_configfs_exit);
--
1.9.1
Register the ACPI subsystem with configfs.
Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/ABI/testing/configfs-acpi | 7 ++++++
MAINTAINERS | 1 +
drivers/acpi/Kconfig | 9 ++++++++
drivers/acpi/Makefile | 1 +
drivers/acpi/configfs.c | 39 +++++++++++++++++++++++++++++++++
5 files changed, 57 insertions(+)
create mode 100644 Documentation/ABI/testing/configfs-acpi
create mode 100644 drivers/acpi/configfs.c
diff --git a/Documentation/ABI/testing/configfs-acpi b/Documentation/ABI/testing/configfs-acpi
new file mode 100644
index 0000000..0c806aa
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-acpi
@@ -0,0 +1,7 @@
+What: /config/acpi
+Date: April 2016
+KernelVersion: 4.6
+Contact: [email protected]
+Description:
+ This represents the ACPI subsystem entry point directory. It
+ contains sub-groups corresponding to ACPI configurable options.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6ee06ea..2f1538d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -283,6 +283,7 @@ F: include/linux/acpi.h
F: include/acpi/
F: Documentation/acpi/
F: Documentation/ABI/testing/sysfs-bus-acpi
+F: Documentation/ABI/testing/configfs-acpi
F: drivers/pci/*acpi*
F: drivers/pci/*/*acpi*
F: drivers/pci/*/*/*acpi*
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 82b96ee..7d02816 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -521,4 +521,13 @@ config XPOWER_PMIC_OPREGION
endif
+config ACPI_CONFIGFS
+ tristate "ACPI configfs support"
+ select CONFIGFS_FS
+ default n
+ help
+ Select this option to enable support for ACPI configuration from
+ userspace. The configurable ACPI groups will be visible under
+ /config/acpi, assuming configfs is mounted under /config.
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index cb648a4..50ceee1 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -97,5 +97,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
+obj-$(CONFIG_ACPI_CONFIGFS) += configfs.o
video-objs += acpi_video.o video_detect.o
diff --git a/drivers/acpi/configfs.c b/drivers/acpi/configfs.c
new file mode 100644
index 0000000..96aa3d8
--- /dev/null
+++ b/drivers/acpi/configfs.c
@@ -0,0 +1,39 @@
+/*
+ * ACPI configfs support
+ *
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/configfs.h>
+#include <linux/acpi.h>
+
+static int __init acpi_configfs_init(void)
+{
+ int ret;
+ struct config_group *root = &acpi_configfs.su_group;
+
+ config_group_init(root);
+
+ ret = configfs_register_subsystem(&acpi_configfs);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+module_init(acpi_configfs_init);
+
+static void __exit acpi_configfs_exit(void)
+{
+ configfs_unregister_subsystem(&acpi_configfs);
+}
+module_exit(acpi_configfs_exit);
+
+MODULE_AUTHOR("Octavian Purdila <[email protected]>");
+MODULE_DESCRIPTION("ACPI configfs support");
+MODULE_LICENSE("GPL v2");
--
1.9.1
This patch allows SSDTs to be loaded from EFI variables. It works by
specifying the EFI variable name containing the SSDT to be loaded. All
variables with the same name (regardless of the vendor GUID) will be
loaded.
Note that we can't use acpi_install_table and we must rely on the
dynamic ACPI table loading and bus re-scanning mechanisms. That is
because I2C/SPI controllers are initialized earlier then the EFI
subsystems and all I2C/SPI ACPI devices are enumerated when the
I2C/SPI controllers are initialized.
Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/acpi/ssdt-overlays.txt | 66 +++++++++++++++++++++
Documentation/kernel-parameters.txt | 7 +++
drivers/firmware/efi/efi.c | 107 +++++++++++++++++++++++++++++++++++
3 files changed, 180 insertions(+)
diff --git a/Documentation/acpi/ssdt-overlays.txt b/Documentation/acpi/ssdt-overlays.txt
index a94c3f9..7c588be 100644
--- a/Documentation/acpi/ssdt-overlays.txt
+++ b/Documentation/acpi/ssdt-overlays.txt
@@ -92,3 +92,69 @@ cp ssdt.aml kernel/firmware/acpi
find kernel | cpio -H newc --create > /boot/instrumented_initrd
cat /boot/initrd >>/boot/instrumented_initrd
+== Loading ACPI SSDTs from EFI variables ==
+
+This is the preferred method, when EFI is supported on the platform, because it
+allows a persistent, OS independent way of storing the user defined SSDTs. There
+is also work underway to implement EFI support for loading user defined SSDTs
+and using this method will make it easier to convert to the EFI loading
+mechanism when that will arrive.
+
+In order to load SSDTs from an EFI variable the efivar_ssdt kernel command line
+parameter can be used. The argument for the option is the variable name to
+use. If there are multiple variables with the same name but with different
+vendor GUIDs, all of them will be loaded.
+
+In order to store the AML code in an EFI variable the efivarfs filesystem can be
+used. It is enabled and mounted by default in /sys/firmware/efi/efivars in all
+recent distribution.
+
+Creating a new file in /sys/firmware/efi/efivars will automatically create a new
+EFI variable. Updating a file in /sys/firmware/efi/efivars will update the EFI
+variable. Please note that the file name needs to be specially formatted as
+"Name-GUID" and that the first 4 bytes in the file (little-endian format)
+represent the attributes of the EFI variable (see EFI_VARIABLE_MASK in
+include/linux/efi.h). Writing to the file must also be done with one write
+operation.
+
+For example, you can use the following bash script to create/update an EFI
+variable with the content from a given file:
+
+#!/bin/sh -e
+
+while ! [ -z "$1" ]; do
+ case "$1" in
+ "-f") filename="$2"; shift;;
+ "-g") guid="$2"; shift;;
+ *) name="$1";;
+ esac
+ shift
+done
+
+if [ -z "$name" ] || [ -z "$filename" ]; then
+ echo "Syntax: ${0##*/} -f filename [ -g guid ] name"
+ exit 1
+fi
+
+EFIVARFS='/sys/firmware/efi/efivars'
+
+[ -d "$EFIVARFS" ] || exit 2
+
+if stat -tf $EFIVARFS | grep -q -v de5e81e4; then
+ mount -t efivarfs none $EFIVARFS
+fi
+
+# try to pick up an existing GUID
+if [ -z "$guid" ]; then
+ guid=$(find "$EFIVARFS" -name "$name-*" | head -n1 | cut -f2- -d-)
+fi
+
+# use a randomly generated GUID
+if [ -z "$guid" ]; then
+ guid="$(cat /proc/sys/kernel/random/uuid)"
+fi
+
+tmp=$(mktemp)
+/bin/echo -ne "\007\000\000\000" | cat - $filename > $tmp
+dd if=$tmp of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp)
+rm $tmp
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 9a53c92..fe89cda 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1144,6 +1144,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Address Range Mirroring feature even if your box
doesn't support it.
+ efivar_ssdt= [EFI; X86] Name of an EFI variable that contains an SSDT
+ that is to be dynamically loaded by Linux. If there are
+ multiple variables with the same name but with different
+ vendor GUIDs, all of them will be loaded. See
+ Documentation/acpi/ssdt-overlays.txt for details.
+
+
eisa_irq_edge= [PARISC,HW]
See header of drivers/parisc/eisa.c.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 2cd37da..dda4778 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -24,6 +24,8 @@
#include <linux/of_fdt.h>
#include <linux/io.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/acpi.h>
#include <asm/early_ioremap.h>
@@ -193,6 +195,108 @@ static void generic_ops_unregister(void)
efivars_unregister(&generic_efivars);
}
+#if IS_ENABLED(CONFIG_ACPI)
+#define EFIVAR_SSDT_NAME_MAX 16
+static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX];
+static int __init efivar_ssdt_setup(char *str)
+{
+ if (strlen(str) < sizeof(efivar_ssdt))
+ memcpy(efivar_ssdt, str, strlen(str));
+ else
+ pr_warn("efivar_ssdt: name too long: %s\n", str);
+ return 0;
+}
+__setup("efivar_ssdt=", efivar_ssdt_setup);
+
+static LIST_HEAD(efivar_ssdts);
+
+static inline void pr_efivar_name(efi_char16_t *name16)
+{
+ char name[EFIVAR_SSDT_NAME_MAX];
+ int i;
+
+ for (i = 0; i < EFIVAR_SSDT_NAME_MAX - 1; i++)
+ name[i] = name16[i] & 0xFF;
+ name[i] = 0;
+ pr_cont("%s", name);
+}
+
+static __init int efivar_acpi_iter(efi_char16_t *name, efi_guid_t vendor,
+ unsigned long name_size, void *data)
+{
+ int i;
+ int str_len = name_size / sizeof(efi_char16_t);
+ struct efivar_entry *entry;
+
+ if (str_len != strlen(efivar_ssdt) + 1)
+ return 0;
+
+ for (i = 0; i < str_len; i++)
+ if ((name[i] & 0xFF) != efivar_ssdt[i])
+ return 0;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ memcpy(entry->var.VariableName, name, name_size);
+ memcpy(&entry->var.VendorGuid, &vendor, sizeof(efi_guid_t));
+
+ efivar_entry_add(entry, &efivar_ssdts);
+
+ return 0;
+}
+
+static __init int efivar_ssdt_load(void)
+{
+ struct efivar_entry *i;
+ int err;
+
+ err = efivar_init(efivar_acpi_iter, NULL, false, false,
+ &efivar_ssdts);
+ if (err) {
+ pr_err("%s: efivar_init failed: %d\n", __func__, err);
+ return err;
+ }
+
+ list_for_each_entry(i, &efivar_ssdts, list) {
+ void *data;
+ unsigned long size;
+
+ pr_info("loading SSDT from EFI variable ");
+ pr_efivar_name(i->var.VariableName); pr_cont("\n");
+
+ err = efivar_entry_size(i, &size);
+ if (err) {
+ pr_err("failed to get size\n");
+ continue;
+ }
+
+ data = kmalloc(size, GFP_KERNEL);
+ if (!data)
+ continue;
+
+ err = efivar_entry_get(i, NULL, &size, data);
+ if (err) {
+ pr_err("failed to get data\n");
+ kfree(data);
+ continue;
+ }
+
+ err = acpi_load_table(data);
+ if (err) {
+ pr_err("failed to load table: %d\n", err);
+ kfree(data);
+ continue;
+ }
+
+ add_taint(TAINT_OVERLAY_ACPI_TABLE, LOCKDEP_STILL_OK);
+ }
+
+ return 0;
+}
+#endif
+
/*
* We register the efi subsystem with the firmware subsystem and the
* efivars subsystem with the efi subsystem, if the system was booted with
@@ -216,6 +320,9 @@ static int __init efisubsys_init(void)
if (error)
goto err_put;
+ if (IS_ENABLED(CONFIG_ACPI))
+ efivar_ssdt_load();
+
error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
if (error) {
pr_err("efi: Sysfs attribute export failed with error %d.\n",
--
1.9.1
This allows the i2c core to enumerate devices from ACPI tables that
are dynamically loaded after the i2c adapter has been probed.
Signed-off-by: Octavian Purdila <[email protected]>
---
drivers/i2c/i2c-core.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index ffe715d..7a6f741 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -150,7 +150,8 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
if (acpi_bus_get_device(handle, &adev))
return AE_OK;
- if (acpi_bus_get_status(adev) || !adev->status.present)
+ if (acpi_bus_get_status(adev) || !adev->status.present ||
+ acpi_device_enumerated(adev))
return AE_OK;
memset(&info, 0, sizeof(info));
@@ -189,6 +190,9 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
adev->power.flags.ignore_parent = true;
strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+
+ adev->flags.visited = true;
+
if (!i2c_new_device(adapter, &info)) {
adev->power.flags.ignore_parent = false;
dev_err(&adapter->dev,
@@ -224,8 +228,36 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
}
+static int acpi_i2c_table_load(struct device *dev, void *data)
+{
+ struct i2c_adapter *adapter = i2c_verify_adapter(dev);
+
+ if (!adapter)
+ return 0;
+
+ acpi_i2c_register_devices(adapter);
+ return 0;
+}
+
+static int acpi_i2c_notify(struct notifier_block *nb, unsigned long value,
+ void *arg)
+{
+ switch (value) {
+ case ACPI_RECONFIG_TABLE_LOAD:
+ bus_find_device(&i2c_bus_type, NULL, NULL,
+ acpi_i2c_table_load);
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block i2c_acpi_notifier = {
+ .notifier_call = acpi_i2c_notify,
+};
#else /* CONFIG_ACPI */
static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) { }
+extern struct notifier_block i2c_acpi_notifier;
#endif /* CONFIG_ACPI */
#ifdef CONFIG_ACPI_I2C_OPREGION
@@ -2117,6 +2149,8 @@ static int __init i2c_init(void)
if (IS_ENABLED(CONFIG_OF_DYNAMIC))
WARN_ON(of_reconfig_notifier_register(&i2c_of_notifier));
+ if (IS_ENABLED(CONFIG_ACPI))
+ WARN_ON(acpi_reconfig_notifier_register(&i2c_acpi_notifier));
return 0;
@@ -2131,6 +2165,8 @@ bus_err:
static void __exit i2c_exit(void)
{
+ if (IS_ENABLED(CONFIG_ACPI))
+ WARN_ON(acpi_reconfig_notifier_unregister(&i2c_acpi_notifier));
if (IS_ENABLED(CONFIG_OF_DYNAMIC))
WARN_ON(of_reconfig_notifier_unregister(&i2c_of_notifier));
i2c_del_driver(&dummy_driver);
--
1.9.1
This patch allows loading user defined SSDTs from the first,
uncompressed, initrd. The SSDT aml code must be stored in files under
the /kernel/firmware/acpi/overlay path.
Signed-off-by: Octavian Purdila <[email protected]>
---
Documentation/acpi/ssdt-overlays.txt | 94 ++++++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 63 ++++++++++++++++++++++++
2 files changed, 157 insertions(+)
create mode 100644 Documentation/acpi/ssdt-overlays.txt
diff --git a/Documentation/acpi/ssdt-overlays.txt b/Documentation/acpi/ssdt-overlays.txt
new file mode 100644
index 0000000..a94c3f9
--- /dev/null
+++ b/Documentation/acpi/ssdt-overlays.txt
@@ -0,0 +1,94 @@
+
+In order to support ACPI open-ended hardware configurations (e.g. development
+boards) we need a way to augment the ACPI configuration provided by the firmware
+image. A common example is connecting sensors on I2C / SPI buses on development
+boards.
+
+Although this can be accomplished by creating a kernel platform driver or
+recompiling the firmware image with updated ACPI tables, neither is practical:
+the former proliferates board specific kernel code while the latter requires
+access to firmware tools which are often not publicly available.
+
+Because ACPI supports external references in AML code a more practical
+way to augment firmware ACPI configuration is by dynamically loading
+user defined SSDT tables that contain the board specific information.
+
+For example, to enumerate a Bosch BMA222E accelerometer on the I2C bus of the
+Minnowboard MAX development board exposed via the LSE connector [1], the
+following ASL code can be used:
+
+DefinitionBlock ("minnowmax.aml", "SSDT", 1, "Vendor", "Accel", 0x00000003)
+{
+ External (\_SB.I2C6, DeviceObj)
+
+ Scope (\_SB.I2C6)
+ {
+ Device (STAC)
+ {
+ Name (_ADR, Zero)
+ Name (_HID, "BMA222E")
+
+ Method (_CRS, 0, Serialized)
+ {
+ Name (RBUF, ResourceTemplate ()
+ {
+ I2cSerialBus (0x0018, ControllerInitiated, 0x00061A80,
+ AddressingMode7Bit, "\\_SB.I2C6", 0x00,
+ ResourceConsumer, ,)
+ GpioInt (Edge, ActiveHigh, Exclusive, PullDown, 0x0000,
+ "\\_SB.GPO2", 0x00, ResourceConsumer, , )
+ { // Pin list
+ 0
+ }
+ })
+ Return (RBUF)
+ }
+ }
+ }
+}
+
+which can then be compiled to AML binary format:
+
+$ iasl minnowmax.asl
+
+Intel ACPI Component Architecture
+ASL Optimizing Compiler version 20140214-64 [Mar 29 2014]
+Copyright (c) 2000 - 2014 Intel Corporation
+
+ASL Input: minnomax.asl - 30 lines, 614 bytes, 7 keywords
+AML Output: minnowmax.aml - 165 bytes, 6 named objects, 1 executable opcodes
+
+[1] http://wiki.minnowboard.org/MinnowBoard_MAX#Low_Speed_Expansion_Connector_.28Top.29
+
+The resulting AML code can then be loaded by the kernel using one of the methods
+below.
+
+== Loading ACPI SSDTs from initrd ==
+
+This option allows loading of user defined SSDTs from initrd and it is useful
+when the system does not support EFI or when there is not enough EFI storage.
+
+It works in a similar way with initrd based ACPI tables overrides: SSDT aml code
+must be placed in the first, uncompressed, initrd under the
+"kernel/firmware/acpi/overlay" path. We use a different path than the initrd
+tables override to avoid conflicts with the override feature.
+
+Multiple files can be used and this will translate in loading multiple
+tables. Only tables with the SSDT signature will be loaded.
+
+Here is an example:
+
+# Add the raw ACPI tables to an uncompressed cpio archive.
+# They must be put into a /kernel/firmware/acpi/overlay directory inside the
+# cpio archive.
+# The uncompressed cpio archive must be the first.
+# Other, typically compressed cpio archives, must be
+# concatenated on top of the uncompressed one.
+mkdir -p kernel/firmware/acpi
+cp ssdt.aml kernel/firmware/acpi
+
+# Create the uncompressed cpio archive and concatenate the original initrd
+# on top:
+find kernel | cpio -H newc --create > /boot/instrumented_initrd
+cat /boot/initrd >>/boot/instrumented_initrd
+
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 891c42d..5e0d076 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -37,9 +37,14 @@
#include <acpi/apei.h>
#include <linux/dmi.h>
#include <linux/suspend.h>
+#include <linux/initrd.h>
+#include <linux/earlycpio.h>
#include "internal.h"
+#undef pr_fmt
+#define pr_fmt(fmt) "ACPI: " fmt
+
#define _COMPONENT ACPI_BUS_COMPONENT
ACPI_MODULE_NAME("bus");
@@ -863,6 +868,62 @@ static int __init acpi_bus_init_irq(void)
return 0;
}
+void __init acpi_load_initrd_ssdts(void)
+{
+ void *data = (void *)initrd_start;
+ int size = initrd_end - initrd_start;
+ const char *path = "kernel/firmware/acpi/overlay";
+ long offset = 0;
+ struct cpio_data file;
+ struct acpi_table_header *header;
+ void *table;
+ acpi_status status;
+
+ while (true) {
+ file = find_cpio_data(path, data, size, &offset);
+ if (!file.data)
+ break;
+
+ data += offset;
+ size -= offset;
+
+ if (file.size < sizeof(struct acpi_table_header)) {
+ pr_err("initrd table smaller than ACPI header [%s%s]\n",
+ path, file.name);
+ continue;
+ }
+
+ header = file.data;
+
+ if (file.size != header->length) {
+ pr_err("initrd file / table length mismatch [%s%s]\n",
+ path, file.name);
+ continue;
+ }
+
+ if (memcmp(header->signature, ACPI_SIG_SSDT, 4)) {
+ pr_warn("skipping non-SSDT initrd table [%s%s]\n",
+ path, file.name);
+ continue;
+ }
+
+ table = kmemdup(file.data, file.size, GFP_KERNEL);
+ if (!table)
+ continue;
+
+ status = acpi_install_table((uintptr_t)table, 0);
+ if (ACPI_FAILURE(status)) {
+ pr_err("failed to install SSDT from initrd [%s%s]\n",
+ path, file.name);
+ kfree(table);
+ }
+
+ pr_info("installed SSDT table found in initrd [%s%s][0x%x]\n",
+ path, file.name, header->length);
+ add_taint(TAINT_OVERLAY_ACPI_TABLE, LOCKDEP_STILL_OK);
+ }
+}
+
/**
* acpi_early_init - Initialize ACPICA and populate the ACPI namespace.
*
@@ -911,6 +972,8 @@ void __init acpi_early_init(void)
goto error0;
}
+ acpi_load_initrd_ssdts();
+
status = acpi_load_tables();
if (ACPI_FAILURE(status)) {
printk(KERN_ERR PREFIX
--
1.9.1
On Thu, Mar 31, 2016 at 12:37:02PM +0300, Octavian Purdila wrote:
> +#if IS_ENABLED(CONFIG_ACPI)
> +static int acpi_spi_table_load(struct device *dev, const void *data)
> +{
> + struct spi_master *master = container_of(dev, struct spi_master, dev);
> +
> + acpi_register_spi_devices(master);
> + return 0;
> +}
Why do we have a separate code path for this coompared to the initial
startup? The handling appears to be identical so it seems we should
drive this from the ACPI code so we don't have to add this to every
single bus with ACPI bindings.
Hi,
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Octavian Purdila
> Subject: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
>
> Add support for acpi_user_table configfs items that allows the user to
> load new tables. The data attributes contains the table data and once it
> is filled from userspace the table is loaded and ACPI devices are
> enumerated.
[Lv Zheng]
We've been considering to implement this facility before.
The 2 alternative solutions are:
1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg - this will be useful for extracting AML byte stream from kernel to be used by a userspace disassembler.
2. transition /sys/firmware/acpi/tables/xxx into directory and implement /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this should be able to meet your requirement.
So my first question is:
Why do you use configfs rather than the existing mechanisms?
>
> Signed-off-by: Octavian Purdila <[email protected]>
> ---
> Documentation/ABI/testing/configfs-acpi | 16 +++++
> Documentation/acpi/ssdt-overlays.txt | 14 +++++
> drivers/acpi/configfs.c | 104 ++++++++++++++++++++++++++++++++
> 3 files changed, 134 insertions(+)
>
> diff --git a/Documentation/ABI/testing/configfs-acpi
> b/Documentation/ABI/testing/configfs-acpi
> index 0c806aa..34a205e 100644
> --- a/Documentation/ABI/testing/configfs-acpi
> +++ b/Documentation/ABI/testing/configfs-acpi
> @@ -5,3 +5,19 @@ Contact: [email protected]
> Description:
> This represents the ACPI subsystem entry point directory. It
> contains sub-groups corresponding to ACPI configurable
> options.
> +
> +What: /config/acpi/table
> +Date: April 2016
> +KernelVersion: 4.6
> +Description:
> +
> + This group contains the configuration for user defined ACPI
> + tables. The attributes of a user define table are:
> +
> + data - a binary write only attribute that the user can use to
> + fill in the ACPI aml definitions. Once the aml data is
> + written to this file and the file is closed the table
> + will be loaded and ACPI device will be enumerated. To
> + check if the operation is successful the user must check
> + the error code for close(). If the operation is
> + successful, subsequent writes to this attribute will fail.
> diff --git a/Documentation/acpi/ssdt-overlays.txt b/Documentation/acpi/ssdt-
> overlays.txt
> index 7c588be..a88a2ce 100644
> --- a/Documentation/acpi/ssdt-overlays.txt
> +++ b/Documentation/acpi/ssdt-overlays.txt
> @@ -158,3 +158,17 @@ tmp=$(mktemp)
> /bin/echo -ne "\007\000\000\000" | cat - $filename > $tmp
> dd if=$tmp of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp)
> rm $tmp
> +
> +== Loading ACPI SSDTs from configfs ==
> +
> +This option allows loading of user defined SSDTs from userspace via the
> configfs
> +interface. The CONFIG_ACPI_CONFIGFS option must be select and configfs
> must be
> +mounted. In the following examples, we assume that configfs has been
> mounted in
> +/config.
> +
> +New tables can be loading by creating new directories in /config/acpi/table/
> and
> +writing the SSDT aml code in the data attribute:
> +
> +cd /config/acpi/table
> +mkdir my_ssdt
> +cat ~/ssdt.aml > my_ssdt/data
[Lv Zheng]
Good to see the table as a directory.
So that we can put more attributes under it.
Thanks
-Lv
> diff --git a/drivers/acpi/configfs.c b/drivers/acpi/configfs.c
> index 96aa3d8..3a194806 100644
> --- a/drivers/acpi/configfs.c
> +++ b/drivers/acpi/configfs.c
> @@ -13,6 +13,104 @@
> #include <linux/configfs.h>
> #include <linux/acpi.h>
>
> +static struct config_group *acpi_table_group;
> +
> +struct acpi_user_table {
> + struct config_item cfg;
> + struct acpi_table_header *table;
> +};
> +
> +static ssize_t acpi_table_data_write(struct config_item *cfg,
> + const void *data, size_t size)
> +{
> + struct acpi_table_header *header = (struct acpi_table_header *)data;
> + struct acpi_user_table *table;
> + int ret;
> +
> + table = container_of(cfg, struct acpi_user_table, cfg);
> +
> + if (table->table) {
> + pr_err("ACPI configfs table: table already loaded\n");
> + return -EBUSY;
> + }
> +
> + if (header->length != size) {
> + pr_err("ACPI configfs table: invalid table length\n");
> + return -EINVAL;
> + }
> +
> + if (memcmp(header->signature, ACPI_SIG_SSDT, 4)) {
> + pr_err("ACPI configfs table: invalid table signature\n");
> + return -EINVAL;
> + }
> +
> + table = container_of(cfg, struct acpi_user_table, cfg);
> +
> + table->table = kmemdup(header, header->length, GFP_KERNEL);
> + if (!table->table)
> + return -ENOMEM;
> +
> + ret = acpi_load_table(table->table);
> + if (ret) {
> + kfree(table->table);
> + table->table = NULL;
> + }
> +
> + add_taint(TAINT_OVERLAY_ACPI_TABLE, LOCKDEP_STILL_OK);
> +
> + return ret;
> +}
> +
> +#define MAX_ACPI_TABLE_SIZE (128 * 1024)
> +
> +CONFIGFS_BIN_ATTR_WO(acpi_table_, data, NULL, MAX_ACPI_TABLE_SIZE);
> +
> +struct configfs_bin_attribute *acpi_table_bin_attrs[] = {
> + &acpi_table_attr_data,
> + NULL,
> +};
> +
> +static struct config_item_type acpi_table_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_bin_attrs = acpi_table_bin_attrs,
> +};
> +
> +static struct config_item *acpi_table_make_item(struct config_group *group,
> + const char *name)
> +{
> + struct acpi_user_table *table;
> +
> + table = kzalloc(sizeof(*table), GFP_KERNEL);
> + if (!table)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&table->cfg, name, &acpi_table_type);
> + return &table->cfg;
> +}
> +
> +struct configfs_group_operations acpi_table_group_ops = {
> + .make_item = acpi_table_make_item,
> +};
> +
> +static struct config_item_type acpi_tables_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_group_ops = &acpi_table_group_ops,
> +};
> +
> +static struct config_item_type acpi_root_group_type = {
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem acpi_configfs = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "acpi",
> + .ci_type = &acpi_root_group_type,
> + },
> + },
> + .su_mutex = __MUTEX_INITIALIZER(acpi_configfs.su_mutex),
> +};
> +
> static int __init acpi_configfs_init(void)
> {
> int ret;
> @@ -24,12 +122,18 @@ static int __init acpi_configfs_init(void)
> if (ret)
> return ret;
>
> + acpi_table_group = configfs_register_default_group(root, "table",
> + &acpi_tables_type);
> + if (IS_ERR(acpi_table_group))
> + return PTR_ERR(acpi_table_group);
> +
> return 0;
> }
> module_init(acpi_configfs_init);
>
> static void __exit acpi_configfs_exit(void)
> {
> + configfs_unregister_default_group(acpi_table_group);
> configfs_unregister_subsystem(&acpi_configfs);
> }
> module_exit(acpi_configfs_exit);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
IMO, there is already a similar function upstreamed:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85cc81
Could it work for your use case?
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Octavian Purdila
> Subject: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
>
> This patch allows loading user defined SSDTs from the first,
> uncompressed, initrd. The SSDT aml code must be stored in files under
> the /kernel/firmware/acpi/overlay path.
>
> Signed-off-by: Octavian Purdila <[email protected]>
> ---
> Documentation/acpi/ssdt-overlays.txt | 94
> ++++++++++++++++++++++++++++++++++++
> drivers/acpi/bus.c | 63 ++++++++++++++++++++++++
> 2 files changed, 157 insertions(+)
> create mode 100644 Documentation/acpi/ssdt-overlays.txt
>
> diff --git a/Documentation/acpi/ssdt-overlays.txt b/Documentation/acpi/ssdt-
> overlays.txt
> new file mode 100644
> index 0000000..a94c3f9
> --- /dev/null
> +++ b/Documentation/acpi/ssdt-overlays.txt
> @@ -0,0 +1,94 @@
> +
> +In order to support ACPI open-ended hardware configurations (e.g.
> development
> +boards) we need a way to augment the ACPI configuration provided by the
> firmware
> +image. A common example is connecting sensors on I2C / SPI buses on
> development
> +boards.
> +
> +Although this can be accomplished by creating a kernel platform driver or
> +recompiling the firmware image with updated ACPI tables, neither is practical:
> +the former proliferates board specific kernel code while the latter requires
> +access to firmware tools which are often not publicly available.
> +
> +Because ACPI supports external references in AML code a more practical
> +way to augment firmware ACPI configuration is by dynamically loading
> +user defined SSDT tables that contain the board specific information.
> +
> +For example, to enumerate a Bosch BMA222E accelerometer on the I2C bus
> of the
> +Minnowboard MAX development board exposed via the LSE connector [1],
> the
> +following ASL code can be used:
> +
> +DefinitionBlock ("minnowmax.aml", "SSDT", 1, "Vendor", "Accel", 0x00000003)
> +{
> + External (\_SB.I2C6, DeviceObj)
> +
> + Scope (\_SB.I2C6)
> + {
> + Device (STAC)
> + {
> + Name (_ADR, Zero)
> + Name (_HID, "BMA222E")
> +
> + Method (_CRS, 0, Serialized)
> + {
> + Name (RBUF, ResourceTemplate ()
> + {
> + I2cSerialBus (0x0018, ControllerInitiated, 0x00061A80,
> + AddressingMode7Bit, "\\_SB.I2C6", 0x00,
> + ResourceConsumer, ,)
> + GpioInt (Edge, ActiveHigh, Exclusive, PullDown, 0x0000,
> + "\\_SB.GPO2", 0x00, ResourceConsumer, , )
> + { // Pin list
> + 0
> + }
> + })
> + Return (RBUF)
> + }
> + }
> + }
> +}
> +
> +which can then be compiled to AML binary format:
> +
> +$ iasl minnowmax.asl
> +
> +Intel ACPI Component Architecture
> +ASL Optimizing Compiler version 20140214-64 [Mar 29 2014]
> +Copyright (c) 2000 - 2014 Intel Corporation
> +
> +ASL Input: minnomax.asl - 30 lines, 614 bytes, 7 keywords
> +AML Output: minnowmax.aml - 165 bytes, 6 named objects, 1 executable
> opcodes
> +
> +[1]
> http://wiki.minnowboard.org/MinnowBoard_MAX#Low_Speed_Expansion_Co
> nnector_.28Top.29
> +
> +The resulting AML code can then be loaded by the kernel using one of the
> methods
> +below.
> +
> +== Loading ACPI SSDTs from initrd ==
> +
> +This option allows loading of user defined SSDTs from initrd and it is useful
> +when the system does not support EFI or when there is not enough EFI
> storage.
> +
> +It works in a similar way with initrd based ACPI tables overrides: SSDT aml
> code
> +must be placed in the first, uncompressed, initrd under the
> +"kernel/firmware/acpi/overlay" path. We use a different path than the initrd
> +tables override to avoid conflicts with the override feature.
> +
> +Multiple files can be used and this will translate in loading multiple
> +tables. Only tables with the SSDT signature will be loaded.
> +
> +Here is an example:
> +
> +# Add the raw ACPI tables to an uncompressed cpio archive.
> +# They must be put into a /kernel/firmware/acpi/overlay directory inside the
> +# cpio archive.
> +# The uncompressed cpio archive must be the first.
> +# Other, typically compressed cpio archives, must be
> +# concatenated on top of the uncompressed one.
> +mkdir -p kernel/firmware/acpi
> +cp ssdt.aml kernel/firmware/acpi
> +
> +# Create the uncompressed cpio archive and concatenate the original initrd
> +# on top:
> +find kernel | cpio -H newc --create > /boot/instrumented_initrd
> +cat /boot/initrd >>/boot/instrumented_initrd
> +
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 891c42d..5e0d076 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -37,9 +37,14 @@
> #include <acpi/apei.h>
> #include <linux/dmi.h>
> #include <linux/suspend.h>
> +#include <linux/initrd.h>
> +#include <linux/earlycpio.h>
>
> #include "internal.h"
>
> +#undef pr_fmt
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
> #define _COMPONENT ACPI_BUS_COMPONENT
> ACPI_MODULE_NAME("bus");
>
> @@ -863,6 +868,62 @@ static int __init acpi_bus_init_irq(void)
> return 0;
> }
>
> +void __init acpi_load_initrd_ssdts(void)
> +{
> + void *data = (void *)initrd_start;
> + int size = initrd_end - initrd_start;
> + const char *path = "kernel/firmware/acpi/overlay";
> + long offset = 0;
> + struct cpio_data file;
> + struct acpi_table_header *header;
> + void *table;
> + acpi_status status;
> +
> + while (true) {
> + file = find_cpio_data(path, data, size, &offset);
> + if (!file.data)
> + break;
> +
> + data += offset;
> + size -= offset;
> +
> + if (file.size < sizeof(struct acpi_table_header)) {
> + pr_err("initrd table smaller than ACPI header
> [%s%s]\n",
> + path, file.name);
> + continue;
> + }
> +
> + header = file.data;
> +
> + if (file.size != header->length) {
> + pr_err("initrd file / table length mismatch [%s%s]\n",
> + path, file.name);
> + continue;
> + }
> +
> + if (memcmp(header->signature, ACPI_SIG_SSDT, 4)) {
> + pr_warn("skipping non-SSDT initrd table [%s%s]\n",
> + path, file.name);
> + continue;
> + }
> +
> + table = kmemdup(file.data, file.size, GFP_KERNEL);
> + if (!table)
> + continue;
> +
> + status = acpi_install_table((uintptr_t)table, 0);
> + if (ACPI_FAILURE(status)) {
> + pr_err("failed to install SSDT from initrd [%s%s]\n",
> + path, file.name);
> + kfree(table);
> + }
> +
> + pr_info("installed SSDT table found in initrd [%s%s][0x%x]\n",
> + path, file.name, header->length);
> + add_taint(TAINT_OVERLAY_ACPI_TABLE, LOCKDEP_STILL_OK);
> + }
> +}
> +
[Lv Zheng]
I can see that this is so similar to the acpi_initrd_initialize_tables() which is in the drivers/acpi/osl.c.
Please check.
Thanks and best regards
-Lv
> /**
> * acpi_early_init - Initialize ACPICA and populate the ACPI namespace.
> *
> @@ -911,6 +972,8 @@ void __init acpi_early_init(void)
> goto error0;
> }
>
> + acpi_load_initrd_ssdts();
> +
> status = acpi_load_tables();
> if (ACPI_FAILURE(status)) {
> printk(KERN_ERR PREFIX
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <[email protected]> wrote:
> Hi,
Hi Lv,
>> Add support for acpi_user_table configfs items that allows the user to
>> load new tables. The data attributes contains the table data and once it
>> is filled from userspace the table is loaded and ACPI devices are
>> enumerated.
> [Lv Zheng]
> We've been considering to implement this facility before.
> The 2 alternative solutions are:
> 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg - this will be useful for extracting AML byte stream from kernel to be used by a userspace disassembler.
AFAIK adding new ioctls is discouraged.
> 2. transition /sys/firmware/acpi/tables/xxx into directory and implement /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this should be able to meet your requirement.
We can't do that as it would break the ABI.
> So my first question is:
> Why do you use configfs rather than the existing mechanisms?
sysfs is not a good choice for dealing with objects created from
userspace, configfs was created to address this specific need. Since
we want to be able to create and load new tables from userspace this
use-case fits very well with configfs.
On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
> Hi,
>
> IMO, there is already a similar function upstreamed:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85cc81
> Could it work for your use case?
Yes, it is basically the same.
The only difference is on how we handle taint. I think we should use a
new taint for overlays and that we don't need to disable lockdep.
BTW, why is lockdep disabled when we override?
On Thu, Mar 31, 2016 at 8:29 PM, Mark Brown <[email protected]> wrote:
> On Thu, Mar 31, 2016 at 12:37:02PM +0300, Octavian Purdila wrote:
>
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static int acpi_spi_table_load(struct device *dev, const void *data)
>> +{
>> + struct spi_master *master = container_of(dev, struct spi_master, dev);
>> +
>> + acpi_register_spi_devices(master);
>> + return 0;
>> +}
>
> Why do we have a separate code path for this coompared to the initial
> startup? The handling appears to be identical so it seems we should
> drive this from the ACPI code so we don't have to add this to every
> single bus with ACPI bindings.
Hi Mark,
I probably don't fully understand your question, but I don't see a way
of how we can create a new SPI device from generic ACPI code. For
example, in acpi_spi_add_device() we need the spi_master node so that
we can allocate the spi device.
The handling is identical because we don't have yet have a way to
identify what where the new nodes added when a new ACPI table /
overlay has been loaded, so we have to rescan the ACPI namespace under
each controller.
On Fri, Apr 01, 2016 at 01:54:28PM +0300, Octavian Purdila wrote:
> I probably don't fully understand your question, but I don't see a way
> of how we can create a new SPI device from generic ACPI code. For
> example, in acpi_spi_add_device() we need the spi_master node so that
> we can allocate the spi device.
Right, but the same applies to initial enumeration so we also have to
manually instantiate ACPI devices on startup. Why do we need to do
that?
> The handling is identical because we don't have yet have a way to
> identify what where the new nodes added when a new ACPI table /
> overlay has been loaded, so we have to rescan the ACPI namespace under
> each controller.
That's not the point. The point is that since the handling is identical
why are we handling it through exactly the same code?
On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <[email protected]> wrote:
> On Fri, Apr 01, 2016 at 01:54:28PM +0300, Octavian Purdila wrote:
>
>> I probably don't fully understand your question, but I don't see a way
>> of how we can create a new SPI device from generic ACPI code. For
>> example, in acpi_spi_add_device() we need the spi_master node so that
>> we can allocate the spi device.
>
> Right, but the same applies to initial enumeration so we also have to
> manually instantiate ACPI devices on startup. Why do we need to do
> that?
>
>> The handling is identical because we don't have yet have a way to
>> identify what where the new nodes added when a new ACPI table /
>> overlay has been loaded, so we have to rescan the ACPI namespace under
>> each controller.
>
> That's not the point. The point is that since the handling is identical
> why are we handling it through exactly the same code?
I think that during the initial enumeration the controller driver's
probe walks the children and creates device objects for them. When a
table is loaded later, the controller driver has been probed already
and there needs to be a way to trigger a walk over the (new) children
from it.
Or a hook somewhere around acpi_platform_notify() is needed.
On Fri, Apr 01, 2016 at 09:26:38PM +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <[email protected]> wrote:
> > That's not the point. The point is that since the handling is identical
> > why are we handling it through exactly the same code?
> I think that during the initial enumeration the controller driver's
> probe walks the children and creates device objects for them. When a
> table is loaded later, the controller driver has been probed already
> and there needs to be a way to trigger a walk over the (new) children
> from it.
> Or a hook somewhere around acpi_platform_notify() is needed.
What I don't understand is why the flow on inital probe isn't simply to
register the controller which then triggers the walk of the children.
That way any bus that supports initial probe also supports hotplug
without needing to go and manually add a second code path.
On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <[email protected]> wrote:
> On Fri, Apr 01, 2016 at 09:26:38PM +0200, Rafael J. Wysocki wrote:
>> On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <[email protected]> wrote:
>
>> > That's not the point. The point is that since the handling is identical
>> > why are we handling it through exactly the same code?
>
>> I think that during the initial enumeration the controller driver's
>> probe walks the children and creates device objects for them. When a
>> table is loaded later, the controller driver has been probed already
>> and there needs to be a way to trigger a walk over the (new) children
>> from it.
>
>> Or a hook somewhere around acpi_platform_notify() is needed.
>
> What I don't understand is why the flow on inital probe isn't simply to
> register the controller which then triggers the walk of the children.
> That way any bus that supports initial probe also supports hotplug
> without needing to go and manually add a second code path.
Do you mean register the notifier per controller instead of per
subsystem? Either way we need changes at the subsystem level and I
choose to follow the device tree implementation for consistency.
The other reason is that (pending other ACPICA changes) we can add
other notification events in the future such as node added or removed
(just like device tree), and in that case the probe and hotplug
handling would be different (and a bit more efficient).
On Fri, Apr 1, 2016 at 1:11 PM, Octavian Purdila
<[email protected]> wrote:
> On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
>> Hi,
>>
>> IMO, there is already a similar function upstreamed:
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85cc81
>> Could it work for your use case?
>
> Yes, it is basically the same.
>
> The only difference is on how we handle taint. I think we should use a
> new taint for overlays and that we don't need to disable lockdep.
>
> BTW, why is lockdep disabled when we override?
The other thing I forgot to mention is that I think we should allow
installing new tables even if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is not
selected. IMO the override and overlay functionality is different,
with the latter being more then a debug option.
I will prepare a patch for the next version of the series to decouple
installing new tables from CONFIG_ACPI_INITRD_TABLE_OVERRIDE.
On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <[email protected]> wrote:
> > What I don't understand is why the flow on inital probe isn't simply to
> > register the controller which then triggers the walk of the children.
> > That way any bus that supports initial probe also supports hotplug
> > without needing to go and manually add a second code path.
> Do you mean register the notifier per controller instead of per
> subsystem? Either way we need changes at the subsystem level and I
> choose to follow the device tree implementation for consistency.
No! I mean use the exact same callback you've got now for everything.
> The other reason is that (pending other ACPICA changes) we can add
> other notification events in the future such as node added or removed
> (just like device tree), and in that case the probe and hotplug
> handling would be different (and a bit more efficient).
Why is probe different to hotplug? We don't need to do that in the
normal driver model.
On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <[email protected]> wrote:
> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <[email protected]> wrote:
>
>> > What I don't understand is why the flow on inital probe isn't simply to
>> > register the controller which then triggers the walk of the children.
>> > That way any bus that supports initial probe also supports hotplug
>> > without needing to go and manually add a second code path.
>
>> Do you mean register the notifier per controller instead of per
>> subsystem? Either way we need changes at the subsystem level and I
>> choose to follow the device tree implementation for consistency.
>
> No! I mean use the exact same callback you've got now for everything.
>
>> The other reason is that (pending other ACPICA changes) we can add
>> other notification events in the future such as node added or removed
>> (just like device tree), and in that case the probe and hotplug
>> handling would be different (and a bit more efficient).
>
> Why is probe different to hotplug? We don't need to do that in the
> normal driver model.
There might be some confusion with the term, I am referring to slave
hotplug, not controller hotplug.
The way I see it, there are two logical operations: probe of a
controller and the associated enumeration of the SPI slaves for that
bus and "hotplug" of new SPI slaves and the enumeration of those
particular slaves.
When we probe the controller we search DT/ACPI and enumerate all the
slaves for *that* controller.
When a slave hotplug happens for device tree we get a device node
notification and we can instantiate the SPI slave based on that info.
In case of ACPI, (at this point) we get a global callback and in that
callback we need to iterate through *all* controllers.
On Mon, Apr 4, 2016 at 9:34 PM, Octavian Purdila
<[email protected]> wrote:
> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <[email protected]> wrote:
>> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <[email protected]> wrote:
>>
>>> > What I don't understand is why the flow on inital probe isn't simply to
>>> > register the controller which then triggers the walk of the children.
>>> > That way any bus that supports initial probe also supports hotplug
>>> > without needing to go and manually add a second code path.
>>
>>> Do you mean register the notifier per controller instead of per
>>> subsystem? Either way we need changes at the subsystem level and I
>>> choose to follow the device tree implementation for consistency.
>>
>> No! I mean use the exact same callback you've got now for everything.
>>
>>> The other reason is that (pending other ACPICA changes) we can add
>>> other notification events in the future such as node added or removed
>>> (just like device tree), and in that case the probe and hotplug
>>> handling would be different (and a bit more efficient).
>>
>> Why is probe different to hotplug? We don't need to do that in the
>> normal driver model.
>
> There might be some confusion with the term, I am referring to slave
> hotplug, not controller hotplug.
>
> The way I see it, there are two logical operations: probe of a
> controller and the associated enumeration of the SPI slaves for that
> bus and "hotplug" of new SPI slaves and the enumeration of those
> particular slaves.
>
> When we probe the controller we search DT/ACPI and enumerate all the
> slaves for *that* controller.
>
> When a slave hotplug happens for device tree we get a device node
> notification and we can instantiate the SPI slave based on that info.
> In case of ACPI, (at this point) we get a global callback and in that
> callback we need to iterate through *all* controllers.
Is that really necessary?
The namespace rescan could notify the parent of a new node in
acpi_default_enumeration(), couldn't it?
Hi,
> From: Octavian Purdila [mailto:[email protected]]
> Subject: Re: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
>
> On Fri, Apr 1, 2016 at 1:11 PM, Octavian Purdila
> <[email protected]> wrote:
> > On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
> >> Hi,
> >>
> >> IMO, there is already a similar function upstreamed:
> >>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85c
> c81
> >> Could it work for your use case?
> >
> > Yes, it is basically the same.
> >
> > The only difference is on how we handle taint. I think we should use a
> > new taint for overlays and that we don't need to disable lockdep.
> >
> > BTW, why is lockdep disabled when we override?
>
> The other thing I forgot to mention is that I think we should allow
> installing new tables even if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is not
> selected. IMO the override and overlay functionality is different,
> with the latter being more then a debug option.
[Lv Zheng]
I don't think so. The initrd override mechanism is not dependent on CONFIG_ACPI_DEBUG.
According to the spec, we can allow a higher versioning same table (same table sig, table id, table oem id) to override the old tables as a patching functionality.
So both the functionalities are not debug options and serve for the same purpose from this point of view.
And IMO that's why the initrd override mechanism needn't be dependent on CONFIG_ACPI_DEBUG.
I'm really OK with removing the acpi_table_taint() for CONFIG_ACPI_INITRD_TABLE_OVERRIDE but leaving some info messages indicating the table upgrades.
I don't think this mechanism is unsafe.
It happens during a initialization step occurring before the table is loaded and hence should be safe even the synchronization is not so robust in ACPICA.
And with the revision support added, we should be able to allow vendors to update the buggy tables.
That means the tables may be originated from the safe sources - the vendors.
>
> I will prepare a patch for the next version of the series to decouple
> installing new tables from CONFIG_ACPI_INITRD_TABLE_OVERRIDE.
[Lv Zheng]
I don't think they need to be decoupled.
The use case is:
If there is an ACPI table in initrd image and:
1. if the table's revision is higher than the existing one, override the existing one;
2. if the table is a brand new one, install it.
Thanks and best regards
-Lv
Hi,
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Octavian Purdila
> Subject: Re: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
>
> On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
> > Hi,
> >
> > IMO, there is already a similar function upstreamed:
> >
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85c
> c81
> > Could it work for your use case?
>
> Yes, it is basically the same.
>
> The only difference is on how we handle taint. I think we should use a
> new taint for overlays and that we don't need to disable lockdep.
>
> BTW, why is lockdep disabled when we override?
[Lv Zheng]
I guess this is because of the old synchronization bugs.
Originally, the table handler may receive table events when the table is installed.
And that may trigger lock issues in such an early stage.
I don't think the acpi_table_taint() need to be there now.
The override mechanisms now happen in an initialization step before the tables are loaded.
It should be safe even the synchronization is not so robust in ACPICA.
Because during this step, all things are serial.
IMO, you can remove acpi_table_taint().
Thanks and best regards
-Lv
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
> From: Octavian Purdila [mailto:[email protected]]
> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
>
> On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <[email protected]> wrote:
> > Hi,
>
> Hi Lv,
>
> >> Add support for acpi_user_table configfs items that allows the user to
> >> load new tables. The data attributes contains the table data and once it
> >> is filled from userspace the table is loaded and ACPI devices are
> >> enumerated.
> > [Lv Zheng]
> > We've been considering to implement this facility before.
> > The 2 alternative solutions are:
> > 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg - this
> will be useful for extracting AML byte stream from kernel to be used by a
> userspace disassembler.
>
> AFAIK adding new ioctls is discouraged.
[Lv Zheng]
Tools/power/acpi/tools/acpidbg is a file descriptor based utility.
And it needs a method to obtain an AML byte stream from kernel.
Using ioctl is a best fit design for acpidbg so that it needn't to access any other files.
>
> > 2. transition /sys/firmware/acpi/tables/xxx into directory and implement
> /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this should
> be able to meet your requirement.
>
> We can't do that as it would break the ABI.
[Lv Zheng]
The only user of this directory hierarchy is acpidump.
And the user of this tool are all developers/reporters on the kernel bugzilla.
We've been asking the Bugzilla users to use the up-to-date acpidump instead of the distribution vendor provided one for so many years.
So IMO, this is not a serious problem you should consider.
You only need to think about an acceptable way for the distribution vendors to synchronize the kernel change and the acpidump change.
IMO:
You may expose a version file from /sys/firmware/acpi.
acpidump can be changed accordingly by referencing the version file.
And old directory hierarchy support could be kept in acpidump.
Note that acpidump is also a part of the kernel, so your change could be consistent.
For example,
If you changed acpidump prior than making the kernel change, the distribution vendors might have already released the new acpidump for all old kernels before you transitioned the directory hierarchy.
>
> > So my first question is:
> > Why do you use configfs rather than the existing mechanisms?
>
> sysfs is not a good choice for dealing with objects created from
> userspace, configfs was created to address this specific need. Since
> we want to be able to create and load new tables from userspace this
> use-case fits very well with configfs.
[Lv Zheng]
Was the table binary stream still maintained by the userspace?
If not, I couldn't see the difference/advantages from using /sys/firmware/acpi/tables to using configfs.
Thanks and best regards
-Lv
On Tue, Apr 5, 2016 at 3:49 AM, Zheng, Lv <[email protected]> wrote:
> Hi,
>
>> From: Octavian Purdila [mailto:[email protected]]
>> Subject: Re: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
>>
>> On Fri, Apr 1, 2016 at 1:11 PM, Octavian Purdila
>> <[email protected]> wrote:
>> > On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
>> >> Hi,
>> >>
>> >> IMO, there is already a similar function upstreamed:
>> >>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85c
>> c81
>> >> Could it work for your use case?
>> >
>> > Yes, it is basically the same.
>> >
>> > The only difference is on how we handle taint. I think we should use a
>> > new taint for overlays and that we don't need to disable lockdep.
>> >
>> > BTW, why is lockdep disabled when we override?
>>
>> The other thing I forgot to mention is that I think we should allow
>> installing new tables even if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is not
>> selected. IMO the override and overlay functionality is different,
>> with the latter being more then a debug option.
> [Lv Zheng]
> I don't think so. The initrd override mechanism is not dependent on CONFIG_ACPI_DEBUG.
> According to the spec, we can allow a higher versioning same table (same table sig, table id, table oem id) to override the old tables as a patching functionality.
> So both the functionalities are not debug options and serve for the same purpose from this point of view.
> And IMO that's why the initrd override mechanism needn't be dependent on CONFIG_ACPI_DEBUG.
>
The problem is that CONFIG_ACPI_INITRD_TABLE_OVERRIDE is presented as
a debug option in Documentation/initrd_table_override.txt and most
distributions are not selecting it which makes it hard to use it in
practice.
> I'm really OK with removing the acpi_table_taint() for CONFIG_ACPI_INITRD_TABLE_OVERRIDE but leaving some info messages indicating the table upgrades.
> I don't think this mechanism is unsafe.
> It happens during a initialization step occurring before the table is loaded and hence should be safe even the synchronization is not so robust in ACPICA.
> And with the revision support added, we should be able to allow vendors to update the buggy tables.
> That means the tables may be originated from the safe sources - the vendors.
>
>>
>> I will prepare a patch for the next version of the series to decouple
>> installing new tables from CONFIG_ACPI_INITRD_TABLE_OVERRIDE.
> [Lv Zheng]
> I don't think they need to be decoupled.
> The use case is:
> If there is an ACPI table in initrd image and:
> 1. if the table's revision is higher than the existing one, override the existing one;
> 2. if the table is a brand new one, install it.
>
The implementation will stay the same of course, I was just suggesting
to move CONFIG_ACPI_INITRD_TABLE_OVERRIDE in
acpi_os_physical_table_override to allow new tables to be loaded even
if the option is not selected.
On Tue, Apr 5, 2016 at 6:11 AM, Zheng, Lv <[email protected]> wrote:
> Hi,
>
>> From: Octavian Purdila [mailto:[email protected]]
>> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
>>
>> On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <[email protected]> wrote:
>> > Hi,
>>
>> Hi Lv,
>>
>> >> Add support for acpi_user_table configfs items that allows the user to
>> >> load new tables. The data attributes contains the table data and once it
>> >> is filled from userspace the table is loaded and ACPI devices are
>> >> enumerated.
>> > [Lv Zheng]
>> > We've been considering to implement this facility before.
>> > The 2 alternative solutions are:
>> > 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg - this
>> will be useful for extracting AML byte stream from kernel to be used by a
>> userspace disassembler.
>>
>> AFAIK adding new ioctls is discouraged.
> [Lv Zheng]
> Tools/power/acpi/tools/acpidbg is a file descriptor based utility.
> And it needs a method to obtain an AML byte stream from kernel.
> Using ioctl is a best fit design for acpidbg so that it needn't to access any other files.
>
>>
>> > 2. transition /sys/firmware/acpi/tables/xxx into directory and implement
>> /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this should
>> be able to meet your requirement.
>>
>> We can't do that as it would break the ABI.
> [Lv Zheng]
> The only user of this directory hierarchy is acpidump.
Some systems (e.g. Android, Brillo) do not ship acpidump and in this
case the only way to dump tables is by accessing sysfs directly.
> And the user of this tool are all developers/reporters on the kernel bugzilla.
> We've been asking the Bugzilla users to use the up-to-date acpidump instead of the distribution vendor provided one for so many years.
> So IMO, this is not a serious problem you should consider.
> You only need to think about an acceptable way for the distribution vendors to synchronize the kernel change and the acpidump change.
>
I guess the perf model where you have a perf package build together
with the kernel will work. But right now distributions are not using
this model and a kernel change will break the userspace tool.
> IMO:
> You may expose a version file from /sys/firmware/acpi.
> acpidump can be changed accordingly by referencing the version file.
> And old directory hierarchy support could be kept in acpidump.
>
That will still break if you upgrade the kernel and use the old tool.
> Note that acpidump is also a part of the kernel, so your change could be consistent.
> For example,
> If you changed acpidump prior than making the kernel change, the distribution vendors might have already released the new acpidump for all old kernels before you transitioned the directory hierarchy.
To me this still looks like breaking userspace, patches have been
reverted for less :) But I can see how this could be considered a gray
area.
ABI aside, see below for why configfs is better then sysfs.
>
>>
>> > So my first question is:
>> > Why do you use configfs rather than the existing mechanisms?
>>
>> sysfs is not a good choice for dealing with objects created from
>> userspace, configfs was created to address this specific need. Since
>> we want to be able to create and load new tables from userspace this
>> use-case fits very well with configfs.
> [Lv Zheng]
> Was the table binary stream still maintained by the userspace?
> If not, I couldn't see the difference/advantages from using /sys/firmware/acpi/tables to using configfs.
>
It is hard to create new kernel objects from sysfs. You need to resort
to hacks like using new_table sysfs entries which does not map to a
kernel object. Writes larger then PAGE_SIZE are impossible to handle
with multiple open files because you have no open callback to create a
file context. It is also not possible to do any clean-up because there
is no close callback and if something goes wrong for example when
trying to install the table you will leak the allocated memory.
configfs was designed for the specific purpose of creating kernel
objects from userspace and addresses all of the limitations above (and
some more).
Initially I started to implement this functionality via sysfs but I
run into the issues mentioned above and decided to use configfs.
On Tue, Apr 5, 2016 at 12:18 AM, Rafael J. Wysocki <[email protected]> wrote:
> On Mon, Apr 4, 2016 at 9:34 PM, Octavian Purdila
> <[email protected]> wrote:
>> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <[email protected]> wrote:
>>> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>>>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <[email protected]> wrote:
>>>
>>>> > What I don't understand is why the flow on inital probe isn't simply to
>>>> > register the controller which then triggers the walk of the children.
>>>> > That way any bus that supports initial probe also supports hotplug
>>>> > without needing to go and manually add a second code path.
>>>
>>>> Do you mean register the notifier per controller instead of per
>>>> subsystem? Either way we need changes at the subsystem level and I
>>>> choose to follow the device tree implementation for consistency.
>>>
>>> No! I mean use the exact same callback you've got now for everything.
>>>
>>>> The other reason is that (pending other ACPICA changes) we can add
>>>> other notification events in the future such as node added or removed
>>>> (just like device tree), and in that case the probe and hotplug
>>>> handling would be different (and a bit more efficient).
>>>
>>> Why is probe different to hotplug? We don't need to do that in the
>>> normal driver model.
>>
>> There might be some confusion with the term, I am referring to slave
>> hotplug, not controller hotplug.
>>
>> The way I see it, there are two logical operations: probe of a
>> controller and the associated enumeration of the SPI slaves for that
>> bus and "hotplug" of new SPI slaves and the enumeration of those
>> particular slaves.
>>
>> When we probe the controller we search DT/ACPI and enumerate all the
>> slaves for *that* controller.
>>
>> When a slave hotplug happens for device tree we get a device node
>> notification and we can instantiate the SPI slave based on that info.
>> In case of ACPI, (at this point) we get a global callback and in that
>> callback we need to iterate through *all* controllers.
>
> Is that really necessary?
>
> The namespace rescan could notify the parent of a new node in
> acpi_default_enumeration(), couldn't it?
We could do that, with some complexity, but I am not sure if it is
worth it - see below. And in either case I still don't see how we can
avoid two enumeration paths: one from the probe of the controller ->
spi_register_master() -> acpi_register_spi_devices() and one from the
notification handler -> acpi_register_spi_devices(). Both will end up
are calling the same enumeration function but there are still two
paths.
Even right now, in the case of dynamic DT, we have the two enumeration paths.
If we really want to have a single path for ACPI enumeration we could
do that by using an ACPI SPI bridge driver or scan handlers after
extending the matching mechanisms. But we would still need to modify
the SPI subsystem and I don't think its worth it just to save a call
to acpi_register_spi_devices() from spi_register_master().
Now for parent notification complexity: in the case of SPI we can
easily to this because current ACPI SPI enumeration supports only
direct children as slaves. However, on I2C we can have an unrelated
node as a slave - that is why the I2C scanning searches the whole
namespaces for references to a particular i2c_adapter. So, we would
need to retrieve the parent node from the namespace node information
which means that we will do SPI specific stuff in ACPI generic code. I
don't think it is a big issue, because we already treat SPI / I2C
special, right?
Once we have the parent handle we would still have to iterate through
all controllers to get the spi_master matching the ACPI handle. It is
a bit more efficient then global notification, but probably won't
matter in practice.
On Mon, Apr 04, 2016 at 10:34:56PM +0300, Octavian Purdila wrote:
> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <[email protected]> wrote:
> > Why is probe different to hotplug? We don't need to do that in the
> > normal driver model.
> There might be some confusion with the term, I am referring to slave
> hotplug, not controller hotplug.
That's what I was talking about too.
> The way I see it, there are two logical operations: probe of a
> controller and the associated enumeration of the SPI slaves for that
> bus and "hotplug" of new SPI slaves and the enumeration of those
> particular slaves.
I don't see a distinction here. The firmware finds some new slaves to
tell the framework about. Quite why it decided to go looking shouldn't
matter.
> When a slave hotplug happens for device tree we get a device node
> notification and we can instantiate the SPI slave based on that info.
> In case of ACPI, (at this point) we get a global callback and in that
> callback we need to iterate through *all* controllers.
That's not really helping me understand why you need every bus to open
code enumeration twice?
On Tue, Apr 05, 2016 at 02:49:13PM +0300, Octavian Purdila wrote:
> If we really want to have a single path for ACPI enumeration we could
> do that by using an ACPI SPI bridge driver or scan handlers after
> extending the matching mechanisms. But we would still need to modify
> the SPI subsystem and I don't think its worth it just to save a call
> to acpi_register_spi_devices() from spi_register_master().
It's not specifically for SPI, it's the fact that you're asking every
single bus type which might be described in ACPI to handle both hotplug
and coldplug paths separately. Given that the code that's being added
just seems like trivial boilerplate it seems like we're doing this
wrong, we should be factoring this out so there's nothing bus types can
get wrong.
> Now for parent notification complexity: in the case of SPI we can
> easily to this because current ACPI SPI enumeration supports only
> direct children as slaves. However, on I2C we can have an unrelated
> node as a slave - that is why the I2C scanning searches the whole
> namespaces for references to a particular i2c_adapter. So, we would
> need to retrieve the parent node from the namespace node information
> which means that we will do SPI specific stuff in ACPI generic code. I
> don't think it is a big issue, because we already treat SPI / I2C
> special, right?
Or perhaps the issue is that we can't make our mind up if the bus
specific code should go in the bus or in the ACPI core?
On Tue, Apr 5, 2016 at 9:32 PM, Mark Brown <[email protected]> wrote:
> On Tue, Apr 05, 2016 at 02:49:13PM +0300, Octavian Purdila wrote:
>
>> If we really want to have a single path for ACPI enumeration we could
>> do that by using an ACPI SPI bridge driver or scan handlers after
>> extending the matching mechanisms. But we would still need to modify
>> the SPI subsystem and I don't think its worth it just to save a call
>> to acpi_register_spi_devices() from spi_register_master().
>
> It's not specifically for SPI, it's the fact that you're asking every
> single bus type which might be described in ACPI to handle both hotplug
> and coldplug paths separately. Given that the code that's being added
> just seems like trivial boilerplate it seems like we're doing this
> wrong, we should be factoring this out so there's nothing bus types can
> get wrong.
>
AFAICS this is exactly the same case for DT: one code path for
coldplug and one for hotplug.
Which makes me think that it is not possible to have a single path for
both, or maybe its not worth it. Do I miss something obvious?
On Tue, Apr 05, 2016 at 10:16:16PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 9:32 PM, Mark Brown <[email protected]> wrote:
> > It's not specifically for SPI, it's the fact that you're asking every
> > single bus type which might be described in ACPI to handle both hotplug
> > and coldplug paths separately. Given that the code that's being added
> > just seems like trivial boilerplate it seems like we're doing this
> > wrong, we should be factoring this out so there's nothing bus types can
> > get wrong.
> AFAICS this is exactly the same case for DT: one code path for
> coldplug and one for hotplug.
In the DT case the DT code understands a bit more of what's going on
with the firmware parsing which makes things better (the code in the two
paths is not identical, unlike ACPI) but yes, that's another aspect of
why I'm not thrilled with this - if it's not the hotplug and coldplug
that should be sharing code then perhaps it's along the firmware
interface axis that we need to be sharing things.
> Which makes me think that it is not possible to have a single path for
> both, or maybe its not worth it. Do I miss something obvious?
I think at a very high level what I'm looking at here is that we're
having to write too much boilerplate per firmware interface. Perhaps
the hotplug vs coldplug path isn't it, it looked like it from your
patch, but there must be something here we're spending too much time on.
Hi,
> From: Octavian Purdila [mailto:[email protected]]
> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
>
> On Tue, Apr 5, 2016 at 6:11 AM, Zheng, Lv <[email protected]> wrote:
> > Hi,
> >
> >> From: Octavian Purdila [mailto:[email protected]]
> >> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via
> configfs
> >>
> >> On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <[email protected]> wrote:
> >> > Hi,
> >>
> >> Hi Lv,
> >>
> >> >> Add support for acpi_user_table configfs items that allows the user to
> >> >> load new tables. The data attributes contains the table data and once it
> >> >> is filled from userspace the table is loaded and ACPI devices are
> >> >> enumerated.
> >> > [Lv Zheng]
> >> > We've been considering to implement this facility before.
> >> > The 2 alternative solutions are:
> >> > 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg -
> this
> >> will be useful for extracting AML byte stream from kernel to be used by a
> >> userspace disassembler.
> >>
> >> AFAIK adding new ioctls is discouraged.
> > [Lv Zheng]
> > Tools/power/acpi/tools/acpidbg is a file descriptor based utility.
> > And it needs a method to obtain an AML byte stream from kernel.
> > Using ioctl is a best fit design for acpidbg so that it needn't to access any
> other files.
> >
> >>
> >> > 2. transition /sys/firmware/acpi/tables/xxx into directory and implement
> >> /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this
> should
> >> be able to meet your requirement.
> >>
> >> We can't do that as it would break the ABI.
> > [Lv Zheng]
> > The only user of this directory hierarchy is acpidump.
>
> Some systems (e.g. Android, Brillo) do not ship acpidump and in this
> case the only way to dump tables is by accessing sysfs directly.
>
> > And the user of this tool are all developers/reporters on the kernel bugzilla.
> > We've been asking the Bugzilla users to use the up-to-date acpidump instead
> of the distribution vendor provided one for so many years.
> > So IMO, this is not a serious problem you should consider.
> > You only need to think about an acceptable way for the distribution vendors
> to synchronize the kernel change and the acpidump change.
> >
>
> I guess the perf model where you have a perf package build together
> with the kernel will work. But right now distributions are not using
> this model and a kernel change will break the userspace tool.
>
> > IMO:
> > You may expose a version file from /sys/firmware/acpi.
> > acpidump can be changed accordingly by referencing the version file.
> > And old directory hierarchy support could be kept in acpidump.
> >
>
> That will still break if you upgrade the kernel and use the old tool.
>
> > Note that acpidump is also a part of the kernel, so your change could be
> consistent.
> > For example,
> > If you changed acpidump prior than making the kernel change, the
> distribution vendors might have already released the new acpidump for all old
> kernels before you transitioned the directory hierarchy.
>
> To me this still looks like breaking userspace, patches have been
> reverted for less :) But I can see how this could be considered a gray
> area.
[Lv Zheng]
For the "userspace breaking" concern.
IMO, we needn't worry about that too much.
>
> ABI aside, see below for why configfs is better then sysfs.
>
> >
> >>
> >> > So my first question is:
> >> > Why do you use configfs rather than the existing mechanisms?
> >>
> >> sysfs is not a good choice for dealing with objects created from
> >> userspace, configfs was created to address this specific need. Since
> >> we want to be able to create and load new tables from userspace this
> >> use-case fits very well with configfs.
> > [Lv Zheng]
> > Was the table binary stream still maintained by the userspace?
> > If not, I couldn't see the difference/advantages from using
> /sys/firmware/acpi/tables to using configfs.
> >
>
> It is hard to create new kernel objects from sysfs. You need to resort
> to hacks like using new_table sysfs entries which does not map to a
> kernel object. Writes larger then PAGE_SIZE are impossible to handle
> with multiple open files because you have no open callback to create a
> file context. It is also not possible to do any clean-up because there
> is no close callback and if something goes wrong for example when
> trying to install the table you will leak the allocated memory.
>
> configfs was designed for the specific purpose of creating kernel
> objects from userspace and addresses all of the limitations above (and
> some more).
>
> Initially I started to implement this functionality via sysfs but I
> run into the issues mentioned above and decided to use configfs.
[Lv Zheng]
I can sense different difficulties from your descriptions.
Let me break it down into details.
We already have acpi_table_handler working there for creating new ACPI table entries for us.
Based on this facility, let's think about the following solution:
1. sysfs presenting change
We can change the table file to a table directory whose name is in the following format:
TableSignature-OemId-OemTableId
Then we can get rid of the annoying numbered table name suffix first.
The numbered table name suffix cannot be kept consistent to reflect the real index if we allow tables to be dynamically loaded/unloaded.
This is the first design difficulty we need to solve.
2. acpi_table_handler change
Now we can append 2 new events to acpi_table_handler - ACPI_TABLE_INSTALL/ACPI_TABLE_UNINSTALL.
With which, the sysfs entries can be created/deleted when the table is added to/removed from the global table list.
And this should be the working mechanism for us
So we actually don't have the trouble to deal with the new kernel object creation/deletion from sysfs.
I agree the dynamic kernel object creation/deletion need special care.
But this actually is what a kernel engineer should do because this kind of things happen here and there throughout the kernel.
We should have already been used to that.
This is the second engineering difficulty we need to face.
3. load/unload commanding
Now we need a character device in sysfs to handle load/unload command.
Well, there are many such kind of files in sysfs, for example, device nodes.
So this is not a non-achievable task, but just a difficult engineering task.
The system engineers need to be skillful enough to implement this.
Like the dynamic kernel object handling, we should have already been used to this.
If you still think this is difficult, the alternative choice is to use acpidbg char device's ioctl interface.
That could simplifies this task.
And since the ioctl interface is required by ACPICA disassembler, the work on that will be inherited by the disassembler porting.
This is the last engineering difficulty we need to face.
So why can't these solutions work for us?
Thanks and best regards
-Lv
Hi,
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Octavian Purdila
> Subject: Re: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
>
> On Tue, Apr 5, 2016 at 3:49 AM, Zheng, Lv <[email protected]> wrote:
> > Hi,
> >
> >> From: Octavian Purdila [mailto:[email protected]]
> >> Subject: Re: [RFC PATCH 02/10] acpi: install SSDT tables from initrd
> >>
> >> On Fri, Apr 1, 2016 at 1:11 PM, Octavian Purdila
> >> <[email protected]> wrote:
> >> > On Fri, Apr 1, 2016 at 8:05 AM, Zheng, Lv <[email protected]> wrote:
> >> >> Hi,
> >> >>
> >> >> IMO, there is already a similar function upstreamed:
> >> >>
> >>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c85c
> >> c81
> >> >> Could it work for your use case?
> >> >
> >> > Yes, it is basically the same.
> >> >
> >> > The only difference is on how we handle taint. I think we should use a
> >> > new taint for overlays and that we don't need to disable lockdep.
> >> >
> >> > BTW, why is lockdep disabled when we override?
> >>
> >> The other thing I forgot to mention is that I think we should allow
> >> installing new tables even if CONFIG_ACPI_INITRD_TABLE_OVERRIDE is not
> >> selected. IMO the override and overlay functionality is different,
> >> with the latter being more then a debug option.
> > [Lv Zheng]
> > I don't think so. The initrd override mechanism is not dependent on
> CONFIG_ACPI_DEBUG.
> > According to the spec, we can allow a higher versioning same table (same
> table sig, table id, table oem id) to override the old tables as a patching
> functionality.
> > So both the functionalities are not debug options and serve for the same
> purpose from this point of view.
> > And IMO that's why the initrd override mechanism needn't be dependent on
> CONFIG_ACPI_DEBUG.
> >
>
> The problem is that CONFIG_ACPI_INITRD_TABLE_OVERRIDE is presented as
> a debug option in Documentation/initrd_table_override.txt and most
> distributions are not selecting it which makes it hard to use it in
> practice.
>
> > I'm really OK with removing the acpi_table_taint() for
> CONFIG_ACPI_INITRD_TABLE_OVERRIDE but leaving some info messages
> indicating the table upgrades.
> > I don't think this mechanism is unsafe.
> > It happens during a initialization step occurring before the table is loaded and
> hence should be safe even the synchronization is not so robust in ACPICA.
> > And with the revision support added, we should be able to allow vendors to
> update the buggy tables.
> > That means the tables may be originated from the safe sources - the vendors.
> >
> >>
> >> I will prepare a patch for the next version of the series to decouple
> >> installing new tables from CONFIG_ACPI_INITRD_TABLE_OVERRIDE.
> > [Lv Zheng]
> > I don't think they need to be decoupled.
> > The use case is:
> > If there is an ACPI table in initrd image and:
> > 1. if the table's revision is higher than the existing one, override the existing
> one;
> > 2. if the table is a brand new one, install it.
> >
>
> The implementation will stay the same of course, I was just suggesting
> to move CONFIG_ACPI_INITRD_TABLE_OVERRIDE in
> acpi_os_physical_table_override to allow new tables to be loaded even
> if the option is not selected.
[Lv Zheng]
This sounds reasonable.
Or you can rename it to CONFIG_ACPI_TABLE_UPGRADE and make it default 'y'.
Also you need to remove acpi_table_taint() which is not so useful now.
Thanks and best regards
-Lv
On Wed, Apr 6, 2016 at 9:05 AM, Zheng, Lv <[email protected]> wrote:
>> It is hard to create new kernel objects from sysfs. You need to resort
>> to hacks like using new_table sysfs entries which does not map to a
>> kernel object. Writes larger then PAGE_SIZE are impossible to handle
>> with multiple open files because you have no open callback to create a
>> file context. It is also not possible to do any clean-up because there
>> is no close callback and if something goes wrong for example when
>> trying to install the table you will leak the allocated memory.
>>
>> configfs was designed for the specific purpose of creating kernel
>> objects from userspace and addresses all of the limitations above (and
>> some more).
>>
>> Initially I started to implement this functionality via sysfs but I
>> run into the issues mentioned above and decided to use configfs.
> [Lv Zheng]
> I can sense different difficulties from your descriptions.
> Let me break it down into details.
>
> We already have acpi_table_handler working there for creating new ACPI table entries for us.
> Based on this facility, let's think about the following solution:
> 1. sysfs presenting change
> We can change the table file to a table directory whose name is in the following format:
> TableSignature-OemId-OemTableId
> Then we can get rid of the annoying numbered table name suffix first.
> The numbered table name suffix cannot be kept consistent to reflect the real index if we allow tables to be dynamically loaded/unloaded.
>
> This is the first design difficulty we need to solve.
>
> 2. acpi_table_handler change
> Now we can append 2 new events to acpi_table_handler - ACPI_TABLE_INSTALL/ACPI_TABLE_UNINSTALL.
> With which, the sysfs entries can be created/deleted when the table is added to/removed from the global table list.
> And this should be the working mechanism for us
> So we actually don't have the trouble to deal with the new kernel object creation/deletion from sysfs.
>
> I agree the dynamic kernel object creation/deletion need special care.
> But this actually is what a kernel engineer should do because this kind of things happen here and there throughout the kernel.
> We should have already been used to that.
>
> This is the second engineering difficulty we need to face.
>
> 3. load/unload commanding
> Now we need a character device in sysfs to handle load/unload command.
> Well, there are many such kind of files in sysfs, for example, device nodes.
> So this is not a non-achievable task, but just a difficult engineering task.
> The system engineers need to be skillful enough to implement this.
> Like the dynamic kernel object handling, we should have already been used to this.
>
>
> If you still think this is difficult, the alternative choice is to use acpidbg char device's ioctl interface.
> That could simplifies this task.
> And since the ioctl interface is required by ACPICA disassembler, the work on that will be inherited by the disassembler porting.
>
Why can't the dissembler access the tables through the existing sysfs interface?
> This is the last engineering difficulty we need to face.
>
> So why can't these solutions work for us?
>
It can be done, but it is not the right way to do it, IMO.
Hi,
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Octavian Purdila
> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
>
> On Wed, Apr 6, 2016 at 9:05 AM, Zheng, Lv <[email protected]> wrote:
>
> >> It is hard to create new kernel objects from sysfs. You need to resort
> >> to hacks like using new_table sysfs entries which does not map to a
> >> kernel object. Writes larger then PAGE_SIZE are impossible to handle
> >> with multiple open files because you have no open callback to create a
> >> file context. It is also not possible to do any clean-up because there
> >> is no close callback and if something goes wrong for example when
> >> trying to install the table you will leak the allocated memory.
> >>
> >> configfs was designed for the specific purpose of creating kernel
> >> objects from userspace and addresses all of the limitations above (and
> >> some more).
> >>
> >> Initially I started to implement this functionality via sysfs but I
> >> run into the issues mentioned above and decided to use configfs.
> > [Lv Zheng]
> > I can sense different difficulties from your descriptions.
> > Let me break it down into details.
> >
> > We already have acpi_table_handler working there for creating new ACPI
> table entries for us.
> > Based on this facility, let's think about the following solution:
> > 1. sysfs presenting change
> > We can change the table file to a table directory whose name is in the
> following format:
> > TableSignature-OemId-OemTableId
> > Then we can get rid of the annoying numbered table name suffix first.
> > The numbered table name suffix cannot be kept consistent to reflect the real
> index if we allow tables to be dynamically loaded/unloaded.
> >
> > This is the first design difficulty we need to solve.
> >
> > 2. acpi_table_handler change
> > Now we can append 2 new events to acpi_table_handler -
> ACPI_TABLE_INSTALL/ACPI_TABLE_UNINSTALL.
> > With which, the sysfs entries can be created/deleted when the table is added
> to/removed from the global table list.
> > And this should be the working mechanism for us
> > So we actually don't have the trouble to deal with the new kernel object
> creation/deletion from sysfs.
> >
> > I agree the dynamic kernel object creation/deletion need special care.
> > But this actually is what a kernel engineer should do because this kind of
> things happen here and there throughout the kernel.
> > We should have already been used to that.
> >
> > This is the second engineering difficulty we need to face.
> >
> > 3. load/unload commanding
> > Now we need a character device in sysfs to handle load/unload command.
> > Well, there are many such kind of files in sysfs, for example, device nodes.
> > So this is not a non-achievable task, but just a difficult engineering task.
> > The system engineers need to be skillful enough to implement this.
> > Like the dynamic kernel object handling, we should have already been used to
> this.
> >
> >
> > If you still think this is difficult, the alternative choice is to use acpidbg char
> device's ioctl interface.
> > That could simplifies this task.
> > And since the ioctl interface is required by ACPICA disassembler, the work on
> that will be inherited by the disassembler porting.
> >
>
> Why can't the dissembler access the tables through the existing sysfs interface?
[Lv Zheng]
The acpidbg utility should be self-contained.
It will be back ported to ACPICA.
So it might be ported to other OSPMs.
>From this point of view, acpidbg need such kind of design - doing everything it need from an OSPM kernel using a single char device.
Thanks and best regards
-Lv
>
> > This is the last engineering difficulty we need to face.
> >
> > So why can't these solutions work for us?
> >
>
> It can be done, but it is not the right way to do it, IMO.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html