From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Starikovskiy Subject: Re: patch for bugs 9998 and 10724 Date: Thu, 04 Sep 2008 16:35:21 +0400 Message-ID: <48BFD609.2050604@suse.de> References: <1219730254.4116.38.camel@yakui_zhao.sh.intel.com> <20080904121820.GC18288@one.firstfloor.org> <48BFD5AA.3090006@suse.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010100020106080505030701" Return-path: Received: from charybdis-ext.suse.de ([195.135.221.2]:57044 "EHLO emea5-mh.id5.novell.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751814AbYIDMez (ORCPT ); Thu, 4 Sep 2008 08:34:55 -0400 In-Reply-To: <48BFD5AA.3090006@suse.de> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Andi Kleen Cc: linux-acpi@vger.kernel.org, Len Brown This is a multi-part message in MIME format. --------------010100020106080505030701 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Here is the same patch as attachment for your convenience. Alexey Starikovskiy wrote: > Hi Andy, > > I just uploaded patch for bugs 9998 and 10724, which solves the problem > of interrupt storm more cleanly. > Could you please add this patch to testing? > > Thanks, > Alex. > > --------------010100020106080505030701 Content-Type: text/x-diff; name="fast_transaction.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="fast_transaction.patch" ACPI: EC: do transaction from interrupt context From: Alexey Starikovskiy It is easier and faster to do transaction directly from interrupt context rather than waking control thread. Also, cleaner GPE storm avoidance is implemented. Signed-off-by: Alexey Starikovskiy --- drivers/acpi/ec.c | 250 ++++++++++++++++++++++++----------------------------- 1 files changed, 113 insertions(+), 137 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 13593f9..1f96f0e 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -1,7 +1,7 @@ /* - * ec.c - ACPI Embedded Controller Driver (v2.0) + * ec.c - ACPI Embedded Controller Driver (v2.1) * - * Copyright (C) 2006, 2007 Alexey Starikovskiy + * Copyright (C) 2006-2008 Alexey Starikovskiy * Copyright (C) 2006 Denis Sadykov * Copyright (C) 2004 Luming Yu * Copyright (C) 2001, 2002 Andy Grover @@ -26,7 +26,7 @@ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ -/* Uncomment next line to get verbose print outs*/ +/* Uncomment next line to get verbose printout */ /* #define DEBUG */ #include @@ -65,22 +65,18 @@ enum ec_command { ACPI_EC_COMMAND_QUERY = 0x84, }; -/* EC events */ -enum ec_event { - ACPI_EC_EVENT_OBF_1 = 1, /* Output buffer full */ - ACPI_EC_EVENT_IBF_0, /* Input buffer empty */ -}; - #define ACPI_EC_DELAY 500 /* Wait 500ms max. during EC ops */ #define ACPI_EC_UDELAY_GLK 1000 /* Wait 1ms max. to get global lock */ #define ACPI_EC_UDELAY 100 /* Wait 100us before polling EC again */ +#define ACPI_EC_STORM_THRESHOLD 20 /* number of false interrupts + per one transaction */ + enum { - EC_FLAGS_WAIT_GPE = 0, /* Don't check status until GPE arrives */ EC_FLAGS_QUERY_PENDING, /* Query is pending */ EC_FLAGS_GPE_MODE, /* Expect GPE to be sent for status change */ EC_FLAGS_NO_GPE, /* Don't use GPE mode */ - EC_FLAGS_RESCHEDULE_POLL /* Re-schedule poll */ + EC_FLAGS_GPE_STORM /* GPE storm detected */ }; /* If we find an EC via the ECDT, we need to keep a ptr to its context */ @@ -95,6 +91,14 @@ struct acpi_ec_query_handler { u8 query_bit; }; +struct transaction_data { + const u8 *wdata; + u8 *rdata; + u8 command; + u8 wlen; + u8 rlen; +}; + static struct acpi_ec { acpi_handle handle; unsigned long gpe; @@ -105,7 +109,7 @@ static struct acpi_ec { struct mutex lock; wait_queue_head_t wait; struct list_head list; - struct delayed_work work; + struct transaction_data t; atomic_t irq_count; u8 handlers_installed; } *boot_ec, *first_ec; @@ -150,13 +154,13 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec) { u8 x = inb(ec->data_addr); pr_debug(PREFIX "---> data = 0x%2.2x\n", x); - return inb(ec->data_addr); + return x; } static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command) { pr_debug(PREFIX "<--- command = 0x%2.2x\n", command); - outb(command, ec->command_addr); + outb((ec->t.command = command), ec->command_addr); } static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data) @@ -165,69 +169,60 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data) outb(data, ec->data_addr); } -static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event) -{ - if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags)) - return 0; - if (event == ACPI_EC_EVENT_OBF_1) { - if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF) - return 1; - } else if (event == ACPI_EC_EVENT_IBF_0) { - if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF)) - return 1; - } - +static int ec_transaction_done(struct acpi_ec *ec) { + if (!ec->t.command) + return 1; + if (!ec->t.wlen && !ec->t.rlen) + return 1; return 0; } -static void ec_schedule_ec_poll(struct acpi_ec *ec) +static void gpe_transaction(struct acpi_ec *ec, u8 status) { - if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags)) - schedule_delayed_work(&ec->work, - msecs_to_jiffies(ACPI_EC_DELAY)); + if (!ec->t.command) + return; + if (ec->t.wlen > 0) { + if ((status & ACPI_EC_FLAG_IBF) == 0) { + acpi_ec_write_data(ec, *(ec->t.wdata++)); + --ec->t.wlen; + } else + /* false interrupt, state didn't change */ + atomic_inc(&ec->irq_count); + + } else if (ec->t.rlen > 0) { + if ((status & ACPI_EC_FLAG_OBF) == 1) { + *(ec->t.rdata++) = acpi_ec_read_data(ec); + --ec->t.rlen; + } else + /* false interrupt, state didn't change */ + atomic_inc(&ec->irq_count); + } } -static void ec_switch_to_poll_mode(struct acpi_ec *ec) +static int acpi_ec_wait(struct acpi_ec *ec) { + if (wait_event_timeout(ec->wait, ec_transaction_done(ec), + msecs_to_jiffies(ACPI_EC_DELAY))) + return 0; + /* missing GPEs, switch back to poll mode */ + if (printk_ratelimit()) + pr_info(PREFIX "missing confirmations, " + "switch off interrupt mode.\n"); set_bit(EC_FLAGS_NO_GPE, &ec->flags); clear_bit(EC_FLAGS_GPE_MODE, &ec->flags); - acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR); - set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags); + return 1; } -static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll) +static void acpi_ec_gpe_query(void *ec_cxt); + +static int ec_check_sci(struct acpi_ec *ec, u8 state) { - atomic_set(&ec->irq_count, 0); - if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) && - likely(!force_poll)) { - if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event), - msecs_to_jiffies(ACPI_EC_DELAY))) - return 0; - clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags); - if (acpi_ec_check_status(ec, event)) { - /* missing GPEs, switch back to poll mode */ - if (printk_ratelimit()) - pr_info(PREFIX "missing confirmations, " - "switch off interrupt mode.\n"); - ec_switch_to_poll_mode(ec); - ec_schedule_ec_poll(ec); - return 0; - } - } else { - unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); - clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags); - while (time_before(jiffies, delay)) { - if (acpi_ec_check_status(ec, event)) - return 0; - msleep(1); - } - if (acpi_ec_check_status(ec,event)) - return 0; + if (state & ACPI_EC_FLAG_SCI) { + if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) + return acpi_os_execute(OSL_EC_BURST_HANDLER, + acpi_ec_gpe_query, ec); } - pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = %s\n", - acpi_ec_read_status(ec), - (event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\""); - return -ETIME; + return 0; } static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command, @@ -235,45 +230,53 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command, u8 * rdata, unsigned rdata_len, int force_poll) { - int result = 0; - set_bit(EC_FLAGS_WAIT_GPE, &ec->flags); + unsigned long delay; pr_debug(PREFIX "transaction start\n"); - acpi_ec_write_cmd(ec, command); - for (; wdata_len > 0; --wdata_len) { - result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll); - if (result) { - pr_err(PREFIX - "write_cmd timeout, command = %d\n", command); - goto end; - } - set_bit(EC_FLAGS_WAIT_GPE, &ec->flags); - acpi_ec_write_data(ec, *(wdata++)); + /* disable GPE during transaction if storm is detected */ + if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { + set_bit(EC_FLAGS_NO_GPE, &ec->flags); + clear_bit(EC_FLAGS_GPE_MODE, &ec->flags); + acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR); } - - if (!rdata_len) { - result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll); - if (result) { - pr_err(PREFIX - "finish-write timeout, command = %d\n", command); - goto end; - } - } else if (command == ACPI_EC_COMMAND_QUERY) - clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); - - for (; rdata_len > 0; --rdata_len) { - result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, force_poll); - if (result) { - pr_err(PREFIX "read timeout, command = %d\n", command); - goto end; + atomic_set(&ec->irq_count, 0); + /* fill in transaction structure */ + ec->t.wdata = wdata; + ec->t.wlen = wdata_len; + ec->t.rdata = rdata; + ec->t.rlen = rdata_len; + /* start transaction */ + acpi_ec_write_cmd(ec, command); + /* if we selected poll mode or failed in GPE-mode do a poll loop */ + if (force_poll || + !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) || + acpi_ec_wait(ec)) { + delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); + while (time_before(jiffies, delay)) { + gpe_transaction(ec, acpi_ec_read_status(ec)); + msleep(1); + if (ec_transaction_done(ec)) + goto end; } - /* Don't expect GPE after last read */ - if (rdata_len > 1) - set_bit(EC_FLAGS_WAIT_GPE, &ec->flags); - *(rdata++) = acpi_ec_read_data(ec); } - end: +end: pr_debug(PREFIX "transaction end\n"); - return result; + ec->t.command = 0; + if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) { + /* check if we received SCI during transaction */ + ec_check_sci(ec, acpi_ec_read_status(ec)); + /* it is safe to enable GPE outside of transaction */ + acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR); + } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) && + atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD) + pr_debug(PREFIX "GPE storm detected\n"); + set_bit(EC_FLAGS_GPE_STORM, &ec->flags); + return 0; +} + +static int ec_check_ibf0(struct acpi_ec *ec) +{ + u8 status = acpi_ec_read_status(ec); + return (status & ACPI_EC_FLAG_IBF) == 0; } static int acpi_ec_transaction(struct acpi_ec *ec, u8 command, @@ -299,8 +302,8 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command, } } - status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0); - if (status) { + if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec), + msecs_to_jiffies(ACPI_EC_DELAY))) { pr_err(PREFIX "input buffer is not empty, " "aborting transaction\n"); goto end; @@ -437,6 +440,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data) */ result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0); + clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); if (result) return result; @@ -515,24 +519,17 @@ static u32 acpi_ec_gpe_handler(void *data) { acpi_status status = AE_OK; struct acpi_ec *ec = data; - u8 state = acpi_ec_read_status(ec); + u8 state; pr_debug(PREFIX "~~~> interrupt\n"); - atomic_inc(&ec->irq_count); - if (atomic_read(&ec->irq_count) > 5) { - pr_err(PREFIX "GPE storm detected, disabling EC GPE\n"); - ec_switch_to_poll_mode(ec); - goto end; - } - clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags); - if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) + state = acpi_ec_read_status(ec); + + gpe_transaction(ec, state); + if ((status & ACPI_EC_FLAG_IBF) == 0) wake_up(&ec->wait); - if (state & ACPI_EC_FLAG_SCI) { - if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) - status = acpi_os_execute(OSL_EC_BURST_HANDLER, - acpi_ec_gpe_query, ec); - } else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) && + status = ec_check_sci(ec, state); + if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) && !test_bit(EC_FLAGS_NO_GPE, &ec->flags) && in_interrupt()) { /* this is non-query, must be confirmation */ @@ -540,21 +537,11 @@ static u32 acpi_ec_gpe_handler(void *data) pr_info(PREFIX "non-query interrupt received," " switching to interrupt mode\n"); set_bit(EC_FLAGS_GPE_MODE, &ec->flags); - clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags); } -end: - ec_schedule_ec_poll(ec); return ACPI_SUCCESS(status) ? ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED; } -static void do_ec_poll(struct work_struct *work) -{ - struct acpi_ec *ec = container_of(work, struct acpi_ec, work.work); - atomic_set(&ec->irq_count, 0); - (void)acpi_ec_gpe_handler(ec); -} - /* -------------------------------------------------------------------------- Address Space Management -------------------------------------------------------------------------- */ @@ -696,7 +683,6 @@ static struct acpi_ec *make_acpi_ec(void) mutex_init(&ec->lock); init_waitqueue_head(&ec->wait); INIT_LIST_HEAD(&ec->list); - INIT_DELAYED_WORK_DEFERRABLE(&ec->work, do_ec_poll); atomic_set(&ec->irq_count, 0); return ec; } @@ -736,15 +722,8 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval) return AE_CTRL_TERMINATE; } -static void ec_poll_stop(struct acpi_ec *ec) -{ - clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags); - cancel_delayed_work(&ec->work); -} - static void ec_remove_handlers(struct acpi_ec *ec) { - ec_poll_stop(ec); if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler))) pr_err(PREFIX "failed to remove space handler\n"); @@ -849,14 +828,12 @@ static int ec_install_handlers(struct acpi_ec *ec) if (ec->handlers_installed) return 0; status = acpi_install_gpe_handler(NULL, ec->gpe, - ACPI_GPE_EDGE_TRIGGERED, - &acpi_ec_gpe_handler, ec); + ACPI_GPE_EDGE_TRIGGERED, + &acpi_ec_gpe_handler, ec); if (ACPI_FAILURE(status)) return -ENODEV; - acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME); acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR); - status = acpi_install_address_space_handler(ec->handle, ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, @@ -887,7 +864,6 @@ static int acpi_ec_start(struct acpi_device *device) /* EC is fully operational, allow queries */ clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags); - ec_schedule_ec_poll(ec); return ret; } --------------010100020106080505030701--