Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Voluspa]

  CC      drivers/acpi/glue.o
  CC      drivers/acpi/ec.o
drivers/acpi/ec.c: In function `acpi_ec_poll_read':
drivers/acpi/ec.c:341: error: union has no member named `polling'
drivers/acpi/ec.c:363: error: union has no member named `polling'
drivers/acpi/ec.c: In function `acpi_ec_poll_write':
drivers/acpi/ec.c:390: error: union has no member named `polling'
drivers/acpi/ec.c:415: error: union has no member named `polling'
drivers/acpi/ec.c:376: warning: unused variable `flags'
drivers/acpi/ec.c: In function `acpi_ec_poll_query':
drivers/acpi/ec.c:599: error: union has no member named `polling'
drivers/acpi/ec.c:615: error: union has no member named `polling'
drivers/acpi/ec.c:578: warning: unused variable `flags'
drivers/acpi/ec.c: In function `acpi_ec_gpe_poll_query':
drivers/acpi/ec.c:705: error: union has no member named `polling'
drivers/acpi/ec.c:708: error: union has no member named `polling'
drivers/acpi/ec.c:694: warning: unused variable `flags'
drivers/acpi/ec.c: In function `acpi_ec_poll_add':
drivers/acpi/ec.c:1020: error: union has no member named `polling'
drivers/acpi/ec.c: In function `acpi_fake_ecdt_poll_callback':
drivers/acpi/ec.c:1315: error: union has no member named `polling'
drivers/acpi/ec.c: In function `acpi_ec_poll_get_real_ecdt':
drivers/acpi/ec.c:1431: error: union has no member named `polling'
make[2]: *** [drivers/acpi/ec.o] Error 1
make[1]: *** [drivers/acpi] Error 2
make: *** [drivers] Error 2

Mvh
Mats Johannesson
--

[UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Randy Dunlap]

From: Ben Collins <bcollins@ubuntu.com>

[UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
interrupts on ACPI EC (embedded controller)

Reference: https://launchpad.net/bugs/39315
http://www.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-dapper.git;a=commitdiff;h=c484728a760fcfcbad2319ed5364414bc86c3d38

Signed-off-by: Ben Collins <bcollins@ubuntu.com>
---

--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -53,8 +53,8 @@ ACPI_MODULE_NAME("acpi_ec")
 #define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
 #define ACPI_EC_DELAY		50	/* Wait 50ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
-#define ACPI_EC_UDELAY         100	/* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT   1000	/* Wait 10ms max. during EC ops */
+#define ACPI_EC_MSLEEP		1	/* Poll @ 1ms increments */
+#define ACPI_EC_MSLEEP_COUNT	10	/* Wait 10ms max. during EC ops */
 #define ACPI_EC_COMMAND_READ	0x80
 #define ACPI_EC_COMMAND_WRITE	0x81
 #define ACPI_EC_BURST_ENABLE	0x82
@@ -116,7 +116,7 @@ union acpi_ec {
 		struct acpi_generic_address command_addr;
 		struct acpi_generic_address data_addr;
 		unsigned long global_lock;
-		spinlock_t lock;
+		struct semaphore sem;
 	} poll;
 };
 
@@ -172,7 +172,7 @@ static int acpi_ec_wait(union acpi_ec *e
 static int acpi_ec_poll_wait(union acpi_ec *ec, u8 event)
 {
 	u32 acpi_ec_status = 0;
-	u32 i = ACPI_EC_UDELAY_COUNT;
+	u32 i = ACPI_EC_MSLEEP_COUNT;
 
 	if (!ec)
 		return -EINVAL;
@@ -185,7 +185,7 @@ static int acpi_ec_poll_wait(union acpi_
 					       &ec->common.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:
@@ -194,7 +194,7 @@ static int acpi_ec_poll_wait(union acpi_
 					       &ec->common.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:
@@ -326,7 +326,6 @@ static int acpi_ec_poll_read(union acpi_
 {
 	acpi_status status = AE_OK;
 	int result = 0;
-	unsigned long flags = 0;
 	u32 glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -342,7 +341,10 @@ static int acpi_ec_poll_read(union acpi_
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible(&ec->polling.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
 				&ec->common.command_addr);
@@ -361,7 +363,8 @@ static int acpi_ec_poll_read(union acpi_
 			  *data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->polling.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -387,7 +390,10 @@ static int acpi_ec_poll_write(union acpi
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible(&ec->polling.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
 				&ec->common.command_addr);
@@ -409,7 +415,8 @@ static int acpi_ec_poll_write(union acpi
 			  data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->polling.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -592,7 +599,10 @@ static int acpi_ec_poll_query(union acpi
 	 * 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->poll.lock, flags);
+	if (down_interruptible(&ec->polling.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
 				&ec->common.command_addr);
@@ -605,7 +615,8 @@ static int acpi_ec_poll_query(union acpi
 		result = -ENODATA;
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->polling.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -694,9 +705,10 @@ static void acpi_ec_gpe_poll_query(void 
 	if (!ec_cxt)
 		goto end;
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible (&ec->polling.sem))
+		return_VOID;
 	acpi_hw_low_level_read(8, &value, &ec->common.command_addr);
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->polling.sem);
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -1008,7 +1020,7 @@ static int acpi_ec_poll_add(struct acpi_
 
 	ec->common.handle = device->handle;
 	ec->common.uid = -1;
-	spin_lock_init(&ec->poll.lock);
+	init_MUTEX(&ec->polling.sem);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -1303,7 +1315,7 @@ acpi_fake_ecdt_poll_callback(acpi_handle
 				  &ec_ecdt->common.gpe_bit);
 	if (ACPI_FAILURE(status))
 		return status;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->polling.sem);
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.handle = handle;
 
@@ -1419,7 +1431,7 @@ static int __init acpi_ec_poll_get_real_
 	ec_ecdt->common.status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->common.data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->common.gpe_bit = ecdt_ptr->gpe_bit;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->polling.sem);
 	/* use the GL just to be safe */
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.uid = ecdt_ptr->uid;

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Randy Dunlap]

Voluspa wrote:
>   CC      drivers/acpi/glue.o
>   CC      drivers/acpi/ec.o
> drivers/acpi/ec.c: In function `acpi_ec_poll_read':
> drivers/acpi/ec.c:341: error: union has no member named `polling'
> drivers/acpi/ec.c:363: error: union has no member named `polling'
> drivers/acpi/ec.c: In function `acpi_ec_poll_write':
> drivers/acpi/ec.c:390: error: union has no member named `polling'
> drivers/acpi/ec.c:415: error: union has no member named `polling'
> drivers/acpi/ec.c:376: warning: unused variable `flags'
> drivers/acpi/ec.c: In function `acpi_ec_poll_query':
> drivers/acpi/ec.c:599: error: union has no member named `polling'
> drivers/acpi/ec.c:615: error: union has no member named `polling'
> drivers/acpi/ec.c:578: warning: unused variable `flags'
> drivers/acpi/ec.c: In function `acpi_ec_gpe_poll_query':
> drivers/acpi/ec.c:705: error: union has no member named `polling'
> drivers/acpi/ec.c:708: error: union has no member named `polling'
> drivers/acpi/ec.c:694: warning: unused variable `flags'
> drivers/acpi/ec.c: In function `acpi_ec_poll_add':
> drivers/acpi/ec.c:1020: error: union has no member named `polling'
> drivers/acpi/ec.c: In function `acpi_fake_ecdt_poll_callback':
> drivers/acpi/ec.c:1315: error: union has no member named `polling'
> drivers/acpi/ec.c: In function `acpi_ec_poll_get_real_ecdt':
> drivers/acpi/ec.c:1431: error: union has no member named `polling'
> make[2]: *** [drivers/acpi/ec.o] Error 1
> make[1]: *** [drivers/acpi] Error 2
> make: *** [drivers] Error 2

