RE: 2.6.0-test9 regression: panic on Fn+F7 hotkey

Li, Shaohua
2003-10-28T12:33:16Z

> -----Original Message-----
> From: Ducrot Bruno [mailto:ducrot-kk6yZipjEM5g9hUCZPvPmw@public.gmane.org]
> Sent: 2003年10月28日 18:04
> To: Li, Shaohua
> Cc: Sérgio Monteiro Basto; Karol Kozimor; acpi-devel
> Subject: Re: [ACPI] 2.6.0-test9 regression: panic on Fn+F7 hotkey
>
> On Tue, Oct 28, 2003 at 04:14:56PM +0800, Li, Shaohua wrote:
> > Hi,
> >
> > I missed non-ECDT situation. Try this one.
> >
>
> Please don't apply. This is not OK. You missed the
> point that an interrupt handler can not sleep. Anyway,
> from my point-of-view, ec.c is not buggy, but
> events/exviface.c::acpi_remove_.*().
>
> IMHO, we should have our own acpi workqueue, at first, and
> then wait for completion when removing any events handlers.
> Those, the correct fix should be in osl.c, and in events/evxface.c
> calling this wait for completition implemented in osl.c.

yes, I know it's a bad idea to execute GPE query in interrupt handler. I do such thing is because this situation is rare. The interrupts seldom occur before EC device is initialized.

But even though acpi has own workqueue, When replace ECDT with EC, we still have a wrong context.

I think a better solution is when replace ECDT with EC, we can change the context in the event queue. Or we can use indirectly accessed context. Like this:

--- ec.c 2003-10-09 03:24:07.000000000 +0800
+++ ec.c.new 2003-10-28 20:17:05.643232728 +0800
@@ -93,7 +93,7 @@

/* External interfaces use first EC only, so remember */
static struct acpi_device *first_ec;
-
+static struct acpi_device *ec_context = NULL;
/* --------------------------------------------------------------------------
Transaction Management
-------------------------------------------------------------------------- */
@@ -386,7 +386,7 @@
void *data)
{
acpi_status status = AE_OK;
- struct acpi_ec *ec = (struct acpi_ec *) data;
+ struct acpi_ec *ec = ec_context;

if (!ec)
return;
@@ -581,7 +581,7 @@
sprintf(acpi_device_name(device), "%s", ACPI_EC_DEVICE_NAME);
sprintf(acpi_device_class(device), "%s", ACPI_EC_CLASS);
acpi_driver_data(device) = ec;
-
+ ec_context = ec;
/* Use the global lock for all EC transactions? */
acpi_evaluate_integer(ec->handle, "_GLK", NULL, &ec->global_lock);

@@ -793,7 +793,7 @@
/* use the GL just to be safe */
ec_ecdt->global_lock = TRUE;
ec_ecdt->uid = ecdt_ptr->uid;
-
+ ec_context = ec_ecdt;
status = acpi_get_handle(NULL, ecdt_ptr->ec_id, &ec_ecdt->handle);
if (ACPI_FAILURE(status)) {
goto error;

In general, ECDT includes similar info as EC device. does this one make sense?

Thanks,
shaohua