patched sms-cm acpi/ec.c to 2.6.11.5

hi everyone,

Since I'm still interested into using the sbs-cm DSDT patch from rich townsend and the most recent release is for 2.6.10, refusing to apply to 2.6.11.X, I changed it.

here it is. rich: could you please take a look and, if it is ok, could you just update your release?

thanks for your time. [patch is also attached]

regards,
pedro venda.

diff -urN linux-2.6.11.5-original/drivers/acpi/ec.c linux-2.6.11.5/drivers/acpi/ec.c
@@ -31,6 +31,7 @@
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <asm/semaphore.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>

@@ -54,8 +55,8 @@
 #define ACPI_EC_EVENT_OBF      0x01    /* Output buffer full */
 #define ACPI_EC_EVENT_IBE      0x02    /* Input buffer empty */
 
-#define ACPI_EC_UDELAY         100     /* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT   1000    /* Wait 10ms max. during EC ops */
+#define ACPI_EC_MSLEEP         100     /* Sleep 1ms between polling */
+#define ACPI_EC_MSLEEP_COUNT   1000    /* Wait 10ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK     1000    /* Wait 1ms max. to get global lock */
 
 #define ACPI_EC_COMMAND_READ   0x80

@@ -87,7 +88,7 @@
        struct acpi_generic_address     command_addr;
        struct acpi_generic_address     data_addr;
        unsigned long                   global_lock;
-       spinlock_t                      lock;
+       struct semaphore                sem;
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */

@@ -106,7 +107,7 @@
        u8                      event)
 {
        u32                     acpi_ec_status = 0;
-       u32                     i = ACPI_EC_UDELAY_COUNT;
+       u32                     i = ACPI_EC_MSLEEP_COUNT;
 
        if (!ec)
                return -EINVAL;

@@ -118,7 +119,7 @@
                        acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
                        if (acpi_ec_status & ACPI_EC_FLAG_OBF)
                                return 0;
-                       udelay(ACPI_EC_UDELAY);
+                       msleep(ACPI_EC_MSLEEP);
                } while (--i>0);
                break;
        case ACPI_EC_EVENT_IBE:

@@ -126,7 +127,7 @@
                        acpi_hw_low_level_read(8, &acpi_ec_status, &ec->status_addr);
                        if (!(acpi_ec_status & ACPI_EC_FLAG_IBF))
                                return 0;
-                       udelay(ACPI_EC_UDELAY);
 
+                       msleep(ACPI_EC_MSLEEP);
                } while (--i>0);
                break;

default:
@@ -137,7 +138,7 @@
}

-static int
+int
acpi_ec_read (
struct acpi_ec *ec,
u8 address,
@@ -145,7 +146,6 @@

 {
        acpi_status             status = AE_OK;
        int                     result = 0;
-       unsigned long           flags = 0;
        u32                     glk = 0;

        ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -161,7 +161,10 @@
                        return_VALUE(-ENODEV);
        }

- spin_lock_irqsave(&ec->lock, flags);
+ if (down_interruptible (&ec->sem)) {
+ result = -ERESTARTSYS;
+ goto end_nosem;
+ }

        acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ, &ec->command_addr);
        result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -180,7 +183,8 @@
                *data, address));

end:
- spin_unlock_irqrestore(&ec->lock, flags);
+ up (&ec->sem);
+end_nosem:

        if (ec->global_lock)
                acpi_release_global_lock(glk);

@@ -189,7 +193,7 @@
}

-static int
+int
acpi_ec_write (
struct acpi_ec *ec,
u8 address,
@@ -197,7 +201,6 @@

 {
        int                     result = 0;
        acpi_status             status = AE_OK;
-       unsigned long           flags = 0;
        u32                     glk = 0;

        ACPI_FUNCTION_TRACE("acpi_ec_write");
@@ -211,7 +214,10 @@
                        return_VALUE(-ENODEV);
        }

- spin_lock_irqsave(&ec->lock, flags);
+ if (down_interruptible (&ec->sem)) {
+ result = -ERESTARTSYS;
+ goto end_nosem;
+ }

        acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE, &ec->command_addr);
        result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -232,7 +238,8 @@
                data, address));

end:
- spin_unlock_irqrestore(&ec->lock, flags);
+ up (&ec->sem);
+end_nosem:

        if (ec->global_lock)
                acpi_release_global_lock(glk);

@@ -284,14 +291,13 @@
EXPORT_SYMBOL(ec_write);

-static int
+int
acpi_ec_query (
struct acpi_ec *ec,
u32 *data)

 {
        int                     result = 0;
        acpi_status             status = AE_OK;
-       unsigned long           flags = 0;
        u32                     glk = 0;

        ACPI_FUNCTION_TRACE("acpi_ec_query");

@@ -312,7 +318,10 @@
* Note that successful completion of the query causes the ACPI_EC_SCI
* bit to be cleared (and thus clearing the interrupt source). */
- spin_lock_irqsave(&ec->lock, flags);
+ if (down_interruptible (&ec->sem)) {
+ result = -ERESTARTSYS;
+ goto end_nosem;
+ }

        acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY, &ec->command_addr);
        result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