Thanks, I'll work on that.

~Randy

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Randy Dunlap]

updated version:

From: Ben Collins <bcollins@ubuntu.com>

[UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
interrupts on ACPI EC (embedded controller)

Reference: https://launchpad.net/bugs/39315
http://www.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-dapper.git;a=commitdiff;h=c484728a760fcfcbad2319ed5364414bc86c3d38

Signed-off-by: Ben Collins <bcollins@ubuntu.com>
---

---
 drivers/acpi/ec.c |   48 ++++++++++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 18 deletions(-)

--- linux-2617-rc6g7.orig/drivers/acpi/ec.c
+++ linux-2617-rc6g7/drivers/acpi/ec.c
@@ -53,8 +53,8 @@ ACPI_MODULE_NAME("acpi_ec")
 #define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
 #define ACPI_EC_DELAY		50	/* Wait 50ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
-#define ACPI_EC_UDELAY         100	/* Poll @ 100us increments */
-#define ACPI_EC_UDELAY_COUNT   1000	/* Wait 10ms max. during EC ops */
+#define ACPI_EC_MSLEEP		1	/* Poll @ 1ms increments */
+#define ACPI_EC_MSLEEP_COUNT	10	/* Wait 10ms max. during EC ops */
 #define ACPI_EC_COMMAND_READ	0x80
 #define ACPI_EC_COMMAND_WRITE	0x81
 #define ACPI_EC_BURST_ENABLE	0x82
@@ -116,7 +116,7 @@ union acpi_ec {
 		struct acpi_generic_address command_addr;
 		struct acpi_generic_address data_addr;
 		unsigned long global_lock;
-		spinlock_t lock;
+		struct semaphore sem;
 	} poll;
 };
 
