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