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

[Hans de Goede]

Hi,

On 7/4/24 6:26 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> 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@xxxxxxxxx>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

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;
> 
> 
>