@@ -324,7 +333,8 @@
                result = -ENODATA;

end:
- spin_unlock_irqrestore(&ec->lock, flags);
+ up (&ec->sem);
+end_nosem:

        if (ec->global_lock)
                acpi_release_global_lock(glk);
@@ -348,7 +358,6 @@
 {
        struct acpi_ec          *ec = (struct acpi_ec *) ec_cxt;
        u32                     value = 0;
-       unsigned long           flags = 0;
        static char             object_name[5] = {'_','Q','0','0','\0'};
        const char              hex[] = {'0','1','2','3','4','5','6','7',
                                         '8','9','A','B','C','D','E','F'};
@@ -358,9 +367,11 @@
        if (!ec_cxt)
                goto end;

- spin_lock_irqsave(&ec->lock, flags);
+ if (down_interruptible (&ec->sem)) {
+ return_VOID;
+ }

        acpi_hw_low_level_read(8, &value, &ec->command_addr);

- spin_unlock_irqrestore(&ec->lock, flags);
+ up(&ec->sem);

/* TBD: Implement asynch events!
* NOTE: All we care about are EC-SCI's. Other EC events are
@@ -623,7 +634,7 @@

        ec->handle = device->handle;
        ec->uid = -1;
-       spin_lock_init(&ec->lock);
+       sema_init(&ec->sem, 1);
        strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
        strcpy(acpi_device_class(device), ACPI_EC_CLASS);
        acpi_driver_data(device) = ec;
@@ -832,7 +843,7 @@
        status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec_ecdt->gpe_bit);
        if (ACPI_FAILURE(status))
                return status;
-       spin_lock_init(&ec_ecdt->lock);
+       sema_init(&ec_ecdt->sem, 1);
        ec_ecdt->global_lock = TRUE;
        ec_ecdt->handle = handle;

@@ -909,7 +920,7 @@

        ec_ecdt->status_addr = ecdt_ptr->ec_control;
        ec_ecdt->data_addr = ecdt_ptr->ec_data;
        ec_ecdt->gpe_bit = ecdt_ptr->gpe_bit;
-       spin_lock_init(&ec_ecdt->lock);
+       sema_init(&ec_ecdt->sem, 1);
        /* use the GL just to be safe */
        ec_ecdt->global_lock = TRUE;
        ec_ecdt->uid = ecdt_ptr->uid;

Pedro João Lopes Venda
email: pjvenda < at > arrakis.dhis.org
http://arrakis.dhis.org

I tried this patch and it doens't work very well. First, the kernel takes a very long time to boot. I have to wait more than 30 seconds to see the first kernel messages appear. Second, it takes also a long time to load the ac and battery modules. It correctly detects the AC status and the presence of the battery. Nevertheless, I can never get the battery charge level (or maybe after a too long time and I didn't wait enough). Finally, it causes problem with USB support (or maybe only with the usblp module). Here below is an extract of dmesg showing the problem:

drivers/usb/class/usblp.c: usblp0: USB Bidirectional printer dev 2 if 1 alt 0 proto 2 vid 0x03F0 pi d 0x3F11
usbcore: registered new driver usblp
drivers/usb/class/usblp.c: v0.13: USB Printer Device Class driver
usb 2-2: USB disconnect, address 2
drivers/usb/class/usblp.c: usblp0: removed
usb 2-2: new full speed USB device using uhci_hcd and address 3
usb 2-2: khubd timed out on ep0in
usb 2-2: unable to read config index 0 descriptor/start
usb 2-2: can't read configurations, error -110
usb 2-2: new full speed USB device using uhci_hcd and address 4 drivers/usb/class/usblp.c: usblp0: USB Bidirectional printer dev 4 if 1 alt 0 proto 2 vid 0x03F0 pi d 0x3F11
So it seems there is still some work needed to get sbs-cm patch working correctly.

François Valenduc

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

hello francois,

François Valenduc wrote:

| I tried this patch and it doens't work very well. First, the kernel
| takes a very long time to boot. I have to wait more than 30 seconds to
| see the first kernel messages appear.

I've also experienced this initial delay, but mine didn't take AT ALL 30 seconds. 5 perhaps but not 30. I'm still trying to figure this out. with me it happens immediately at kernel boot. no messages, just a black screen. after a while, the kernel boots normally as if it just began booting.

| Second, it takes also a long time
| to load the ac and battery modules. It correctly detects the AC status
| and the presence of the battery. Nevertheless, I can never get the
| battery charge level (or maybe after a too long time and I didn't wait
| enough).

sorry, didn't happen here. this behaviour I could explain with a buggy patch. you see, since the latest rich's patch didn't apply, I looked at it and implemented the same changes on the current drivers/acpi/ec.c, so I could have ruined it.

basically I switched the lock struct member with a semaphore and replaced every spinlock call by a corresponding semaphore call.

