[PATCH 1/5] ACPI: EC: clean up tmp variable before reuse

[PATCH 1/5] ACPI: EC: clean up tmp variable before reuse

[Alexey Starikovskiy]

Fix breakage introduced by following patch
27663c5855b10af9ec67bc7dfba001426ba21222 is first bad commit
commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <willy@linux.intel.com>
Date:   Fri Oct 10 02:22:59 2008 -0400

acpi_evaluate_integer() does not clear passed variable if
there is an error at evaluation.
So if we ignore error, we must supply initialized variable.

References: http://bugzilla.kernel.org/show_bug.cgi?id=11917
	    http://bugzilla.kernel.org/show_bug.cgi?id=11896

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---

 drivers/acpi/ec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ef42316..523ac5b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -736,7 +736,7 @@ static acpi_status
 ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 {
 	acpi_status status;
-	unsigned long long tmp;
+	unsigned long long tmp = 0;
 
 	struct acpi_ec *ec = context;
 	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
@@ -751,6 +751,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 		return status;
 	ec->gpe = tmp;
 	/* Use the global lock for all EC transactions? */
+	tmp = 0;
 	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
 	ec->global_lock = tmp;
 	ec->handle = handle;

[PATCH 2/5] ACPI: EC: revert msleep patch

[Alexey Starikovskiy]

With the better solution for EC interrupt storm issue,
there is no need to use msleep over udelay.

References:
	http://bugzilla.kernel.org/show_bug.cgi?id=11810
	http://bugzilla.kernel.org/show_bug.cgi?id=10724


Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 523ac5b..e5dbe21 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -239,10 +239,10 @@ static int ec_check_sci(struct acpi_ec *ec, u8 state)
 static int ec_poll(struct acpi_ec *ec)
 {
 	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-	msleep(1);
+	udelay(ACPI_EC_UDELAY);
 	while (time_before(jiffies, delay)) {
 		gpe_transaction(ec, acpi_ec_read_status(ec));
-		msleep(1);
+		udelay(ACPI_EC_UDELAY);
 		if (ec_transaction_done(ec))
 			return 0;
 	}

[PATCH 3/5] ACPI: EC: wait for last write gpe

[Alexey Starikovskiy]

There is a possibility that EC might break if next command is
issued within 1 us after write or burst-disable command.
This "possibility" was in EC driver for 3.5 years, after
451566f45a2e6cd10ba56e7220a9dd84ba3ef550.

References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
Cc: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index e5dbe21..d6007ac 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -102,6 +102,7 @@ struct transaction {
 	u8 command;
 	u8 wlen;
 	u8 rlen;
+	bool done;
 };
 
 static struct acpi_ec {
@@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)
 	unsigned long flags;
 	int ret = 0;
 	spin_lock_irqsave(&ec->curr_lock, flags);
-	if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen))
+	if (!ec->curr || ec->curr->done)
 		ret = 1;
 	spin_unlock_irqrestore(&ec->curr_lock, flags);
 	return ret;
@@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)
 			acpi_ec_write_data(ec, *(ec->curr->wdata++));
 			--ec->curr->wlen;
 		} else
-			/* false interrupt, state didn't change */
-			++ec->curr->irq_count;
-
+			goto err;
 	} else if (ec->curr->rlen > 0) {
 		if ((status & ACPI_EC_FLAG_OBF) == 1) {
 			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
-			--ec->curr->rlen;
+			if (--ec->curr->rlen == 0)
+				ec->curr->done = true;
 		} else
-			/* false interrupt, state didn't change */
-			++ec->curr->irq_count;
-	}
+			goto err;
+	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
+		ec->curr->done = true;
+	goto unlock;
+err:
+	/* false interrupt, state didn't change */
+	++ec->curr->irq_count;
 unlock:
 	spin_unlock_irqrestore(&ec->curr_lock, flags);
 }
@@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	spin_lock_irqsave(&ec->curr_lock, tmp);
 	/* following two actions should be kept atomic */
 	t->irq_count = 0;
+	t->done = false;
 	ec->curr = t;
 	acpi_ec_write_cmd(ec, ec->curr->command);
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)

[PATCH 4/5] ACPI: EC: restart failed command

[Alexey Starikovskiy]

