Commit c0d65341 authored by Lv Zheng's avatar Lv Zheng Committed by Rafael J. Wysocki
Browse files

ACPI / EC: Fix race condition in ec_transaction_completed() 

There is a race condition in ec_transaction_completed().

When ec_transaction_completed() is called in the GPE handler, it could
return true because of (ec->curr == NULL). Then the wake_up() invocation
could complete the next command unexpectedly since there is no lock between
the 2 invocations. With the previous cleanup, the IBF=0 waiter race need
not be handled any more. It's now safe to return a flag from
advance_condition() to indicate the requirement of wakeup, the flag is
returned from a locked context.

The ec_transaction_completed() is now only invoked by the ec_poll() where
the ec->curr is ensured to be different from NULL.

After cleaning up, the EVT_SCI=1 check should be moved out of the wakeup
condition so that an EVT_SCI raised with (ec->curr == NULL) can trigger a
QR_SC command.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=70891
Link: https://bugzilla.kernel.org/show_bug.cgi?id=63931
Link: https://bugzilla.kernel.org/show_bug.cgi?id=59911


Reported-and-tested-by: default avatarGareth Williams <gareth@garethwilliams.me.uk>
Reported-and-tested-by: default avatarHans de Goede <jwrdegoede@fedoraproject.org>
Reported-by: default avatarBarton Xu <tank.xuhan@gmail.com>
Tested-by: default avatarSteffen Weber <steffen.weber@gmail.com>
Tested-by: default avatarArthur Chen <axchen@nvidia.com>
Signed-off-by: default avatarLv Zheng <lv.zheng@intel.com>
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
parent 9b80f0f7

drivers/acpi/ec.c

+17 −13
Original line number Diff line number Diff line
@@ -158,16 +158,17 @@ static int ec_transaction_completed(struct acpi_ec *ec) 
	unsigned long flags;

	int ret = 0;

	spin_lock_irqsave(&ec->lock, flags);

	if (!ec->curr || (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))

	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))

		ret = 1;

	spin_unlock_irqrestore(&ec->lock, flags);

	return ret;

}



static void advance_transaction(struct acpi_ec *ec)

static bool advance_transaction(struct acpi_ec *ec)

{

	struct transaction *t;

	u8 status;

	bool wakeup = false;



	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");

	status = acpi_ec_read_status(ec);
@@ -183,21 +184,25 @@ static void advance_transaction(struct acpi_ec *ec) 
		} else if (t->rlen > t->ri) {

			if ((status & ACPI_EC_FLAG_OBF) == 1) {

				t->rdata[t->ri++] = acpi_ec_read_data(ec);

				if (t->rlen == t->ri)

				if (t->rlen == t->ri) {

					t->flags |= ACPI_EC_COMMAND_COMPLETE;

					wakeup = true;

				}

			} else

				goto err;

		} else if (t->wlen == t->wi &&

			   (status & ACPI_EC_FLAG_IBF) == 0)

			   (status & ACPI_EC_FLAG_IBF) == 0) {

			t->flags |= ACPI_EC_COMMAND_COMPLETE;

		return;

			wakeup = true;

		}

		return wakeup;

	} else {

		if ((status & ACPI_EC_FLAG_IBF) == 0) {

			acpi_ec_write_cmd(ec, t->command);

			t->flags |= ACPI_EC_COMMAND_POLL;

		} else

			goto err;

		return;

		return wakeup;

	}

err:

	/*
@@ -208,13 +213,14 @@ err: 
		if (in_interrupt() && t)

			++t->irq_count;

	}

	return wakeup;

}



static void start_transaction(struct acpi_ec *ec)

{

	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;

	ec->curr->flags = 0;

	advance_transaction(ec);

	(void)advance_transaction(ec);

}



static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
@@ -248,7 +254,7 @@ static int ec_poll(struct acpi_ec *ec) 
					return 0;

			}

			spin_lock_irqsave(&ec->lock, flags);

			advance_transaction(ec);

			(void)advance_transaction(ec);

			spin_unlock_irqrestore(&ec->lock, flags);

		} while (time_before(jiffies, delay));

		pr_debug("controller reset, restart transaction\n");
@@ -627,12 +633,10 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, 
	struct acpi_ec *ec = data;



	spin_lock_irqsave(&ec->lock, flags);

	advance_transaction(ec);

	spin_unlock_irqrestore(&ec->lock, flags);

	if (ec_transaction_completed(ec)) {

	if (advance_transaction(ec))

		wake_up(&ec->wait);

	spin_unlock_irqrestore(&ec->lock, flags);

	ec_check_sci(ec, acpi_ec_read_status(ec));

	}

	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE