From patchwork Thu Jul 4 16:26:54 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 13724004 Received: from cloudserver094114.home.pl (cloudserver094114.home.pl [79.96.170.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 352661AEFC9; Thu, 4 Jul 2024 16:26:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=79.96.170.134 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720110421; cv=none; b=Q4zQ4M6K+Y2BsPzTvK372YySUMFY6uEcY7l3xWWgU9OgR2TmtLvOKzW5kgikaC4q0TaCC/pSz1K5MFJ7cVvZMkvfT65OQGw24GFStLsFO0H7Seb34E62QZ4O/u7u1EndlKbfNBpMn6QaCcQIHp7HPTeQ+NQy6ue4IAUPwk4P5kw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720110421; c=relaxed/simple; bh=5pNezDxTRTgWQ3fPoix/Z760fcy8zD638XqFcSStAmM=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-Type; b=Ar7KUvhmvdR5O+T2rxNBGxrOeM/9kiXdatpa9xG+h2YIPG0A89U4mJEZBvM5AG1ReDJqMfmSXdtZZMawSeuF9dd6/GiIq3DkOdR3XzaJAcG0VajjXHKhEXw20cFmXw4UN5foVwtCJ8E4Opw1i/oxqCelV/V4qT7bNM8FheeKvLg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rjwysocki.net; spf=pass smtp.mailfrom=rjwysocki.net; dkim=fail (2048-bit key) header.d=rjwysocki.net header.i=@rjwysocki.net header.b=fqktYgRE reason="signature verification failed"; arc=none smtp.client-ip=79.96.170.134 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rjwysocki.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rjwysocki.net Authentication-Results: smtp.subspace.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=rjwysocki.net header.i=@rjwysocki.net header.b="fqktYgRE" Received: from localhost (127.0.0.1) (HELO v370.home.net.pl) by /usr/run/smtp (/usr/run/postfix/private/idea_relay_lmtp) via UNIX with SMTP (IdeaSmtpServer 6.2.0) id 28ce5c95349cd6ad; Thu, 4 Jul 2024 18:26:55 +0200 Received: from kreacher.localnet (unknown [195.136.19.94]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by cloudserver094114.home.pl (Postfix) with ESMTPSA id 338FCA20EA8; Thu, 4 Jul 2024 18:26:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=rjwysocki.net; s=dkim; t=1720110415; bh=5pNezDxTRTgWQ3fPoix/Z760fcy8zD638XqFcSStAmM=; h=From:To:Cc:Subject:Date; b=fqktYgRERVexuBPNcUYl4auemwL716Urnnb2ED/l4fbogpc9JHdLsCFnUDOr8c3xH XGyN5c2G8XnQsCkOdY4+A7MJmXVIiST/s15Mhrifvq/zQRB+o9nbc/acMm+SodxjvR nUscyC0rrkf/tR/o+JuEgDNnxxdcVDeaOnhLIX98s+FeB1LzN9kKabdMVSHvsk/kPJ um4wQpuAMD5FCoEOAeQ0k28VK/o1ARjfCpvZf3aZHTyLGt4/FaQkuImobFk6+3VEMm RZeD7IGdjvtOuYY5GLPLHwpQGszgztQ3C3+rRQYIFam2H/KdWxK6/zJ9M19twC+fHZ a7GIUbbtTB6QA== From: "Rafael J. Wysocki" To: Linux ACPI Cc: LKML , Hans de Goede , Armin Wolf Subject: [PATCH v1] ACPI: EC: Do not release locks during operation region accesses Date: Thu, 04 Jul 2024 18:26:54 +0200 Message-ID: <12473338.O9o76ZdvQC@rjwysocki.net> Precedence: bulk X-Mailing-List: linux-acpi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-CLIENT-IP: 195.136.19.94 X-CLIENT-HOSTNAME: 195.136.19.94 X-VADE-SPAMSTATE: clean X-VADE-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeeftddrudelgdellecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfjqffogffrnfdpggftiffpkfenuceurghilhhouhhtmecuudehtdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhephffvvefufffkggfgtgesthfuredttddtjeenucfhrhhomhepfdftrghfrggvlhculfdrucghhihsohgtkhhifdcuoehrjhifsehrjhifhihsohgtkhhirdhnvghtqeenucggtffrrghtthgvrhhnpeffffffkefgheehffelteeiveeffeevhfelteejvddvieejjeelvdeiheeuveeuffenucfkphepudelhedrudefiedrudelrdelgeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepihhnvghtpeduleehrddufeeirdduledrleegpdhhvghlohepkhhrvggrtghhvghrrdhlohgtrghlnhgvthdpmhgrihhlfhhrohhmpedftfgrfhgrvghlucflrdcuhgihshhotghkihdfuceorhhjfiesrhhjfiihshhotghkihdrnhgvtheqpdhnsggprhgtphhtthhopeegpdhrtghpthhtoheplhhinhhugidqrggtphhisehvghgvrhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtoheplhhinhhugidqkhgvrhhnvghlsehvghgvrhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtohephhguvghgohgvuggvsehrvgguhhgrthdrtghomhdprhgtphhtthhopeifpggrrhhmihhnsehgmhigrdguvg X-DCC--Metrics: v370.home.net.pl 1024; Body=4 Fuz1=4 Fuz2=4 From: Rafael J. Wysocki 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 Reviewed-by: Hans de Goede --- 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;