Restart current transaction if status unexpectedly becomes clear.
There is an external timeout for transaction completion, thus there is
no need to place additional restriction on restart count.
Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11896
Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |   41 +++++++++++++++++++++++++++--------------
 1 files changed, 27 insertions(+), 14 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d6007ac..9646604 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -100,6 +100,8 @@ struct transaction {
 	u8 *rdata;
 	unsigned short irq_count;
 	u8 command;
+	u8 wi;
+	u8 ri;
 	u8 wlen;
 	u8 rlen;
 	bool done;
@@ -185,31 +187,44 @@ static int ec_transaction_done(struct acpi_ec *ec)
 	return ret;
 }
 
+static void start_transaction(struct acpi_ec *ec)
+{
+	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
+	ec->curr->done = false;
+	acpi_ec_write_cmd(ec, ec->curr->command);
+}
+
 static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
 	unsigned long flags;
 	spin_lock_irqsave(&ec->curr_lock, flags);
 	if (!ec->curr)
 		goto unlock;
-	if (ec->curr->wlen > 0) {
-		if ((status & ACPI_EC_FLAG_IBF) == 0) {
-			acpi_ec_write_data(ec, *(ec->curr->wdata++));
-			--ec->curr->wlen;
-		} else
+	if (ec->curr->wlen > ec->curr->wi) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0)
+			acpi_ec_write_data(ec,
+				ec->curr->wdata[ec->curr->wi++]);
+		else
 			goto err;
-	} else if (ec->curr->rlen > 0) {
+	} else if (ec->curr->rlen > ec->curr->ri) {
 		if ((status & ACPI_EC_FLAG_OBF) == 1) {
-			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
-			if (--ec->curr->rlen == 0)
+			ec->curr->rdata[ec->curr->ri++] = acpi_ec_read_data(ec);
+			if (ec->curr->rlen == ec->curr->ri)
 				ec->curr->done = true;
 		} else
 			goto err;
-	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
+	} else if (ec->curr->wlen == ec->curr->wi &&
+		   (status & ACPI_EC_FLAG_IBF) == 0)
 		ec->curr->done = true;
 	goto unlock;
 err:
-	/* false interrupt, state didn't change */
-	++ec->curr->irq_count;
+	/* restart command */
+	if (!status) {
+		pr_debug(PREFIX "controller reset, restart transaction\n");
+		start_transaction(ec);
+	} else
+		/* false interrupt, state didn't change */
+		++ec->curr->irq_count;
 unlock:
 	spin_unlock_irqrestore(&ec->curr_lock, flags);
 }
@@ -268,10 +283,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	/* start transaction */
 	spin_lock_irqsave(&ec->curr_lock, tmp);
 	/* following two actions should be kept atomic */
-	t->irq_count = 0;
-	t->done = false;
 	ec->curr = t;
-	acpi_ec_write_cmd(ec, ec->curr->command);
+	start_transaction(ec);
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	spin_unlock_irqrestore(&ec->curr_lock, tmp);

[PATCH 5/5] ACPI: EC: lower interrupt storm treshold