@@ -172,7 +172,7 @@ static int acpi_ec_wait(union acpi_ec *e
 static int acpi_ec_poll_wait(union acpi_ec *ec, u8 event)
 {
 	u32 acpi_ec_status = 0;
-	u32 i = ACPI_EC_UDELAY_COUNT;
+	u32 i = ACPI_EC_MSLEEP_COUNT;
 
 	if (!ec)
 		return -EINVAL;
@@ -185,7 +185,7 @@ static int acpi_ec_poll_wait(union acpi_
 					       &ec->common.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:
@@ -194,7 +194,7 @@ static int acpi_ec_poll_wait(union acpi_
 					       &ec->common.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:
@@ -323,7 +323,6 @@ static int acpi_ec_poll_read(union acpi_
 {
 	acpi_status status = AE_OK;
 	int result = 0;
-	unsigned long flags = 0;
 	u32 glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -339,7 +338,10 @@ static int acpi_ec_poll_read(union acpi_
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
 				&ec->common.command_addr);
@@ -358,7 +360,8 @@ static int acpi_ec_poll_read(union acpi_
 			  *data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->poll.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -384,7 +387,10 @@ static int acpi_ec_poll_write(union acpi
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
 				&ec->common.command_addr);
@@ -406,7 +412,8 @@ static int acpi_ec_poll_write(union acpi
 			  data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->poll.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -589,7 +596,10 @@ static int acpi_ec_poll_query(union acpi
 	 * 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->poll.lock, flags);
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
 
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
 				&ec->common.command_addr);
@@ -602,7 +612,8 @@ static int acpi_ec_poll_query(union acpi
 		result = -ENODATA;
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->poll.sem);
+end_nosem:
 
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
@@ -691,9 +702,10 @@ static void acpi_ec_gpe_poll_query(void 
 	if (!ec_cxt)
 		goto end;
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible (&ec->poll.sem))
+		return_VOID;
 	acpi_hw_low_level_read(8, &value, &ec->common.command_addr);
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->poll.sem);
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -1005,7 +1017,7 @@ static int acpi_ec_poll_add(struct acpi_
 
 	ec->common.handle = device->handle;
 	ec->common.uid = -1;
-	spin_lock_init(&ec->poll.lock);
+	init_MUTEX(&ec->poll.sem);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -1300,7 +1312,7 @@ acpi_fake_ecdt_poll_callback(acpi_handle
 				  &ec_ecdt->common.gpe_bit);
 	if (ACPI_FAILURE(status))
 		return status;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->poll.sem);
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.handle = handle;
 
@@ -1416,7 +1428,7 @@ static int __init acpi_ec_poll_get_real_
 	ec_ecdt->common.status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->common.data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->common.gpe_bit = ecdt_ptr->gpe_bit;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->poll.sem);
 	/* use the GL just to be safe */
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.uid = ecdt_ptr->uid;

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Bongani Hlope]

On Thursday 15 June 2006 01:22, Randy Dunlap wrote:
> From: Ben Collins <bcollins@ubuntu.com>
>
> [UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
> interrupts on ACPI EC (embedded controller)
>
> Reference: https://launchpad.net/bugs/39315
> http://www.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-dapper.git;a=
>commitdiff;h=c484728a760fcfcbad2319ed5364414bc86c3d38
>
> Signed-off-by: Ben Collins <bcollins@ubuntu.com>
> ---
>
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -53,8 +53,8 @@ ACPI_MODULE_NAME("acpi_ec")
>  #define ACPI_EC_EVENT_IBE	0x02	/* Input buffer empty */
>  #define ACPI_EC_DELAY		50	/* Wait 50ms max. during EC ops */
>  #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
> -#define ACPI_EC_UDELAY         100	/* Poll @ 100us increments */
> -#define ACPI_EC_UDELAY_COUNT   1000	/* Wait 10ms max. during EC ops */
> +#define ACPI_EC_MSLEEP		1	/* Poll @ 1ms increments */
> +#define ACPI_EC_MSLEEP_COUNT	10	/* Wait 10ms max. during EC ops */
>  #define ACPI_EC_COMMAND_READ	0x80
>  #define ACPI_EC_COMMAND_WRITE	0x81
>  #define ACPI_EC_BURST_ENABLE	0x82
> @@ -116,7 +116,7 @@ union acpi_ec {
>  		struct acpi_generic_address command_addr;
>  		struct acpi_generic_address data_addr;
>  		unsigned long global_lock;
> -		spinlock_t lock;
> +		struct semaphore sem;
>  	} poll;
>  };
>
> @@ -172,7 +172,7 @@ static int acpi_ec_wait(union acpi_ec *e
>  static int acpi_ec_poll_wait(union acpi_ec *ec, u8 event)
>  {
>  	u32 acpi_ec_status = 0;
> -	u32 i = ACPI_EC_UDELAY_COUNT;
> +	u32 i = ACPI_EC_MSLEEP_COUNT;
>
>  	if (!ec)
>  		return -EINVAL;
> @@ -185,7 +185,7 @@ static int acpi_ec_poll_wait(union acpi_
>  					       &ec->common.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:
> @@ -194,7 +194,7 @@ static int acpi_ec_poll_wait(union acpi_
>  					       &ec->common.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:
> @@ -326,7 +326,6 @@ static int acpi_ec_poll_read(union acpi_
>  {
>  	acpi_status status = AE_OK;
>  	int result = 0;
> -	unsigned long flags = 0;
>  	u32 glk = 0;
>
>  	ACPI_FUNCTION_TRACE("acpi_ec_read");
> @@ -342,7 +341,10 @@ static int acpi_ec_poll_read(union acpi_
>  			return_VALUE(-ENODEV);
>  	}
>
> -	spin_lock_irqsave(&ec->poll.lock, flags);
> +	if (down_interruptible(&ec->polling.sem)) {
                                                     ^^^^
isn't this suppose to be: &ec->poll.sem

> +		result = -ERESTARTSYS;
> +		goto end_nosem;
> +	}
>
>  	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
>  				&ec->common.command_addr);
> @@ -361,7 +363,8 @@ static int acpi_ec_poll_read(union acpi_
>  			  *data, address));
>
>        end:
> -	spin_unlock_irqrestore(&ec->poll.lock, flags);
> +	up(&ec->polling.sem);
                     ^^^^
&ec->poll.sem

> +end_nosem:
>
>  	if (ec->common.global_lock)
>  		acpi_release_global_lock(glk);
> @@ -387,7 +390,10 @@ static int acpi_ec_poll_write(union acpi
>  			return_VALUE(-ENODEV);
>  	}
>
> -	spin_lock_irqsave(&ec->poll.lock, flags);
> +	if (down_interruptible(&ec->polling.sem)) {
                     ^^^^
&ec->poll.sem

> +		result = -ERESTARTSYS;
> +		goto end_nosem;
> +	}
>
>  	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
>  				&ec->common.command_addr);
> @@ -409,7 +415,8 @@ static int acpi_ec_poll_write(union acpi
>  			  data, address));
>
>        end:
> -	spin_unlock_irqrestore(&ec->poll.lock, flags);
> +	up(&ec->polling.sem);
                     ^^^^
&ec->poll.sem

> +end_nosem:
>
>  	if (ec->common.global_lock)
>  		acpi_release_global_lock(glk);
> @@ -592,7 +599,10 @@ static int acpi_ec_poll_query(union acpi
>  	 * 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->poll.lock, flags);
> +	if (down_interruptible(&ec->polling.sem)) {
                                                    ^^^^
&ec->poll.sem

> +		result = -ERESTARTSYS;
> +		goto end_nosem;
> +	}
>
>  	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
>  				&ec->common.command_addr);
> @@ -605,7 +615,8 @@ static int acpi_ec_poll_query(union acpi
>  		result = -ENODATA;
>
>        end:
> -	spin_unlock_irqrestore(&ec->poll.lock, flags);
> +	up(&ec->polling.sem);
                     ^^^^
&ec->poll.sem

> +end_nosem:
>
>  	if (ec->common.global_lock)
>  		acpi_release_global_lock(glk);
> @@ -694,9 +705,10 @@ static void acpi_ec_gpe_poll_query(void
>  	if (!ec_cxt)
>  		goto end;
>
> -	spin_lock_irqsave(&ec->poll.lock, flags);
> +	if (down_interruptible (&ec->polling.sem))
                                                  ^^^^
&ec->poll.sem

> +		return_VOID;
>  	acpi_hw_low_level_read(8, &value, &ec->common.command_addr);
> -	spin_unlock_irqrestore(&ec->poll.lock, flags);
> +	up(&ec->polling.sem);
                     ^^^^
&ec->poll.sem

>
>  	/* TBD: Implement asynch events!
>  	 * NOTE: All we care about are EC-SCI's.  Other EC events are
> @@ -1008,7 +1020,7 @@ static int acpi_ec_poll_add(struct acpi_
>
>  	ec->common.handle = device->handle;
>  	ec->common.uid = -1;
> -	spin_lock_init(&ec->poll.lock);
> +	init_MUTEX(&ec->polling.sem);
                                    ^^^^
&ec->poll.sem

>  	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
>  	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
>  	acpi_driver_data(device) = ec;
> @@ -1303,7 +1315,7 @@ acpi_fake_ecdt_poll_callback(acpi_handle
>  				  &ec_ecdt->common.gpe_bit);
>  	if (ACPI_FAILURE(status))
>  		return status;
> -	spin_lock_init(&ec_ecdt->poll.lock);
> +	init_MUTEX(&ec_ecdt->polling.sem);
                     ^^^^
&ec->poll.sem

>  	ec_ecdt->common.global_lock = TRUE;
>  	ec_ecdt->common.handle = handle;
>
> @@ -1419,7 +1431,7 @@ static int __init acpi_ec_poll_get_real_
>  	ec_ecdt->common.status_addr = ecdt_ptr->ec_control;
>  	ec_ecdt->common.data_addr = ecdt_ptr->ec_data;
>  	ec_ecdt->common.gpe_bit = ecdt_ptr->gpe_bit;
> -	spin_lock_init(&ec_ecdt->poll.lock);
> +	init_MUTEX(&ec_ecdt->polling.sem);
                                         ^^^^
&ec->poll.sem

>  	/* use the GL just to be safe */
>  	ec_ecdt->common.global_lock = TRUE;
>  	ec_ecdt->common.uid = ecdt_ptr->uid;
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Roland Dreier]

 > [UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
 > interrupts on ACPI EC (embedded controller)

 > -		spinlock_t lock;
 > +		struct semaphore sem;

 > +	init_MUTEX(&ec->poll.sem);

I think nowadays this should be a struct mutex...

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Voluspa]

On Wed, 14 Jun 2006 18:14:54 -0700 Randy Dunlap wrote:
> updated version:

As a user, it's great if this fixes people's keyboards and mice. But it's
not a panacea. Gkrellm reads CPU temperatures from
/proc/acpi/thermal_zone/*/temperature and that disturbs a time-critical
application like mplayer, both when reading normal video and hacked mms:
sound streams (ogg sound is OK, though):

rtc: lost some interrupts at 1024Hz.
rtc: lost some interrupts at 1024Hz.

The loss frequency is much lower with this patch, by something like an
estimated 5-10 times.

Also, here the notebook's touchpad gets clobbered by temperature reads
when dragging a small xterm window around:

psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: issuing reconnect request
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 4
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 - driver resynched.
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 4
psmouse.c: TouchPad at isa0060/serio4/input0 lost sync at byte 1
psmouse.c: TouchPad at isa0060/serio4/input0 - driver resynched.

So, here temperature reading stays off even if this patch hits mainline.
Battery reading is fine, strangely enough. Perhaps because I've set
a period of 30 seconds between updates.

Or it might be that a lot of work has gone in regarding acpi BAT...

Mvh
Mats Johannesson
--

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Arjan van de Ven]

On Wed, 2006-06-14 at 18:14 -0700, Randy Dunlap wrote:
> updated version:
> 
> From: Ben Collins <bcollins@ubuntu.com>
> 
> [UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
> interrupts on ACPI EC (embedded controller)


that is odd.

first of all this should use a mutex not a semaphore.

Second, if this isn't used in irq context, then just dropping the
"_irqsave" from the spinlocks should be enough to actually get rid of
the missed interrupts.... and if it is used there then I highly question
your use of semaphores etc...

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Lee Revell]

On Thu, 2006-06-15 at 13:03 +0200, Voluspa wrote:
> On Wed, 14 Jun 2006 18:14:54 -0700 Randy Dunlap wrote:
> > updated version:
> 
> As a user, it's great if this fixes people's keyboards and mice. But it's
> not a panacea. Gkrellm reads CPU temperatures from
> /proc/acpi/thermal_zone/*/temperature and that disturbs a time-critical
> application like mplayer, both when reading normal video and hacked mms:
> sound streams (ogg sound is OK, though):
> 

It would be helpful to analyze this with Ingo's latency tracing patch.

Lee

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Voluspa]

On Thu, 15 Jun 2006 12:27:03 -0400 Lee Revell wrote:
> On Thu, 2006-06-15 at 13:03 +0200, Voluspa wrote:
> > On Wed, 14 Jun 2006 18:14:54 -0700 Randy Dunlap wrote:
> > > updated version:
> > 
> > As a user, it's great if this fixes people's keyboards and mice. But it's
> > not a panacea. Gkrellm reads CPU temperatures from
> > /proc/acpi/thermal_zone/*/temperature and that disturbs a time-critical
> > application like mplayer, both when reading normal video and hacked mms:
> > sound streams (ogg sound is OK, though):
> > 
> 
> It would be helpful to analyze this with Ingo's latency tracing patch.

Do you mean proper in relation to ubuntu-patched, or just proper? And, hmmm,
I do read a lot of archived lkml threads, but latency tracing patch... Is
it buried in the -rt set?

Mvh
Mats Johannesson
--

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Luiz Fernando N. Capitulino]

On Wed, 14 Jun 2006 16:22:26 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

| From: Ben Collins <bcollins@ubuntu.com>
| 
| [UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
| interrupts on ACPI EC (embedded controller)

 Why not the new mutex API?

-- 
Luiz Fernando N. Capitulino

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Randy Dunlap]

Luiz Fernando N. Capitulino wrote:
> On Wed, 14 Jun 2006 16:22:26 -0700
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> | From: Ben Collins <bcollins@ubuntu.com>
> | 
> | [UBUNTU:acpi/ec] Use semaphore instead of spinlock to get rid of missed
> | interrupts on ACPI EC (embedded controller)
> 
>  Why not the new mutex API?

Yes, of course, as Arjan also said.  Arjan also had some other
good comments about this particular patch.

Thanks,
~Randy

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Lee Revell]

On Fri, 2006-06-16 at 11:23 +0200, Voluspa wrote:
> On Thu, 15 Jun 2006 12:27:03 -0400 Lee Revell wrote:
> > On Thu, 2006-06-15 at 13:03 +0200, Voluspa wrote:
> > > On Wed, 14 Jun 2006 18:14:54 -0700 Randy Dunlap wrote:
> > > > updated version:
> > > 
> > > As a user, it's great if this fixes people's keyboards and mice. But it's
> > > not a panacea. Gkrellm reads CPU temperatures from
> > > /proc/acpi/thermal_zone/*/temperature and that disturbs a time-critical
> > > application like mplayer, both when reading normal video and hacked mms:
> > > sound streams (ogg sound is OK, though):
> > > 
> > 
> > It would be helpful to analyze this with Ingo's latency tracing patch.
> 
> Do you mean proper in relation to ubuntu-patched, or just proper?

Not sure, I'm not familiar with the exact reason that reading CPU
temperatures would cause long latencies like this.  The latency tracer
can tell you the exact code path responsible.

>  And, hmmm,
> I do read a lot of archived lkml threads, but latency tracing patch... Is
> it buried in the -rt set?

It's part of the -rt set and also available broken out for 2.6.16:

http://people.redhat.com/mingo/latency-tracing-patches/latency-tracing-v2.6.16.patch

Lee

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Andrew Morton]

Bongani Hlope <bhlope@mweb.co.za> wrote:
>
> > @@ -342,7 +341,10 @@ static int acpi_ec_poll_read(union acpi_
> >  			return_VALUE(-ENODEV);
> >  	}
> >
> > -	spin_lock_irqsave(&ec->poll.lock, flags);
> > +	if (down_interruptible(&ec->polling.sem)) {
>                                                      ^^^^
> isn't this suppose to be: &ec->poll.sem

Good question.   Did it get resolved?

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Randy Dunlap]

Andrew Morton wrote:
> Bongani Hlope <bhlope@mweb.co.za> wrote:
>>> @@ -342,7 +341,10 @@ static int acpi_ec_poll_read(union acpi_
>>>  			return_VALUE(-ENODEV);
>>>  	}
>>>
>>> -	spin_lock_irqsave(&ec->poll.lock, flags);
>>> +	if (down_interruptible(&ec->polling.sem)) {
>>                                                      ^^^^
>> isn't this suppose to be: &ec->poll.sem
> 
> Good question.   Did it get resolved?

I posted a corrected patch that does that.
No responses from the ACPI people.

~Randy

Re: [UBUNTU:acpi/ec] Use semaphore instead of spinlock

[Andrew Morton]

Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
> Andrew Morton wrote:
> > Bongani Hlope <bhlope@mweb.co.za> wrote:
> >>> @@ -342,7 +341,10 @@ static int acpi_ec_poll_read(union acpi_
> >>>  			return_VALUE(-ENODEV);
> >>>  	}
> >>>
> >>> -	spin_lock_irqsave(&ec->poll.lock, flags);
> >>> +	if (down_interruptible(&ec->polling.sem)) {
> >>                                                      ^^^^
> >> isn't this suppose to be: &ec->poll.sem
> > 
> > Good question.   Did it get resolved?
> 
> I posted a corrected patch that does that.
> No responses from the ACPI people.
> 

OK.  It appears that this patch is already somewhat partially in the acpi
devel tree.

But it's using a semaphore and not a mutex, and those udelays are still in
there.

Here's what git-apci has for that file:

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index eee0864..18b3ea9 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -116,7 +116,7 @@ union acpi_ec {
 		struct acpi_generic_address command_addr;
 		struct acpi_generic_address data_addr;
 		unsigned long global_lock;
-		spinlock_t lock;
+		struct semaphore sem;
 	} poll;
 };
 
@@ -323,7 +323,6 @@ static int acpi_ec_poll_read(union acpi_
 {
 	acpi_status status = AE_OK;
 	int result = 0;
-	unsigned long flags = 0;
 	u32 glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_read");
@@ -339,8 +338,11 @@ static int acpi_ec_poll_read(union acpi_
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
-
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
 				&ec->common.command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -358,8 +360,8 @@ static int acpi_ec_poll_read(union acpi_
 			  *data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
-
+	up(&ec->poll.sem);
+end_nosem:
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
 
@@ -370,7 +372,6 @@ static int acpi_ec_poll_write(union acpi
 {
 	int result = 0;
 	acpi_status status = AE_OK;
-	unsigned long flags = 0;
 	u32 glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_write");
@@ -384,8 +385,11 @@ static int acpi_ec_poll_write(union acpi
 			return_VALUE(-ENODEV);
 	}
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
-
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
 				&ec->common.command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
@@ -406,8 +410,8 @@ static int acpi_ec_poll_write(union acpi
 			  data, address));
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
-
+	up(&ec->poll.sem);
+end_nosem:
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
 
@@ -568,7 +572,6 @@ static int acpi_ec_poll_query(union acpi
 {
 	int result = 0;
 	acpi_status status = AE_OK;
-	unsigned long flags = 0;
 	u32 glk = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_query");
@@ -589,8 +592,11 @@ static int acpi_ec_poll_query(union acpi
 	 * 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->poll.lock, flags);
-
+	if (down_interruptible(&ec->poll.sem)) {
+		result = -ERESTARTSYS;
+		goto end_nosem;
+	}
+	
 	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
 				&ec->common.command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
@@ -602,8 +608,8 @@ static int acpi_ec_poll_query(union acpi
 		result = -ENODATA;
 
       end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
-
+	up(&ec->poll.sem);
+end_nosem:
 	if (ec->common.global_lock)
 		acpi_release_global_lock(glk);
 
@@ -680,7 +686,6 @@ static void acpi_ec_gpe_poll_query(void 
 {
 	union acpi_ec *ec = (union 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'
@@ -691,9 +696,11 @@ static void acpi_ec_gpe_poll_query(void 
 	if (!ec_cxt)
 		goto end;
 
-	spin_lock_irqsave(&ec->poll.lock, flags);
+	if (down_interruptible (&ec->poll.sem)) {
+		return_VOID;
+	}
 	acpi_hw_low_level_read(8, &value, &ec->common.command_addr);
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	up(&ec->poll.sem);
 
 	/* TBD: Implement asynch events!
 	 * NOTE: All we care about are EC-SCI's.  Other EC events are
@@ -763,8 +770,7 @@ static u32 acpi_ec_gpe_poll_handler(void
 
 	acpi_disable_gpe(NULL, ec->common.gpe_bit, ACPI_ISR);
 
-	status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
-					     acpi_ec_gpe_query, ec);
+	status = acpi_os_execute(OSL_EC_POLL_HANDLER, acpi_ec_gpe_query, ec);
 
 	if (status == AE_OK)
 		return ACPI_INTERRUPT_HANDLED;
@@ -799,7 +805,7 @@ static u32 acpi_ec_gpe_intr_handler(void
 
 	if (value & ACPI_EC_FLAG_SCI) {
 		atomic_add(1, &ec->intr.pending_gpe);
-		status = acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
+		status = acpi_os_execute(OSL_EC_BURST_HANDLER,
 						     acpi_ec_gpe_query, ec);
 		return status == AE_OK ?
 		    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
@@ -991,7 +997,6 @@ static int acpi_ec_poll_add(struct acpi_
 	int result = 0;
 	acpi_status status = AE_OK;
 	union acpi_ec *ec = NULL;
-	unsigned long uid;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add");
 
@@ -1005,7 +1010,7 @@ static int acpi_ec_poll_add(struct acpi_
 
 	ec->common.handle = device->handle;
 	ec->common.uid = -1;
-	spin_lock_init(&ec->poll.lock);
+	init_MUTEX(&ec->poll.sem);
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 	acpi_driver_data(device) = ec;
@@ -1014,10 +1019,9 @@ static int acpi_ec_poll_add(struct acpi_
 	acpi_evaluate_integer(ec->common.handle, "_GLK", NULL,
 			      &ec->common.global_lock);
 
-	/* If our UID matches the UID for the ECDT-enumerated EC,
-	   we now have the *real* EC info, so kill the makeshift one. */
-	acpi_evaluate_integer(ec->common.handle, "_UID", NULL, &uid);
-	if (ec_ecdt && ec_ecdt->common.uid == uid) {
+	/* XXX we don't test uids, because on some boxes ecdt uid = 0, see:
+	   http://bugzilla.kernel.org/show_bug.cgi?id=6111 */
+	if (ec_ecdt) {
 		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
 						  ACPI_ADR_SPACE_EC,
 						  &acpi_ec_space_handler);
@@ -1062,7 +1066,6 @@ static int acpi_ec_intr_add(struct acpi_
 	int result = 0;
 	acpi_status status = AE_OK;
 	union acpi_ec *ec = NULL;
-	unsigned long uid;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_add");
 
@@ -1088,10 +1091,9 @@ static int acpi_ec_intr_add(struct acpi_
 	acpi_evaluate_integer(ec->common.handle, "_GLK", NULL,
 			      &ec->common.global_lock);
 
-	/* If our UID matches the UID for the ECDT-enumerated EC,
-	   we now have the *real* EC info, so kill the makeshift one. */
-	acpi_evaluate_integer(ec->common.handle, "_UID", NULL, &uid);
-	if (ec_ecdt && ec_ecdt->common.uid == uid) {
+	/* XXX we don't test uids, because on some boxes ecdt uid = 0, see:
+	   http://bugzilla.kernel.org/show_bug.cgi?id=6111 */
+	if (ec_ecdt) {
 		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
 						  ACPI_ADR_SPACE_EC,
 						  &acpi_ec_space_handler);
@@ -1300,7 +1302,7 @@ acpi_fake_ecdt_poll_callback(acpi_handle
 				  &ec_ecdt->common.gpe_bit);
 	if (ACPI_FAILURE(status))
 		return status;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->poll.sem);
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.handle = handle;
 
@@ -1416,7 +1418,7 @@ static int __init acpi_ec_poll_get_real_
 	ec_ecdt->common.status_addr = ecdt_ptr->ec_control;
 	ec_ecdt->common.data_addr = ecdt_ptr->ec_data;
 	ec_ecdt->common.gpe_bit = ecdt_ptr->gpe_bit;
-	spin_lock_init(&ec_ecdt->poll.lock);
+	init_MUTEX(&ec_ecdt->poll.sem);
 	/* use the GL just to be safe */
 	ec_ecdt->common.global_lock = TRUE;
 	ec_ecdt->common.uid = ecdt_ptr->uid;