ACPI: EC: clean up tmp variable before reuse

PATCH In review Alexey Starikovskiy 4 participants 12 messages Nov 9, 2008 – Nov 12, 2008 1 message not in the archive ACPI

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20081109155847.13635.15244.stgit@thinkpad

Alexey Starikovskiy

author

2008-11-09 16:00 UTC #1 lore raw

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

Fix breakage introduced by following patch 27663c5855b10af9ec67bc7dfba001426ba21222 is first bad commit commit 27663c5855b10af9ec67bc7dfba001426ba21222

Author: Matthew Wilcox <[email protected]>
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 <[email protected]>

Tested-by Alan Jenkins <[email protected]>

---

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

drivers/acpi/ec.c +2 -1

index ef42316..523ac5b 100644
@@ -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;

Len Brown

reviewer

2008-11-10 20:55 UTC #2 lore raw

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

thanks,
-len

Quoted 48 lines · Alexey Starikovskiy

Fix breakage introduced by following patch 27663c5855b10af9ec67bc7dfba001426ba21222 is first bad commit commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]>
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 <[email protected]>
Tested-by Alan Jenkins <[email protected]>
---
 drivers/acpi/ec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/acpi/ec.c +2 -2
index ef42316..523ac5b 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

author

2008-11-09 16:00 UTC #3 lore raw

PATCH 2/5 ACPI: EC: revert msleep patch

In reply to a message not in the archive

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 <[email protected]>

---

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

drivers/acpi/ec.c +2 -2

index 523ac5b..e5dbe21 100644
@@ -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;
 	}

Alexey Starikovskiy

author

2008-11-09 16:00 UTC #4 lore raw

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

In reply to a message not in the archive

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 <[email protected]>

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +13 -8

index e5dbe21..d6007ac 100644
@@ -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)

Len Brown

reviewer

2008-11-11 18:27 UTC #5 lore raw

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

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

Quoted 77 lines · Alexey Starikovskiy

References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4

Cc Zhao Yakui <[email protected]>

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +13 -9

index e5dbe21..d6007ac 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

2008-11-11 20:13 UTC #6 lore raw

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

Quoted 7 lines · 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?

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.

Quoted 90 lines

thanks,
-Len

Quoted 73 lines · Alexey Starikovskiy

References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4

Cc Zhao Yakui <[email protected]>

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +13 -9

index e5dbe21..d6007ac 100644
@@ -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)
 
--

Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Zhao Yakui

2008-11-12 01:27 UTC #7 lore raw

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

In reply to Len Brown

Quoted 5 lines · 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?

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

Quoted 88 lines

thanks,
-Len

Quoted 73 lines · Alexey Starikovskiy

References: http://marc.info/?l=linux-acpi&m=122616284402886&w=4

Cc Zhao Yakui <[email protected]>

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +13 -9

index e5dbe21..d6007ac 100644
@@ -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)
 
--

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

author

2008-11-09 16:01 UTC #8 lore raw

PATCH 4/5 ACPI: EC: restart failed command

In reply to a message not in the archive

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 <[email protected]>

---

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

drivers/acpi/ec.c +27 ~~-14~~

index d6007ac..9646604 100644
@@ -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);

Alexey Starikovskiy

2008-11-10 21:09 UTC #9 lore raw

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

Len,
Please don't apply this patch

Thanks,
Alex.

Quoted 98 lines · Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +27 ~~-15~~

index d6007ac..9646604 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Len Brown

reviewer

2008-11-11 18:10 UTC #10 lore raw

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

In reply to Alexey Starikovskiy

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

Quoted 98 lines · Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +27 ~~-15~~

index d6007ac..9646604 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

author

2008-11-09 16:01 UTC #11 lore raw

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

In reply to a message not in the archive

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11892

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ec.c +1 -1

index 9646604..628b076 100644
@@ -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 {

Len Brown

reviewer

2008-11-11 18:12 UTC #12 lore raw

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

applied to acpi-test

Quoted 27 lines · Alexey Starikovskiy

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=11892
Signed-off-by Alexey Starikovskiy <[email protected]>
---
 drivers/acpi/ec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/ec.c +1 -2
index 9646604..628b076 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html