[Alexey Starikovskiy]

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11892
Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

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


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 9646604..628b076 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -70,7 +70,7 @@ enum ec_command {
 #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
+#define ACPI_EC_STORM_THRESHOLD 8	/* number of false interrupts
 					   per one transaction */
 
 enum {

Re: [PATCH 1/5] ACPI: EC: clean up tmp variable before reuse

[Len Brown]

please don't re-send send me patches that i've already acked
as checked in.

thanks,
-len

On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:

> Fix breakage introduced by following patch
> 27663c5855b10af9ec67bc7dfba001426ba21222 is first bad commit
> commit 27663c5855b10af9ec67bc7dfba001426ba21222
> Author: Matthew Wilcox <willy@linux.intel.com>
> Date:   Fri Oct 10 02:22:59 2008 -0400
> 
> acpi_evaluate_integer() does not clear passed variable if
> there is an error at evaluation.
> So if we ignore error, we must supply initialized variable.
> 
> References: http://bugzilla.kernel.org/show_bug.cgi?id=11917
> 	    http://bugzilla.kernel.org/show_bug.cgi?id=11896
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> ---
> 
>  drivers/acpi/ec.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index ef42316..523ac5b 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -736,7 +736,7 @@ static acpi_status
>  ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  {
>  	acpi_status status;
> -	unsigned long long tmp;
> +	unsigned long long tmp = 0;
>  
>  	struct acpi_ec *ec = context;
>  	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> @@ -751,6 +751,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  		return status;
>  	ec->gpe = tmp;
>  	/* Use the global lock for all EC transactions? */
> +	tmp = 0;
>  	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>  	ec->global_lock = tmp;
>  	ec->handle = handle;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/5] ACPI: EC: restart failed command

[Alexey Starikovskiy]

Len,
Please don't apply this patch

Thanks,
Alex.

Alexey Starikovskiy wrote:
> Restart current transaction if status unexpectedly becomes clear.
> There is an external timeout for transaction completion, thus there is
> no need to place additional restriction on restart count.
> Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11896
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
>
>  drivers/acpi/ec.c |   41 +++++++++++++++++++++++++++--------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
>
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index d6007ac..9646604 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -100,6 +100,8 @@ struct transaction {
>  	u8 *rdata;
>  	unsigned short irq_count;
>  	u8 command;
> +	u8 wi;
> +	u8 ri;
>  	u8 wlen;
>  	u8 rlen;
>  	bool done;
> @@ -185,31 +187,44 @@ static int ec_transaction_done(struct acpi_ec *ec)
>  	return ret;
>  }
>  
> +static void start_transaction(struct acpi_ec *ec)
> +{
> +	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> +	ec->curr->done = false;
> +	acpi_ec_write_cmd(ec, ec->curr->command);
> +}
> +
>  static void gpe_transaction(struct acpi_ec *ec, u8 status)
>  {
>  	unsigned long flags;
>  	spin_lock_irqsave(&ec->curr_lock, flags);
>  	if (!ec->curr)
>  		goto unlock;
> -	if (ec->curr->wlen > 0) {
> -		if ((status & ACPI_EC_FLAG_IBF) == 0) {
> -			acpi_ec_write_data(ec, *(ec->curr->wdata++));
> -			--ec->curr->wlen;
> -		} else
> +	if (ec->curr->wlen > ec->curr->wi) {
> +		if ((status & ACPI_EC_FLAG_IBF) == 0)
> +			acpi_ec_write_data(ec,
> +				ec->curr->wdata[ec->curr->wi++]);
> +		else
>  			goto err;
> -	} else if (ec->curr->rlen > 0) {
> +	} else if (ec->curr->rlen > ec->curr->ri) {
>  		if ((status & ACPI_EC_FLAG_OBF) == 1) {
> -			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
> -			if (--ec->curr->rlen == 0)
> +			ec->curr->rdata[ec->curr->ri++] = acpi_ec_read_data(ec);
> +			if (ec->curr->rlen == ec->curr->ri)
>  				ec->curr->done = true;
>  		} else
>  			goto err;
> -	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
> +	} else if (ec->curr->wlen == ec->curr->wi &&
> +		   (status & ACPI_EC_FLAG_IBF) == 0)
>  		ec->curr->done = true;
>  	goto unlock;
>  err:
> -	/* false interrupt, state didn't change */
> -	++ec->curr->irq_count;
> +	/* restart command */
> +	if (!status) {
> +		pr_debug(PREFIX "controller reset, restart transaction\n");
> +		start_transaction(ec);
> +	} else
> +		/* false interrupt, state didn't change */
> +		++ec->curr->irq_count;
>  unlock:
>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>  }
> @@ -268,10 +283,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  	/* start transaction */
>  	spin_lock_irqsave(&ec->curr_lock, tmp);
>  	/* following two actions should be kept atomic */
> -	t->irq_count = 0;
> -	t->done = false;
>  	ec->curr = t;
> -	acpi_ec_write_cmd(ec, ec->curr->command);
> +	start_transaction(ec);
>  	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
>  		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  	spin_unlock_irqrestore(&ec->curr_lock, tmp);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH 4/5] ACPI: EC: restart failed command

[Len Brown]

ignored, per request of Alexey on IRC
due to thrash in bugzilla 11896

On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:

> Restart current transaction if status unexpectedly becomes clear.
> There is an external timeout for transaction completion, thus there is
> no need to place additional restriction on restart count.
> Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11896
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/ec.c |   41 +++++++++++++++++++++++++++--------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index d6007ac..9646604 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -100,6 +100,8 @@ struct transaction {
>  	u8 *rdata;
>  	unsigned short irq_count;
>  	u8 command;
> +	u8 wi;
> +	u8 ri;
>  	u8 wlen;
>  	u8 rlen;
>  	bool done;
> @@ -185,31 +187,44 @@ static int ec_transaction_done(struct acpi_ec *ec)
>  	return ret;
>  }
>  
> +static void start_transaction(struct acpi_ec *ec)
> +{
> +	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> +	ec->curr->done = false;
> +	acpi_ec_write_cmd(ec, ec->curr->command);
> +}
> +
>  static void gpe_transaction(struct acpi_ec *ec, u8 status)
>  {
>  	unsigned long flags;
>  	spin_lock_irqsave(&ec->curr_lock, flags);
>  	if (!ec->curr)
>  		goto unlock;
> -	if (ec->curr->wlen > 0) {
> -		if ((status & ACPI_EC_FLAG_IBF) == 0) {
> -			acpi_ec_write_data(ec, *(ec->curr->wdata++));
> -			--ec->curr->wlen;
> -		} else
> +	if (ec->curr->wlen > ec->curr->wi) {
> +		if ((status & ACPI_EC_FLAG_IBF) == 0)
> +			acpi_ec_write_data(ec,
> +				ec->curr->wdata[ec->curr->wi++]);
> +		else
>  			goto err;
> -	} else if (ec->curr->rlen > 0) {
> +	} else if (ec->curr->rlen > ec->curr->ri) {
>  		if ((status & ACPI_EC_FLAG_OBF) == 1) {
> -			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
> -			if (--ec->curr->rlen == 0)
> +			ec->curr->rdata[ec->curr->ri++] = acpi_ec_read_data(ec);
> +			if (ec->curr->rlen == ec->curr->ri)
>  				ec->curr->done = true;
>  		} else
>  			goto err;
> -	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
> +	} else if (ec->curr->wlen == ec->curr->wi &&
> +		   (status & ACPI_EC_FLAG_IBF) == 0)
>  		ec->curr->done = true;
>  	goto unlock;
>  err:
> -	/* false interrupt, state didn't change */
> -	++ec->curr->irq_count;
> +	/* restart command */
> +	if (!status) {
> +		pr_debug(PREFIX "controller reset, restart transaction\n");
> +		start_transaction(ec);
> +	} else
> +		/* false interrupt, state didn't change */
> +		++ec->curr->irq_count;
>  unlock:
>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>  }
> @@ -268,10 +283,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  	/* start transaction */
>  	spin_lock_irqsave(&ec->curr_lock, tmp);
>  	/* following two actions should be kept atomic */
> -	t->irq_count = 0;
> -	t->done = false;
>  	ec->curr = t;
> -	acpi_ec_write_cmd(ec, ec->curr->command);
> +	start_transaction(ec);
>  	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
>  		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  	spin_unlock_irqrestore(&ec->curr_lock, tmp);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5] ACPI: EC: lower interrupt storm treshold

[Len Brown]

applied to acpi-test

On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:

> Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11892
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/ec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 9646604..628b076 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -70,7 +70,7 @@ enum ec_command {
>  #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
> +#define ACPI_EC_STORM_THRESHOLD 8	/* number of false interrupts
>  					   per one transaction */
>  
>  enum {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] ACPI: EC: wait for last write gpe

[Len Brown]

applied to acpi-test.

we struggled with ec burst mode for a long time before enabling it
finally in 2005.  have we had any problems with it since then
that might be addressed by this race?

thanks,
-Len


On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:

> There is a possibility that EC might break if next command is
> issued within 1 us after write or burst-disable command.
> This "possibility" was in EC driver for 3.5 years, after
> 451566f45a2e6cd10ba56e7220a9dd84ba3ef550.
> 
> References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
> Cc: Zhao Yakui <yakui.zhao@intel.com>
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> 
>  drivers/acpi/ec.c |   21 +++++++++++++--------
>  1 files changed, 13 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index e5dbe21..d6007ac 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -102,6 +102,7 @@ struct transaction {
>  	u8 command;
>  	u8 wlen;
>  	u8 rlen;
> +	bool done;
>  };
>  
>  static struct acpi_ec {
> @@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)
>  	unsigned long flags;
>  	int ret = 0;
>  	spin_lock_irqsave(&ec->curr_lock, flags);
> -	if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen))
> +	if (!ec->curr || ec->curr->done)
>  		ret = 1;
>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>  	return ret;
> @@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)
>  			acpi_ec_write_data(ec, *(ec->curr->wdata++));
>  			--ec->curr->wlen;
>  		} else
> -			/* false interrupt, state didn't change */
> -			++ec->curr->irq_count;
> -
> +			goto err;
>  	} else if (ec->curr->rlen > 0) {
>  		if ((status & ACPI_EC_FLAG_OBF) == 1) {
>  			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
> -			--ec->curr->rlen;
> +			if (--ec->curr->rlen == 0)
> +				ec->curr->done = true;
>  		} else
> -			/* false interrupt, state didn't change */
> -			++ec->curr->irq_count;
> -	}
> +			goto err;
> +	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
> +		ec->curr->done = true;
> +	goto unlock;
> +err:
> +	/* false interrupt, state didn't change */
> +	++ec->curr->irq_count;
>  unlock:
>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>  }
> @@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  	spin_lock_irqsave(&ec->curr_lock, tmp);
>  	/* following two actions should be kept atomic */
>  	t->irq_count = 0;
> +	t->done = false;
>  	ec->curr = t;
>  	acpi_ec_write_cmd(ec, ec->curr->command);
>  	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] ACPI: EC: wait for last write gpe

