From 636c19d38920caee61d694ef306d110d33935038 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 9 Sep 2015 23:59:41 +0200 Subject: [PATCH 01/10] ACPI / scan: constify first argument of struct acpi_scan_handler::match One wouldn't expect a "match" function modify the string it searches for, and indeed the only instance of the struct acpi_scan_handler::match callback, acpi_pnp_match, can easily be changed. While there, update its helper matching_id(). This is also preparation for constifying struct acpi_hardware_id::id. Signed-off-by: Rasmus Villemoes Signed-off-by: Rafael J. Wysocki --- drivers/acpi/acpi_pnp.c | 4 ++-- include/acpi/acpi_bus.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c index c58940b231d6..48fc3ad13a4b 100644 --- a/drivers/acpi/acpi_pnp.c +++ b/drivers/acpi/acpi_pnp.c @@ -316,7 +316,7 @@ static const struct acpi_device_id acpi_pnp_device_ids[] = { {""}, }; -static bool matching_id(char *idstr, char *list_id) +static bool matching_id(const char *idstr, const char *list_id) { int i; @@ -333,7 +333,7 @@ static bool matching_id(char *idstr, char *list_id) return true; } -static bool acpi_pnp_match(char *idstr, const struct acpi_device_id **matchid) +static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matchid) { const struct acpi_device_id *devid; diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 5ba8fb64f664..9a1b46c64fc1 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -129,7 +129,7 @@ static inline struct acpi_hotplug_profile *to_acpi_hotplug_profile( struct acpi_scan_handler { const struct acpi_device_id *ids; struct list_head list_node; - bool (*match)(char *idstr, const struct acpi_device_id **matchid); + bool (*match)(const char *idstr, const struct acpi_device_id **matchid); int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id); void (*detach)(struct acpi_device *dev); void (*bind)(struct device *phys_dev); From 844142c3f80c66fb2c311b118d60abdfe02322cb Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 9 Sep 2015 23:59:42 +0200 Subject: [PATCH 02/10] ACPI / scan: constify struct acpi_hardware_id::id This is preparation for using kstrdup_const to initialize that member. Signed-off-by: Rasmus Villemoes Signed-off-by: Rafael J. Wysocki --- drivers/acpi/scan.c | 4 ++-- drivers/pnp/pnpacpi/core.c | 4 ++-- include/acpi/acpi_bus.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 01136b879038..a3eaf2080707 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1472,7 +1472,7 @@ bool acpi_device_is_present(struct acpi_device *adev) } static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler, - char *idstr, + const char *idstr, const struct acpi_device_id **matchid) { const struct acpi_device_id *devid; @@ -1491,7 +1491,7 @@ static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler, return false; } -static struct acpi_scan_handler *acpi_scan_match_handler(char *idstr, +static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr, const struct acpi_device_id **matchid) { struct acpi_scan_handler *handler; diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c index 5153d1d69aee..9113876487ed 100644 --- a/drivers/pnp/pnpacpi/core.c +++ b/drivers/pnp/pnpacpi/core.c @@ -207,7 +207,7 @@ struct pnp_protocol pnpacpi_protocol = { }; EXPORT_SYMBOL(pnpacpi_protocol); -static char *__init pnpacpi_get_id(struct acpi_device *device) +static const char *__init pnpacpi_get_id(struct acpi_device *device) { struct acpi_hardware_id *id; @@ -222,7 +222,7 @@ static char *__init pnpacpi_get_id(struct acpi_device *device) static int __init pnpacpi_add_device(struct acpi_device *device) { struct pnp_dev *dev; - char *pnpid; + const char *pnpid; struct acpi_hardware_id *id; int error; diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 9a1b46c64fc1..fec002919b65 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -227,7 +227,7 @@ typedef char acpi_device_class[20]; struct acpi_hardware_id { struct list_head list; - char *id; + const char *id; }; struct acpi_pnp_type { From 6a0d12ef993c5291047d72e88d73d0aa2b967335 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Wed, 9 Sep 2015 23:59:43 +0200 Subject: [PATCH 03/10] ACPI / scan: use kstrdup_const() in acpi_add_id() Empirically, acpi_add_id is mostly called with string literals, so using kstrdup_const for initializing struct acpi_hardware_id::id saves a little run-time memory and a string copy. Signed-off-by: Rasmus Villemoes Signed-off-by: Rafael J. Wysocki --- drivers/acpi/scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index a3eaf2080707..afaac47eefb4 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1184,7 +1184,7 @@ static void acpi_add_id(struct acpi_device_pnp *pnp, const char *dev_id) if (!id) return; - id->id = kstrdup(dev_id, GFP_KERNEL); + id->id = kstrdup_const(dev_id, GFP_KERNEL); if (!id->id) { kfree(id); return; @@ -1322,7 +1322,7 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp) struct acpi_hardware_id *id, *tmp; list_for_each_entry_safe(id, tmp, &pnp->ids, list) { - kfree(id->id); + kfree_const(id->id); kfree(id); } kfree(pnp->unique_id); From c33cab60bc0e05ddf1ef1b2cba8d16dd010f600a Mon Sep 17 00:00:00 2001 From: Rami Rosen Date: Thu, 10 Sep 2015 23:48:09 +0300 Subject: [PATCH 04/10] ACPI: change init_acpi_device_notify() to return void This patch changes the type of the return value of the init_acpi_device_notify() method to be void, as this method never fails and its return value is never used. Signed-off-by: Rami Rosen Signed-off-by: Rafael J. Wysocki --- drivers/acpi/glue.c | 5 ++--- drivers/acpi/internal.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c index b9657af751d1..1470ae4f98c0 100644 --- a/drivers/acpi/glue.c +++ b/drivers/acpi/glue.c @@ -351,13 +351,12 @@ static int acpi_platform_notify_remove(struct device *dev) return 0; } -int __init init_acpi_device_notify(void) +void __init init_acpi_device_notify(void) { if (platform_notify || platform_notify_remove) { printk(KERN_ERR PREFIX "Can't use platform_notify\n"); - return 0; + return; } platform_notify = acpi_platform_notify; platform_notify_remove = acpi_platform_notify_remove; - return 0; } diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 9e426210c2a8..e72c6a67201f 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -21,7 +21,7 @@ #define PREFIX "ACPI: " acpi_status acpi_os_initialize1(void); -int init_acpi_device_notify(void); +void init_acpi_device_notify(void); int acpi_scan_init(void); void acpi_pci_root_init(void); void acpi_pci_link_init(void); From f934c745079ea3a15e8619350faf681dad42cc91 Mon Sep 17 00:00:00 2001 From: Rami Rosen Date: Fri, 11 Sep 2015 17:49:22 +0300 Subject: [PATCH 05/10] ACPI: change acpi_sleep_proc_init() to return void This patch changes the type of the return value of the acpi_sleep_proc_init() method to be void, as this method never fails and its return value is never used. Signed-off-by: Rami Rosen \ [ rjw : Fixed up the static inline stub ] Signed-off-by: Rafael J. Wysocki --- drivers/acpi/internal.h | 4 ++-- drivers/acpi/proc.c | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index e72c6a67201f..c31787bef2d3 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -179,13 +179,13 @@ static inline int acpi_sleep_init(void) { return -ENXIO; } #endif #ifdef CONFIG_ACPI_SLEEP -int acpi_sleep_proc_init(void); +void acpi_sleep_proc_init(void); int suspend_nvs_alloc(void); void suspend_nvs_free(void); int suspend_nvs_save(void); void suspend_nvs_restore(void); #else -static inline int acpi_sleep_proc_init(void) { return 0; } +static inline void acpi_sleep_proc_init(void) {} static inline int suspend_nvs_alloc(void) { return 0; } static inline void suspend_nvs_free(void) {} static inline int suspend_nvs_save(void) { return 0; } diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c index 75c28eae8860..2a358154b770 100644 --- a/drivers/acpi/proc.c +++ b/drivers/acpi/proc.c @@ -144,11 +144,9 @@ static const struct file_operations acpi_system_wakeup_device_fops = { .release = single_release, }; -int __init acpi_sleep_proc_init(void) +void __init acpi_sleep_proc_init(void) { /* 'wakeup device' [R/W] */ proc_create("wakeup", S_IFREG | S_IRUGO | S_IWUSR, acpi_root_dir, &acpi_system_wakeup_device_fops); - - return 0; } From 0700c047f66101c7d0d23e3583f1ae9c3c2dd71c Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Thu, 24 Sep 2015 14:54:48 +0800 Subject: [PATCH 06/10] ACPI / EC: Fix query handler related issues 1. acpi_ec_remove_query_handlers() This patch refines the query handler removal logic implemented in acpi_ec_remove_query_handler(), making it to invoke new acpi_ec_remove_query_handlers() API, and ensuring all other removal code paths to invoke the new API to honor the reference count of the query handlers. 2. acpi_ec_get_query_handler_by_value() This patch also refines the query handler search logic originally implemented in acpi_ec_query(), collecting it into acpi_ec_get_query_handler_by_value(). And since schedule_work() can ensure the serilization of acpi_ec_event_handler(), we needn't put the mutex_lock() around schedule_work(). Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 73 +++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 42c66b64c12c..2db0912bb2e6 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -929,6 +929,23 @@ acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler) return handler; } +static struct acpi_ec_query_handler * +acpi_ec_get_query_handler_by_value(struct acpi_ec *ec, u8 value) +{ + struct acpi_ec_query_handler *handler; + bool found = false; + + mutex_lock(&ec->mutex); + list_for_each_entry(handler, &ec->list, node) { + if (value == handler->query_bit) { + found = true; + break; + } + } + mutex_unlock(&ec->mutex); + return found ? acpi_ec_get_query_handler(handler) : NULL; +} + static void acpi_ec_query_handler_release(struct kref *kref) { struct acpi_ec_query_handler *handler = @@ -964,14 +981,15 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit, } EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler); -void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit) +static void acpi_ec_remove_query_handlers(struct acpi_ec *ec, + bool remove_all, u8 query_bit) { struct acpi_ec_query_handler *handler, *tmp; LIST_HEAD(free_list); mutex_lock(&ec->mutex); list_for_each_entry_safe(handler, tmp, &ec->list, node) { - if (query_bit == handler->query_bit) { + if (remove_all || query_bit == handler->query_bit) { list_del_init(&handler->node); list_add(&handler->node, &free_list); } @@ -980,6 +998,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit) list_for_each_entry_safe(handler, tmp, &free_list, node) acpi_ec_put_query_handler(handler); } + +void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit) +{ + acpi_ec_remove_query_handlers(ec, false, query_bit); +} EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler); static struct acpi_ec_query *acpi_ec_create_query(u8 *pval) @@ -1025,7 +1048,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data) { u8 value = 0; int result; - struct acpi_ec_query_handler *handler; struct acpi_ec_query *q; q = acpi_ec_create_query(&value); @@ -1043,25 +1065,26 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data) if (result) goto err_exit; - mutex_lock(&ec->mutex); - result = -ENODATA; - list_for_each_entry(handler, &ec->list, node) { - if (value == handler->query_bit) { - result = 0; - q->handler = acpi_ec_get_query_handler(handler); - ec_dbg_evt("Query(0x%02x) scheduled", - q->handler->query_bit); - /* - * It is reported that _Qxx are evaluated in a - * parallel way on Windows: - * https://bugzilla.kernel.org/show_bug.cgi?id=94411 - */ - if (!schedule_work(&q->work)) - result = -EBUSY; - break; - } + q->handler = acpi_ec_get_query_handler_by_value(ec, value); + if (!q->handler) { + result = -ENODATA; + goto err_exit; + } + + /* + * It is reported that _Qxx are evaluated in a parallel way on + * Windows: + * https://bugzilla.kernel.org/show_bug.cgi?id=94411 + * + * Put this log entry before schedule_work() in order to make + * it appearing before any other log entries occurred during the + * work queue execution. + */ + ec_dbg_evt("Query(0x%02x) scheduled", value); + if (!schedule_work(&q->work)) { + ec_dbg_evt("Query(0x%02x) overlapped", value); + result = -EBUSY; } - mutex_unlock(&ec->mutex); err_exit: if (result && q) @@ -1354,19 +1377,13 @@ static int acpi_ec_add(struct acpi_device *device) static int acpi_ec_remove(struct acpi_device *device) { struct acpi_ec *ec; - struct acpi_ec_query_handler *handler, *tmp; if (!device) return -EINVAL; ec = acpi_driver_data(device); ec_remove_handlers(ec); - mutex_lock(&ec->mutex); - list_for_each_entry_safe(handler, tmp, &ec->list, node) { - list_del(&handler->node); - kfree(handler); - } - mutex_unlock(&ec->mutex); + acpi_ec_remove_query_handlers(ec, true, 0); release_region(ec->data_addr, 1); release_region(ec->command_addr, 1); device->driver_data = NULL; From 611975470740b38084610e0a64ff4c6ff5aec89a Mon Sep 17 00:00:00 2001 From: Lv Zheng Date: Thu, 24 Sep 2015 14:54:54 +0800 Subject: [PATCH 07/10] ACPI / EC: Fix a race issue in acpi_ec_guard_event() In acpi_ec_guard_event(), EC transaction state machine variables should be checked with the EC spinlock locked. The bug doesn't trigger any real issue now because this bug can only occur when the ec_event_clearing=event mode is applied while there is no user currently using this mode. Signed-off-by: Lv Zheng Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 2db0912bb2e6..f61a7c834540 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -441,17 +441,31 @@ static void acpi_ec_complete_query(struct acpi_ec *ec) static bool acpi_ec_guard_event(struct acpi_ec *ec) { + bool guarded = true; + unsigned long flags; + + spin_lock_irqsave(&ec->lock, flags); + /* + * If firmware SCI_EVT clearing timing is "event", we actually + * don't know when the SCI_EVT will be cleared by firmware after + * evaluating _Qxx, so we need to re-check SCI_EVT after waiting an + * acceptable period. + * + * The guarding period begins when EC_FLAGS_QUERY_PENDING is + * flagged, which means SCI_EVT check has just been performed. + * But if the current transaction is ACPI_EC_COMMAND_QUERY, the + * guarding should have already been performed (via + * EC_FLAGS_QUERY_GUARDING) and should not be applied so that the + * ACPI_EC_COMMAND_QUERY transaction can be transitioned into + * ACPI_EC_COMMAND_POLL state immediately. + */ if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS || ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY || !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) || (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY)) - return false; - - /* - * Postpone the query submission to allow the firmware to proceed, - * we shouldn't check SCI_EVT before the firmware reflagging it. - */ - return true; + guarded = false; + spin_unlock_irqrestore(&ec->lock, flags); + return guarded; } static int ec_transaction_polled(struct acpi_ec *ec) @@ -597,6 +611,7 @@ static int ec_guard(struct acpi_ec *ec) unsigned long guard = usecs_to_jiffies(ec_polling_guard); unsigned long timeout = ec->timestamp + guard; + /* Ensure guarding period before polling EC status */ do { if (ec_busy_polling) { /* Perform busy polling */ @@ -606,11 +621,13 @@ static int ec_guard(struct acpi_ec *ec) } else { /* * Perform wait polling - * - * For SCI_EVT clearing timing of "event", - * performing guarding before re-checking the - * SCI_EVT. Otherwise, such guarding is not needed - * due to the old practices. + * 1. Wait the transaction to be completed by the + * GPE handler after the transaction enters + * ACPI_EC_COMMAND_POLL state. + * 2. A special guarding logic is also required + * for event clearing mode "event" before the + * transaction enters ACPI_EC_COMMAND_POLL + * state. */ if (!ec_transaction_polled(ec) && !acpi_ec_guard_event(ec)) @@ -620,7 +637,6 @@ static int ec_guard(struct acpi_ec *ec) guard)) return 0; } - /* Guard the register accesses for the polling modes */ } while (time_before(jiffies, timeout)); return -ETIME; } From 9b3fedde27d3d63055c43c05e8254e252e58ba48 Mon Sep 17 00:00:00 2001 From: Lukasz Anaczkowski Date: Wed, 9 Sep 2015 15:47:28 +0200 Subject: [PATCH 08/10] ACPI / tables: Add acpi_subtable_proc to ACPI table parsers ACPI subtable parsing needs to be extended to allow two or more handlers to be run in the same ACPI table walk, thus adding acpi_subtable_proc structure which stores () ACPI table id () handler that processes table () counter how many items has been processed and passing it to acpi_parse_entries_array() and acpi_table_parse_entries_array(). This is needed to fix CPU enumeration when APIC/X2APIC entries are interleaved. Signed-off-by: Lukasz Anaczkowski Reviewed-by: Marc Zyngier Tested-by: Marc Zyngier Signed-off-by: Rafael J. Wysocki --- drivers/acpi/tables.c | 93 ++++++++++++++++++++++++++++++++++--------- include/linux/acpi.h | 19 +++++++-- 2 files changed, 91 insertions(+), 21 deletions(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 17a6fa01a338..c1ff58de99e9 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -210,20 +210,39 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } -int __init -acpi_parse_entries(char *id, unsigned long table_size, - acpi_tbl_entry_handler handler, +/** + * acpi_parse_entries_array - for each proc_num find a suitable subtable + * + * @id: table id (for debugging purposes) + * @table_size: single entry size + * @table_header: where does the table start? + * @proc: array of acpi_subtable_proc struct containing entry id + * and associated handler with it + * @proc_num: how big proc is? + * @max_entries: how many entries can we process? + * + * For each proc_num find a subtable with proc->id and run proc->handler + * on it. Assumption is that there's only single handler for particular + * entry id. + * + * On success returns sum of all matching entries for all proc handlers. + * Otherwise, -ENODEV or -EINVAL is returned. + */ +static int __init +acpi_parse_entries_array(char *id, unsigned long table_size, struct acpi_table_header *table_header, - int entry_id, unsigned int max_entries) + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { struct acpi_subtable_header *entry; - int count = 0; unsigned long table_end; + int count = 0; + int i; if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!table_size) @@ -243,20 +262,27 @@ acpi_parse_entries(char *id, unsigned long table_size, while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < table_end) { - if (entry->type == entry_id - && (!max_entries || count < max_entries)) { - if (handler(entry, table_end)) + if (max_entries && count >= max_entries) + break; + + for (i = 0; i < proc_num; i++) { + if (entry->type != proc[i].id) + continue; + if (!proc->handler || proc[i].handler(entry, table_end)) return -EINVAL; - count++; + proc->count++; + break; } + if (i != proc_num) + count++; /* * If entry->length is 0, break from this loop to avoid * infinite loop. */ if (entry->length == 0) { - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id); + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); return -EINVAL; } @@ -266,17 +292,32 @@ acpi_parse_entries(char *id, unsigned long table_size, if (max_entries && count > max_entries) { pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n", - id, entry_id, count - max_entries, count); + id, proc->id, count - max_entries, count); } return count; } int __init -acpi_table_parse_entries(char *id, +acpi_parse_entries(char *id, + unsigned long table_size, + acpi_tbl_entry_handler handler, + struct acpi_table_header *table_header, + int entry_id, unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; + + return acpi_parse_entries_array(id, table_size, table_header, + &proc, 1, max_entries); +} + +int __init +acpi_table_parse_entries_array(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, + struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_table_header *table_header = NULL; @@ -287,7 +328,7 @@ acpi_table_parse_entries(char *id, if (acpi_disabled) return -ENODEV; - if (!id || !handler) + if (!id) return -EINVAL; if (!strncmp(id, ACPI_SIG_MADT, 4)) @@ -299,13 +340,29 @@ acpi_table_parse_entries(char *id, return -ENODEV; } - count = acpi_parse_entries(id, table_size, handler, table_header, - entry_id, max_entries); + count = acpi_parse_entries_array(id, table_size, table_header, + proc, proc_num, max_entries); early_acpi_os_unmap_memory((char *)table_header, tbl_size); return count; } +int __init +acpi_table_parse_entries(char *id, + unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries) +{ + struct acpi_subtable_proc proc = { + .id = entry_id, + .handler = handler, + }; + + return acpi_table_parse_entries_array(id, table_size, &proc, 1, + max_entries); +} + int __init acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 7235c4851460..b0299f8db660 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -131,6 +131,12 @@ static inline void acpi_initrd_override(void *data, size_t size) (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) +struct acpi_subtable_proc { + int id; + acpi_tbl_entry_handler handler; + int count; +}; + char * __acpi_map_table (unsigned long phys_addr, unsigned long size); void __acpi_unmap_table(char *map, unsigned long size); int early_acpi_boot_init(void); @@ -146,9 +152,16 @@ int __init acpi_parse_entries(char *id, unsigned long table_size, struct acpi_table_header *table_header, int entry_id, unsigned int max_entries); int __init acpi_table_parse_entries(char *id, unsigned long table_size, - int entry_id, - acpi_tbl_entry_handler handler, - unsigned int max_entries); + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries); +int __init acpi_table_parse_entries(char *id, unsigned long table_size, + int entry_id, + acpi_tbl_entry_handler handler, + unsigned int max_entries); +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries); int acpi_table_parse_madt(enum acpi_madt_type id, acpi_tbl_entry_handler handler, unsigned int max_entries); From d81056b5278c520f1dede99bd59c14366ac8b1e1 Mon Sep 17 00:00:00 2001 From: Lukasz Anaczkowski Date: Wed, 9 Sep 2015 15:47:29 +0200 Subject: [PATCH 09/10] x86, ACPI: Handle apic/x2apic entries in MADT in correct order ACPI specifies the following rules when listing APIC IDs: (1) Boot processor is listed first (2) For multi-threaded processors, BIOS should list the first logical processor of each of the individual multi-threaded processors in MADT before listing any of the second logical processors. (3) APIC IDs < 0xFF should be listed in APIC subtable, APIC IDs >= 0xFF should be listed in X2APIC subtable Because of above, when there's more than 0xFF logical CPUs, BIOS interleaves APIC/X2APIC subtables. Assuming, there's 72 cores, 72 hyper-threads each, 288 CPUs total, listing is like this: APIC (0,4,8, .., 252) X2APIC (258,260,264, .. 284) APIC (1,5,9,...,253) X2APIC (259,261,265,...,285) APIC (2,6,10,...,254) X2APIC (260,262,266,..,286) APIC (3,7,11,...,251) X2APIC (255,261,262,266,..,287) Now, before this patch, due to how ACPI MADT subtables were parsed (BSP then X2APIC then APIC), kernel enumerated CPUs in reverted order (i.e. high APIC IDs were getting low logical IDs, and low APIC IDs were getting high logical IDs). This is wrong for the following reasons: () it's hard to predict how cores and threads are enumerated () when it's hard to predict, s/w threads cannot be properly affinitized causing significant performance impact due to e.g. inproper cache sharing () enumeration is inconsistent with how threads are enumerated on other Intel Xeon processors So, order in which MADT APIC/X2APIC handlers are passed is reverse and both handlers are passed to be called during same MADT table to walk to achieve correct CPU enumeration. In scenario when someone boots kernel with options 'maxcpus=72 nox2apic', in result less cores may be booted, since some of the CPUs the kernel will try to use will have APIC ID >= 0xFF. In such case, one should not pass 'nox2apic'. Disclimer: code parsing MADT APIC/X2APIC has not been touched since 2009, when X2APIC support was initially added. I do not know why MADT parsing code was added in the reversed order in the first place. I guess it didn't matter at that time since nobody cared about cores with APIC IDs >= 0xFF, right? This patch is based on work of "Yinghai Lu " previously published at https://lkml.org/lkml/2013/1/21/563 Here's the explanation why parsing interface needs to be changed and why simpler approach will not work https://lkml.org/lkml/2015/9/7/285 Signed-off-by: Lukasz Anaczkowski Acked-by: Thomas Gleixner (commit message) Signed-off-by: Rafael J. Wysocki --- arch/x86/kernel/acpi/boot.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index ded848c20e05..e75907601a41 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -976,6 +976,8 @@ static int __init acpi_parse_madt_lapic_entries(void) { int count; int x2count = 0; + int ret; + struct acpi_subtable_proc madt_proc[2]; if (!cpu_has_apic) return -ENODEV; @@ -999,10 +1001,22 @@ static int __init acpi_parse_madt_lapic_entries(void) acpi_parse_sapic, MAX_LOCAL_APIC); if (!count) { - x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC, - acpi_parse_x2apic, MAX_LOCAL_APIC); - count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC, - acpi_parse_lapic, MAX_LOCAL_APIC); + memset(madt_proc, 0, sizeof(madt_proc)); + madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC; + madt_proc[0].handler = acpi_parse_lapic; + madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC; + madt_proc[1].handler = acpi_parse_x2apic; + ret = acpi_table_parse_entries_array(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC); + if (ret < 0) { + printk(KERN_ERR PREFIX + "Error parsing LAPIC/X2APIC entries\n"); + return ret; + } + + x2count = madt_proc[0].count; + count = madt_proc[1].count; } if (!count && !x2count) { printk(KERN_ERR PREFIX "No LAPIC entries present\n"); From 362414d9d2824ee0f4910a73e880860a90e75c3c Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 22 Sep 2015 15:29:25 +0300 Subject: [PATCH 10/10] ACPI / tables: test the correct variable The intent was to test "proc[i].handler" instead of "proc->handler". Signed-off-by: Dan Carpenter Signed-off-by: Rafael J. Wysocki --- drivers/acpi/tables.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index c1ff58de99e9..6c0f0794aa82 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -268,7 +268,8 @@ acpi_parse_entries_array(char *id, unsigned long table_size, for (i = 0; i < proc_num; i++) { if (entry->type != proc[i].id) continue; - if (!proc->handler || proc[i].handler(entry, table_end)) + if (!proc[i].handler || + proc[i].handler(entry, table_end)) return -EINVAL; proc->count++;