diff mbox series

[v1] ACPI: EC: Do not release locks during operation region accesses

Message ID 12473338.O9o76ZdvQC@rjwysocki.net (mailing list archive)
State Mainlined, archived
Headers show
Series [v1] ACPI: EC: Do not release locks during operation region accesses | expand

Commit Message

Rafael J. Wysocki July 4, 2024, 4:26 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is not particularly useful to release locks (the EC mutex and the
ACPI global lock, if present) and re-acquire them immediately thereafter
during EC address space accesses in acpi_ec_space_handler().

First, releasing them for a while before grabbing them again does not
really help anyone because there may not be enough time for another
thread to acquire them.

Second, if another thread successfully acquires them and carries out
a new EC write or read in the middle if an operation region access in
progress, it may confuse the EC firmware, especially after the burst
mode has been enabled.

Finally, manipulating the locks after writing or reading every single
byte of data is overhead that it is better to avoid.

Accordingly, modify the code to carry out EC address space accesses
entirely without releasing the locks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

For 6.12.

---
 drivers/acpi/ec.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

Comments

Hans de Goede July 12, 2024, 2:59 p.m. UTC | #1 
Hi,

On 7/4/24 6:26 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is not particularly useful to release locks (the EC mutex and the
> ACPI global lock, if present) and re-acquire them immediately thereafter
> during EC address space accesses in acpi_ec_space_handler().
> 
> First, releasing them for a while before grabbing them again does not
> really help anyone because there may not be enough time for another
> thread to acquire them.
> 
> Second, if another thread successfully acquires them and carries out
> a new EC write or read in the middle if an operation region access in
> progress, it may confuse the EC firmware, especially after the burst
> mode has been enabled.
> 
> Finally, manipulating the locks after writing or reading every single
> byte of data is overhead that it is better to avoid.
> 
> Accordingly, modify the code to carry out EC address space accesses
> entirely without releasing the locks.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
> 
> For 6.12.
> 
> ---
>  drivers/acpi/ec.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -783,6 +783,9 @@ static int acpi_ec_transaction_unlocked(
>  	unsigned long tmp;
>  	int ret = 0;
>  
> +	if (t->rdata)
> +		memset(t->rdata, 0, t->rlen);
> +
>  	/* start transaction */
>  	spin_lock_irqsave(&ec->lock, tmp);
>  	/* Enable GPE for command processing (IBF=0/OBF=1) */
> @@ -819,8 +822,6 @@ static int acpi_ec_transaction(struct ac
>  
>  	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
>  		return -EINVAL;
> -	if (t->rdata)
> -		memset(t->rdata, 0, t->rlen);
>  
>  	mutex_lock(&ec->mutex);
>  	if (ec->global_lock) {
> @@ -847,7 +848,7 @@ static int acpi_ec_burst_enable(struct a
>  				.wdata = NULL, .rdata = &d,
>  				.wlen = 0, .rlen = 1};
>  
> -	return acpi_ec_transaction(ec, &t);
> +	return acpi_ec_transaction_unlocked(ec, &t);
>  }
>  
>  static int acpi_ec_burst_disable(struct acpi_ec *ec)
> @@ -857,7 +858,7 @@ static int acpi_ec_burst_disable(struct
>  				.wlen = 0, .rlen = 0};
>  
>  	return (acpi_ec_read_status(ec) & ACPI_EC_FLAG_BURST) ?
> -				acpi_ec_transaction(ec, &t) : 0;
> +				acpi_ec_transaction_unlocked(ec, &t) : 0;
>  }
>  
>  static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
> @@ -873,6 +874,19 @@ static int acpi_ec_read(struct acpi_ec *
>  	return result;
>  }
>  
> +static int acpi_ec_read_unlocked(struct acpi_ec *ec, u8 address, u8 *data)
> +{
> +	int result;
> +	u8 d;
> +	struct transaction t = {.command = ACPI_EC_COMMAND_READ,
> +				.wdata = &address, .rdata = &d,
> +				.wlen = 1, .rlen = 1};
> +
> +	result = acpi_ec_transaction_unlocked(ec, &t);
> +	*data = d;
> +	return result;
> +}
> +
>  static int acpi_ec_write(struct acpi_ec *ec, u8 address, u8 data)
>  {
>  	u8 wdata[2] = { address, data };
> @@ -883,6 +897,16 @@ static int acpi_ec_write(struct acpi_ec
>  	return acpi_ec_transaction(ec, &t);
>  }
>  
> +static int acpi_ec_write_unlocked(struct acpi_ec *ec, u8 address, u8 data)
> +{
> +	u8 wdata[2] = { address, data };
> +	struct transaction t = {.command = ACPI_EC_COMMAND_WRITE,
> +				.wdata = wdata, .rdata = NULL,
> +				.wlen = 2, .rlen = 0};
> +
> +	return acpi_ec_transaction_unlocked(ec, &t);
> +}
> +
>  int ec_read(u8 addr, u8 *val)
>  {
>  	int err;
> @@ -1323,6 +1347,7 @@ acpi_ec_space_handler(u32 function, acpi
>  	struct acpi_ec *ec = handler_context;
>  	int result = 0, i, bytes = bits / 8;
>  	u8 *value = (u8 *)value64;
> +	u32 glk;
>  
>  	if ((address > 0xFF) || !value || !handler_context)
>  		return AE_BAD_PARAMETER;
> @@ -1330,13 +1355,25 @@ acpi_ec_space_handler(u32 function, acpi
>  	if (function != ACPI_READ && function != ACPI_WRITE)
>  		return AE_BAD_PARAMETER;
>  
> +	mutex_lock(&ec->mutex);
> +
> +	if (ec->global_lock) {
> +		acpi_status status;
> +
> +		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
> +		if (ACPI_FAILURE(status)) {
> +			result = -ENODEV;
> +			goto unlock;
> +		}
> +	}
> +
>  	if (ec->busy_polling || bits > 8)
>  		acpi_ec_burst_enable(ec);
>  
>  	for (i = 0; i < bytes; ++i, ++address, ++value) {
>  		result = (function == ACPI_READ) ?
> -			acpi_ec_read(ec, address, value) :
> -			acpi_ec_write(ec, address, *value);
> +			acpi_ec_read_unlocked(ec, address, value) :
> +			acpi_ec_write_unlocked(ec, address, *value);
>  		if (result < 0)
>  			break;
>  	}
> @@ -1344,6 +1381,12 @@ acpi_ec_space_handler(u32 function, acpi
>  	if (ec->busy_polling || bits > 8)
>  		acpi_ec_burst_disable(ec);
>  
> +	if (ec->global_lock)
> +		acpi_release_global_lock(glk);
> +
> +unlock:
> +	mutex_unlock(&ec->mutex);
> +
>  	switch (result) {
>  	case -EINVAL:
>  		return AE_BAD_PARAMETER;
> 
> 
>
diff mbox series

Patch

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -783,6 +783,9 @@  static int acpi_ec_transaction_unlocked(
 	unsigned long tmp;
 	int ret = 0;
 
+	if (t->rdata)
+		memset(t->rdata, 0, t->rlen);
+
 	/* start transaction */
 	spin_lock_irqsave(&ec->lock, tmp);
 	/* Enable GPE for command processing (IBF=0/OBF=1) */
@@ -819,8 +822,6 @@  static int acpi_ec_transaction(struct ac
 
 	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
 		return -EINVAL;
-	if (t->rdata)
-		memset(t->rdata, 0, t->rlen);
 
 	mutex_lock(&ec->mutex);
 	if (ec->global_lock) {
@@ -847,7 +848,7 @@  static int acpi_ec_burst_enable(struct a
 				.wdata = NULL, .rdata = &d,
 				.wlen = 0, .rlen = 1};
 
-	return acpi_ec_transaction(ec, &t);
+	return acpi_ec_transaction_unlocked(ec, &t);
 }
 
 static int acpi_ec_burst_disable(struct acpi_ec *ec)
@@ -857,7 +858,7 @@  static int acpi_ec_burst_disable(struct
 				.wlen = 0, .rlen = 0};
 
 	return (acpi_ec_read_status(ec) & ACPI_EC_FLAG_BURST) ?
-				acpi_ec_transaction(ec, &t) : 0;
+				acpi_ec_transaction_unlocked(ec, &t) : 0;
 }
 
 static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
@@ -873,6 +874,19 @@  static int acpi_ec_read(struct acpi_ec *
 	return result;
 }
 
+static int acpi_ec_read_unlocked(struct acpi_ec *ec, u8 address, u8 *data)
+{
+	int result;
+	u8 d;
+	struct transaction t = {.command = ACPI_EC_COMMAND_READ,
+				.wdata = &address, .rdata = &d,
+				.wlen = 1, .rlen = 1};
+
+	result = acpi_ec_transaction_unlocked(ec, &t);
+	*data = d;
+	return result;
+}
+
 static int acpi_ec_write(struct acpi_ec *ec, u8 address, u8 data)
 {
 	u8 wdata[2] = { address, data };
@@ -883,6 +897,16 @@  static int acpi_ec_write(struct acpi_ec
 	return acpi_ec_transaction(ec, &t);
 }
 
+static int acpi_ec_write_unlocked(struct acpi_ec *ec, u8 address, u8 data)
+{
+	u8 wdata[2] = { address, data };
+	struct transaction t = {.command = ACPI_EC_COMMAND_WRITE,
+				.wdata = wdata, .rdata = NULL,
+				.wlen = 2, .rlen = 0};
+
+	return acpi_ec_transaction_unlocked(ec, &t);
+}
+
 int ec_read(u8 addr, u8 *val)
 {
 	int err;
@@ -1323,6 +1347,7 @@  acpi_ec_space_handler(u32 function, acpi
 	struct acpi_ec *ec = handler_context;
 	int result = 0, i, bytes = bits / 8;
 	u8 *value = (u8 *)value64;
+	u32 glk;
 
 	if ((address > 0xFF) || !value || !handler_context)
 		return AE_BAD_PARAMETER;
@@ -1330,13 +1355,25 @@  acpi_ec_space_handler(u32 function, acpi
 	if (function != ACPI_READ && function != ACPI_WRITE)
 		return AE_BAD_PARAMETER;
 
+	mutex_lock(&ec->mutex);
+
+	if (ec->global_lock) {
+		acpi_status status;
+
+		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
+		if (ACPI_FAILURE(status)) {
+			result = -ENODEV;
+			goto unlock;
+		}
+	}
+
 	if (ec->busy_polling || bits > 8)
 		acpi_ec_burst_enable(ec);
 
 	for (i = 0; i < bytes; ++i, ++address, ++value) {
 		result = (function == ACPI_READ) ?
-			acpi_ec_read(ec, address, value) :
-			acpi_ec_write(ec, address, *value);
+			acpi_ec_read_unlocked(ec, address, value) :
+			acpi_ec_write_unlocked(ec, address, *value);
 		if (result < 0)
 			break;
 	}
@@ -1344,6 +1381,12 @@  acpi_ec_space_handler(u32 function, acpi
 	if (ec->busy_polling || bits > 8)
 		acpi_ec_burst_disable(ec);
 
+	if (ec->global_lock)
+		acpi_release_global_lock(glk);
+
+unlock:
+	mutex_unlock(&ec->mutex);
+
 	switch (result) {
 	case -EINVAL:
 		return AE_BAD_PARAMETER;