[Alexey Starikovskiy]

Len Brown wrote:
> applied to acpi-test.
>
> we struggled with ec burst mode for a long time before enabling it
> finally in 2005.  have we had any problems with it since then
> that might be addressed by this race?
>
>   
hard to say, I was thinking that with the fast transaction, we may fall 
into this window,
but it does not help in any of the current bug reports.

regards,
Alex.
> thanks,
> -Len
>
>
> On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:
>
>   
>> There is a possibility that EC might break if next command is
>> issued within 1 us after write or burst-disable command.
>> This "possibility" was in EC driver for 3.5 years, after
>> 451566f45a2e6cd10ba56e7220a9dd84ba3ef550.
>>
>> References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
>> Cc: Zhao Yakui <yakui.zhao@intel.com>
>> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
>> ---
>>
>>  drivers/acpi/ec.c |   21 +++++++++++++--------
>>  1 files changed, 13 insertions(+), 8 deletions(-)
>>
>>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index e5dbe21..d6007ac 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -102,6 +102,7 @@ struct transaction {
>>  	u8 command;
>>  	u8 wlen;
>>  	u8 rlen;
>> +	bool done;
>>  };
>>  
>>  static struct acpi_ec {
>> @@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)
>>  	unsigned long flags;
>>  	int ret = 0;
>>  	spin_lock_irqsave(&ec->curr_lock, flags);
>> -	if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen))
>> +	if (!ec->curr || ec->curr->done)
>>  		ret = 1;
>>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>>  	return ret;
>> @@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)
>>  			acpi_ec_write_data(ec, *(ec->curr->wdata++));
>>  			--ec->curr->wlen;
>>  		} else
>> -			/* false interrupt, state didn't change */
>> -			++ec->curr->irq_count;
>> -
>> +			goto err;
>>  	} else if (ec->curr->rlen > 0) {
>>  		if ((status & ACPI_EC_FLAG_OBF) == 1) {
>>  			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
>> -			--ec->curr->rlen;
>> +			if (--ec->curr->rlen == 0)
>> +				ec->curr->done = true;
>>  		} else
>> -			/* false interrupt, state didn't change */
>> -			++ec->curr->irq_count;
>> -	}
>> +			goto err;
>> +	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
>> +		ec->curr->done = true;
>> +	goto unlock;
>> +err:
>> +	/* false interrupt, state didn't change */
>> +	++ec->curr->irq_count;
>>  unlock:
>>  	spin_unlock_irqrestore(&ec->curr_lock, flags);
>>  }
>> @@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>>  	spin_lock_irqsave(&ec->curr_lock, tmp);
>>  	/* following two actions should be kept atomic */
>>  	t->irq_count = 0;
>> +	t->done = false;
>>  	ec->curr = t;
>>  	acpi_ec_write_cmd(ec, ec->curr->command);
>>  	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH 3/5] ACPI: EC: wait for last write gpe