| Finally, it causes problem with USB support (or maybe only with | the usblp module). Here below is an extract of dmesg showing the problem:

that's new. I'm just using an usb mouse on the laptop...

| So it seems there is still some work needed to get sbs-cm patch working | correctly.

indeed. the problem is that I'm not experienced in kernel programming and rich townsend is currently pursuing another path to support smart-batteries.

thanks for your bug reports. Not sure if I'll be able to help you though.

regards,
pedro venda.
- --

Pedro João Lopes Venda
email: pjvenda < at > arrakis.dhis.org
http://arrakis.dhis.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCP1LNeRy7HWZxjWERAhMIAJ4lR4Xn1ctA+GCWjQyX+Cee6kehgwCg+RHv hOR2oUDE32Z8g9fCmqe4iQ8=
=8Jm2
-----END PGP SIGNATURE-----

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_ide95&alloc_id396&op=click

<snipped patch>

Pedro --

Thanks for the patch, which is now incorporated in the latest release (20050328) of the sbs-cm package. This release also includes a modified SMBus-CM driver, which should reduce mutex thrashing (read: less noisy debug output).

FYI, It's doubtful whether I'm going to put that much work into furthering the sbs-cm stuff. From what a number of people have said, the HAL is the place to be implementing the Smart Battery support, and that's where I'm going to be investing most of my future effort.

cheers,

Rich

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

ok, thanks. So the patch is ok? what about the ec.c driver problems?

I understand. I have a question though: without kernel drivers, how can the HAL talk to smart batteries?

Also, if I apply some effort to port the kernel patch into other kernel versions, could you please give it quick reviews and possibly update the package? not by introducing features, but just keeping it working. That'll imply some work from you on a less-than-montly basis, I predict, and only while HAL isn't widespread enough... let's face it: few people are using it these days, I guess.

any comments?

regards,
pedro venda.

Pedro João Lopes Venda
email: pjvenda < at > arrakis.dhis.org
http://arrakis.dhis.org

hello rich,

take your time. we're all doing what we can.

it does NOT seem ok. after some more tests with the patch, I found out that francois was right about most of the problems:
* timeout on boot, just after the boot loader. this is very confusing to me...
* when I plug in the battery, it takes ages to show information about it... more than 1 minute.
* about the usb problems, apparently I wasn't affected.

could you please tip us about what could be causing these delays? especially the boot delay... it really puzzles my mind.

regards,
pedro venda.

Pedro João Lopes Venda
email: pjvenda < at > arrakis.dhis.org
http://arrakis.dhis.org

Concerning USB problems, I think it happens because I tried the patch with the 2.6.11 kernel. I have retried with the 2.6.11.5 and there are no more USB problems. But all the other problems remains. Concerning the timeout on boot, could it be caused by the acpi-dsdt-initrd patch made by Markus Gaugausch. Maybe we should try other ways to incorporate the DSDT to the kernel. François Valenduc

-------------------------------------------------------
This SF.net email is sponsored by Microsoft Mobile & Embedded DevCon 2005 Attend MEDC 2005 May 9-12 in Vegas. Learn more about the latest Windows Embedded(r) & Windows Mobile(tm) platforms, applications & content. Register by 3/29 & save $300 http://ads.osdn.com/?ad_id=6883&alloc_id=15149&op=click

I doubt it is the DSDT patch; rather, it looks like something has gone wrong in the embedded controller driver in going from 2.6.10 to 2.6.11.
I've tried out a number of different ways of fixing ec event handling,
including my own spinlock kludge and Pedro's version of it, and also a series of patches by Dmitry Torokhov; but all of them seem to have problems with very sluggish EC response and/or EC timeouts (indicated by "Handler for [EmbeddedControl] returned AE_TIME").

Who currently maintains ec.c? And what exactly has been done to it since 2.6.10?

cheers,

Rich

-------------------------------------------------------
This SF.net email is sponsored by Microsoft Mobile & Embedded DevCon 2005 Attend MEDC 2005 May 9-12 in Vegas. Learn more about the latest Windows Embedded(r) & Windows Mobile(tm) platforms, applications & content. Register by 3/29 & save $300 http://ads.osdn.com/?ad_idh83&alloc_id149&op=click

Just by curiosity I tried the DSDT patches without applying any patch on the ec.c file and I don't notice any problem yet. I choose for a static DSDT override and there doesn't seem to be any slowdown of my computer. If I have well understood I could see "Handler for [EmbeddedControl] returned AE_TIME" appearing in kernel messages but it doesn't appear. Maybe I am very lucky!

François

-------------------------------------------------------
This SF.net email is sponsored by Microsoft Mobile & Embedded DevCon 2005 Attend MEDC 2005 May 9-12 in Vegas. Learn more about the latest Windows Embedded(r) & Windows Mobile(tm) platforms, applications & content. Register by 3/29 & save $300 http://ads.osdn.com/?ad_id=6883&alloc_id=15149&op=click