From a9527fd686dcdd0b04c4eac6690531d945e7f221 Mon Sep 17 00:00:00 2001 From: Shawn Nematbakhsh Date: Mon, 27 Jul 2015 14:36:53 -0700 Subject: [PATCH] acpi: Ensure continuity of memmap data with a read cache For multi-byte ACPI memmap reads, we previously had a mutex to ensure data continuity. A better approach is to use a read cache. Since the kernel will enable burst mode before reading a multi-byte memmap variable and disable it afterward, we can populate the cache on the first read after enabling burst. This solution removes deadlock bugs, is contained entirely in acpi.c, and saves a deferred function. BUG=chromium:514283 TEST=Manual on Glados. Add prints in acpi_read, verify that multi-byte reads come from cache and non-burst reads continue to function as before. BRANCH=Cyan Signed-off-by: Shawn Nematbakhsh Change-Id: I74e4927bf2b433e31a9ff65d72820fa087c51722 Reviewed-on: https://chromium-review.googlesource.com/288871 Reviewed-by: Bill Richardson Reviewed-by: Vincent Palatin --- board/glados/board.h | 2 +- board/kunimitsu/board.h | 2 +- board/rambi/board.h | 2 +- board/samus/board.h | 2 +- common/acpi.c | 113 +++++++++++++++++++++++++-------------- common/als.c | 2 - common/charge_state_v2.c | 18 ------- common/fan.c | 3 -- common/host_command.c | 18 ------- include/host_command.h | 19 ------- 10 files changed, 76 insertions(+), 105 deletions(-) diff --git a/board/glados/board.h b/board/glados/board.h index d75b2a26e0..603f55f694 100644 --- a/board/glados/board.h +++ b/board/glados/board.h @@ -113,7 +113,7 @@ #endif #undef DEFERRABLE_MAX_COUNT -#define DEFERRABLE_MAX_COUNT 13 +#define DEFERRABLE_MAX_COUNT 12 #ifndef __ASSEMBLER__ diff --git a/board/kunimitsu/board.h b/board/kunimitsu/board.h index e132357a55..1c4e4fbcdc 100644 --- a/board/kunimitsu/board.h +++ b/board/kunimitsu/board.h @@ -87,7 +87,7 @@ #define I2C_PORT_USB_CHARGER_2 MEC1322_I2C3 #undef DEFERRABLE_MAX_COUNT -#define DEFERRABLE_MAX_COUNT 13 +#define DEFERRABLE_MAX_COUNT 12 #define CONFIG_ALS #define CONFIG_ALS_OPT3001 diff --git a/board/rambi/board.h b/board/rambi/board.h index d8255734bc..7568aae85f 100644 --- a/board/rambi/board.h +++ b/board/rambi/board.h @@ -49,7 +49,7 @@ (EC_WIRELESS_SWITCH_WLAN | EC_WIRELESS_SWITCH_WLAN_POWER) #undef DEFERRABLE_MAX_COUNT -#define DEFERRABLE_MAX_COUNT 9 +#define DEFERRABLE_MAX_COUNT 8 #ifndef __ASSEMBLER__ diff --git a/board/samus/board.h b/board/samus/board.h index d35ffee5f7..3acd4bc427 100644 --- a/board/samus/board.h +++ b/board/samus/board.h @@ -82,7 +82,7 @@ /* Do we want EC_WIRELESS_SWITCH_WWAN as well? */ #undef DEFERRABLE_MAX_COUNT -#define DEFERRABLE_MAX_COUNT 10 +#define DEFERRABLE_MAX_COUNT 9 #ifndef __ASSEMBLER__ diff --git a/common/acpi.c b/common/acpi.c index b5cb763c40..e12c4e2d18 100644 --- a/common/acpi.c +++ b/common/acpi.c @@ -13,6 +13,7 @@ #include "ec_commands.h" #include "pwm.h" #include "timer.h" +#include "util.h" /* Console output macros */ #define CPUTS(outstr) cputs(CC_LPC, outstr) @@ -29,28 +30,77 @@ static int dptf_temp_sensor_id; /* last sensor ID written */ static int dptf_temp_threshold; /* last threshold written */ #endif +/* + * Keep a read cache of four bytes when burst mode is enabled, which is the + * size of the largest non-string memmap data type. + */ +#define ACPI_READ_CACHE_SIZE 4 + +/* Start address that indicates read cache is flushed. */ +#define ACPI_READ_CACHE_FLUSHED (EC_ACPI_MEM_MAPPED_BEGIN - 1) + +/* Calculate size of valid cache based upon end of memmap data. */ +#define ACPI_VALID_CACHE_SIZE(addr) (MIN( \ + EC_ACPI_MEM_MAPPED_SIZE + EC_ACPI_MEM_MAPPED_BEGIN - (addr), \ + ACPI_READ_CACHE_SIZE)) + +/* + * In burst mode, read the requested memmap data and the data immediately + * following it into a cache. For future reads in burst mode, try to grab + * data from the cache. This ensures the continuity of multi-byte reads, + * which is important when dealing with data types > 8 bits. + */ +static struct { + int enabled; + uint8_t start_addr; + uint8_t data[ACPI_READ_CACHE_SIZE]; +} acpi_read_cache; + /* * Deferred function to ensure that ACPI burst mode doesn't remain enabled * indefinitely. */ -static void acpi_unlock_memmap_deferred(void) +static void acpi_disable_burst_deferred(void) { + acpi_read_cache.enabled = 0; lpc_clear_acpi_status_mask(EC_LPC_STATUS_BURST_MODE); - host_unlock_memmap(); - CPUTS("ACPI force unlock mutex, missed burst disable?"); + CPUTS("ACPI missed burst disable?"); } -DECLARE_DEFERRED(acpi_unlock_memmap_deferred); +DECLARE_DEFERRED(acpi_disable_burst_deferred); -/* - * Deferred function to lock memmap and to enable LPC interrupts. - * This is due to LPC interrupt was unable to acquire memmap mutex. - */ -static void deferred_host_lock_memmap(void) +/* Read memmapped data, returns read data or 0xff on error. */ +static int acpi_read(uint8_t addr) { - host_lock_memmap(); - lpc_enable_acpi_interrupts(); + uint8_t *memmap_addr = (uint8_t *)(lpc_get_memmap_range() + addr - + EC_ACPI_MEM_MAPPED_BEGIN); + + /* Check for out-of-range read. */ + if (addr < EC_ACPI_MEM_MAPPED_BEGIN || + addr >= EC_ACPI_MEM_MAPPED_BEGIN + EC_ACPI_MEM_MAPPED_SIZE) { + CPRINTS("ACPI read 0x%02x (ignored)", + acpi_addr); + return 0xff; + } + + /* Read from cache if enabled (burst mode). */ + if (acpi_read_cache.enabled) { + /* Fetch to cache on miss. */ + if (acpi_read_cache.start_addr == ACPI_READ_CACHE_FLUSHED || + acpi_read_cache.start_addr > addr || + addr - acpi_read_cache.start_addr >= + ACPI_READ_CACHE_SIZE) { + memcpy(acpi_read_cache.data, + memmap_addr, + ACPI_VALID_CACHE_SIZE(addr)); + acpi_read_cache.start_addr = addr; + } + /* Return data from cache. */ + return acpi_read_cache.data[addr - acpi_read_cache.start_addr]; + } else { + /* Read directly from memmap data. */ + return *memmap_addr; + } } -DECLARE_DEFERRED(deferred_host_lock_memmap); /* * This handles AP writes to the EC via the ACPI I/O port. There are only a few @@ -114,15 +164,7 @@ int acpi_ap_to_ec(int is_cmd, uint8_t value, uint8_t *resultptr) break; #endif default: - if (acpi_addr >= EC_ACPI_MEM_MAPPED_BEGIN && - acpi_addr < - EC_ACPI_MEM_MAPPED_BEGIN + EC_ACPI_MEM_MAPPED_SIZE) - result = *((uint8_t *)(lpc_get_memmap_range() + - acpi_addr - - EC_ACPI_MEM_MAPPED_BEGIN)); - else - CPRINTS("ACPI read 0x%02x (ignored)", - acpi_addr); + result = acpi_read(acpi_addr); break; } @@ -196,40 +238,29 @@ int acpi_ap_to_ec(int is_cmd, uint8_t value, uint8_t *resultptr) * value reads over the ACPI port. We don't do such reads * when our memmap data can be accessed directly over LPC, * so on LM4, for example, this is dead code. We might want - * to re-add the CONFIG, now that we have overhead of one - * deferred function. + * to add a config to skip this code for certain chips. */ - if (host_memmap_is_locked()) { - /* - * If already locked by a task, we can not acquire - * the mutex and will have to wait in the interrupt - * context. But then the task will not get chance to - * release the mutex. This will create deadlock - * situation. To avoid the deadlock, disable ACPI - * interrupts and defer locking. - */ - lpc_disable_acpi_interrupts(); - hook_call_deferred(deferred_host_lock_memmap, 0); - } else { - host_lock_memmap(); - } + acpi_read_cache.enabled = 1; + acpi_read_cache.start_addr = ACPI_READ_CACHE_FLUSHED; + /* Enter burst mode */ lpc_set_acpi_status_mask(EC_LPC_STATUS_BURST_MODE); /* - * Unlock from deferred function in case burst mode is enabled + * Disable from deferred function in case burst mode is enabled * for an extremely long time (ex. kernel bug / crash). */ - hook_call_deferred(acpi_unlock_memmap_deferred, 1*SECOND); + hook_call_deferred(acpi_disable_burst_deferred, 1*SECOND); /* ACPI 5.0-12.3.3: Burst ACK */ *resultptr = 0x90; retval = 1; } else if (acpi_cmd == EC_CMD_ACPI_BURST_DISABLE && !acpi_data_count) { + acpi_read_cache.enabled = 0; + /* Leave burst mode */ - hook_call_deferred(acpi_unlock_memmap_deferred, -1); + hook_call_deferred(acpi_disable_burst_deferred, -1); lpc_clear_acpi_status_mask(EC_LPC_STATUS_BURST_MODE); - host_unlock_memmap(); } return retval; diff --git a/common/als.c b/common/als.c index 0ae06566aa..fac97e9944 100644 --- a/common/als.c +++ b/common/als.c @@ -30,9 +30,7 @@ void als_task(void) while (1) { for (i = 0; i < EC_ALS_ENTRIES && i < ALS_COUNT; i++) { als_data = als_read(i, &val) == EC_SUCCESS ? val : 0; - host_lock_memmap(); mapped[i] = als_data; - host_unlock_memmap(); } task_wait_event(SECOND); diff --git a/common/charge_state_v2.c b/common/charge_state_v2.c index d588ae3362..0ca0974739 100644 --- a/common/charge_state_v2.c +++ b/common/charge_state_v2.c @@ -124,12 +124,6 @@ static int update_static_battery_info(void) */ int rv; - /* - * We're updating multi-byte memmap vars, don't allow ACPI to do - * reads while we're updating. - */ - host_lock_memmap(); - /* Smart battery serial number is 16 bits */ batt_str = (char *)host_get_memmap(EC_MEMMAP_BATT_SERIAL); memset(batt_str, 0, EC_MEMMAP_TEXT_MAX); @@ -173,9 +167,6 @@ static int update_static_battery_info(void) *(int *)host_get_memmap(EC_MEMMAP_BATT_LFCC) = 0; *host_get_memmap(EC_MEMMAP_BATT_FLAG) = 0; - /* No more multi-byte memmap writes. */ - host_unlock_memmap(); - if (rv) problem(PR_STATIC_UPDATE, rv); else @@ -220,12 +211,6 @@ static void update_dynamic_battery_info(void) batt_present = 0; } - /* - * We're updating multi-byte memmap vars, don't allow ACPI to do - * reads while we're updating. - */ - host_lock_memmap(); - if (!(curr.batt.flags & BATT_FLAG_BAD_VOLTAGE)) *memmap_volt = curr.batt.voltage; @@ -252,9 +237,6 @@ static void update_dynamic_battery_info(void) send_batt_info_event++; } - /* No more multi-byte memmap writes. */ - host_unlock_memmap(); - if (curr.batt.is_present == BP_YES && !(curr.batt.flags & BATT_FLAG_BAD_STATE_OF_CHARGE) && curr.batt.state_of_charge <= BATTERY_LEVEL_CRITICAL) diff --git a/common/fan.c b/common/fan.c index 6cb64e777a..9c2a7c587e 100644 --- a/common/fan.c +++ b/common/fan.c @@ -484,10 +484,7 @@ static void pwm_fan_second(void) rpm = fan_get_rpm_actual(fans[fan].ch); } - /* Lock ACPI read access to memmap during multi-byte write */ - host_lock_memmap(); mapped[fan] = rpm; - host_unlock_memmap(); } /* diff --git a/common/host_command.c b/common/host_command.c index 778eeab369..387f0d03a2 100644 --- a/common/host_command.c +++ b/common/host_command.c @@ -81,24 +81,6 @@ uint8_t *host_get_memmap(int offset) #endif } -static struct mutex memmap_write_mutex; - -void host_lock_memmap() -{ - mutex_lock(&memmap_write_mutex); -} - -/* Returns host memmap lock status */ -int host_memmap_is_locked(void) -{ - return (memmap_write_mutex.lock != 0); -} - -void host_unlock_memmap() -{ - mutex_unlock(&memmap_write_mutex); -} - int host_get_vboot_mode(void) { return g_vboot_mode; diff --git a/include/host_command.h b/include/host_command.h index 0d1ff998f3..08141e2204 100644 --- a/include/host_command.h +++ b/include/host_command.h @@ -123,25 +123,6 @@ struct host_command { */ uint8_t *host_get_memmap(int offset); -/** - * Grab the memmap write mutex. This function should be called before - * multi-byte variable reads from ACPI, and before updating multi-byte - * memmap variables anywhere else. - */ -void host_lock_memmap(void); - -/* - * Returns status of host memmap lock. - */ -int host_memmap_is_locked(void); - -/** - * Release the memmap write mutex. This function should be called once - * a multi-byte variable read from ACPI is done, and when updating multi-byte - * memmap variables is done. - */ -void host_unlock_memmap(void); - /** * Process a host command and return its response *