[Zhao Yakui]

On Wed, 2008-11-12 at 02:26 +0800, Len Brown wrote:
> applied to acpi-test.
> 
> we struggled with ec burst mode for a long time before enabling it
> finally in 2005.  have we had any problems with it since then
> that might be addressed by this race?
We don't know whether this race really happens. Even when it happens, it
will be mixed up with other problem. It is difficult to identify.In fact
before Alexey's patch is shipped, there is no such issue that OS can
begin another EC transaction before the previous EC transaction is
really finished(In previous kernel only when EC transaction is really
finished, the EC mutex will be released. It means that OS can begin
another EC transaction).

Another reason is that OS had better wait for the end of EC transaction
and confirm whether the EC transaction is successful. If the program
logic states that the EC transaction is already finished and successful,
the failure in EC transaction can't be detected in time. So to make the
EC transaction more stable it is appropriate that OS should wait for the
end of EC transaction.

Best regards.
   Yakui
> 
> thanks,
> -Len
> 
> 
> On Sun, 9 Nov 2008, Alexey Starikovskiy wrote:
> 
> > There is a possibility that EC might break if next command is
> > issued within 1 us after write or burst-disable command.
> > This "possibility" was in EC driver for 3.5 years, after
> > 451566f45a2e6cd10ba56e7220a9dd84ba3ef550.
> > 
> > References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4
> > Cc: Zhao Yakui <yakui.zhao@intel.com>
> > Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> > ---
> > 
> >  drivers/acpi/ec.c |   21 +++++++++++++--------
> >  1 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index e5dbe21..d6007ac 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -102,6 +102,7 @@ struct transaction {
> >  	u8 command;
> >  	u8 wlen;
> >  	u8 rlen;
> > +	bool done;
> >  };
> >  
> >  static struct acpi_ec {
> > @@ -178,7 +179,7 @@ static int ec_transaction_done(struct acpi_ec *ec)
> >  	unsigned long flags;
> >  	int ret = 0;
> >  	spin_lock_irqsave(&ec->curr_lock, flags);
> > -	if (!ec->curr || (!ec->curr->wlen && !ec->curr->rlen))
> > +	if (!ec->curr || ec->curr->done)
> >  		ret = 1;
> >  	spin_unlock_irqrestore(&ec->curr_lock, flags);
> >  	return ret;
> > @@ -195,17 +196,20 @@ static void gpe_transaction(struct acpi_ec *ec, u8 status)
> >  			acpi_ec_write_data(ec, *(ec->curr->wdata++));
> >  			--ec->curr->wlen;
> >  		} else
> > -			/* false interrupt, state didn't change */
> > -			++ec->curr->irq_count;
> > -
> > +			goto err;
> >  	} else if (ec->curr->rlen > 0) {
> >  		if ((status & ACPI_EC_FLAG_OBF) == 1) {
> >  			*(ec->curr->rdata++) = acpi_ec_read_data(ec);
> > -			--ec->curr->rlen;
> > +			if (--ec->curr->rlen == 0)
> > +				ec->curr->done = true;
> >  		} else
> > -			/* false interrupt, state didn't change */
> > -			++ec->curr->irq_count;
> > -	}
> > +			goto err;
> > +	} else if (ec->curr->wlen == 0 && (status & ACPI_EC_FLAG_IBF) == 0)
> > +		ec->curr->done = true;
> > +	goto unlock;
> > +err:
> > +	/* false interrupt, state didn't change */
> > +	++ec->curr->irq_count;
> >  unlock:
> >  	spin_unlock_irqrestore(&ec->curr_lock, flags);
> >  }
> > @@ -265,6 +269,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
> >  	spin_lock_irqsave(&ec->curr_lock, tmp);
> >  	/* following two actions should be kept atomic */
> >  	t->irq_count = 0;
> > +	t->done = false;
> >  	ec->curr = t;
> >  	acpi_ec_write_cmd(ec, ec->curr->command);
> >  	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html