[PATCH v3 0/3] ACPI: SBS: Fix various issues

[PATCH v3 0/3] ACPI: SBS: Fix various issues

[Armin Wolf]

On my Acer Travelmate 4002WLMi, the system locks up upon
suspend/shutdown. After a lot of research, it turned out
that the sbs module was the culprit. The driver would not
correctly mask out the value used to select a battery using
the "Smart Battery Selector" (subset of the "Smart Battery Manager").
This accidentally caused a invalid power source to be selected,
which was automatically corrected by the selector. Upon
notifing the host about the corrected power source, some batteries
would be selected for re-reading, causing a endless loop.
This would lead to some workqueues filling up, which caused the
lockup upon suspend/shutdown.

The first patch fixes an issue inside the ec driver regarding the
removal of query handlers discovered thru ACPI. The second patch fixes
a kernel oops on module removal caused by a race condition when removing
custom EC query handlers. The last patch finally fixes the
suspend/shutdown issues.

As a side note: This was the first machine on which i installed Linux,
to finally fixing this took ~5 years of tinkering.

Tested on a Acer Travelmate 4002WLMi.
---
Changes in v3:
- Rework solution for the kernel oops on module removal
Changes in v2:
- make acpi_ec_add_query_handler() static to fix warning

Armin Wolf (3):
  ACPI: EC: Limit explicit removal of query handlers to custom query
    handlers
  ACPI: EC: Fix oops when removing custom query handlers
  ACPI: SBS: Fix handling of Smart Battery Selectors

 drivers/acpi/ec.c  | 17 ++++++++++++++---
 drivers/acpi/sbs.c | 27 ++++++++++++++++++---------
 2 files changed, 32 insertions(+), 12 deletions(-)

--
2.30.2

[PATCH v3 1/3] ACPI: EC: Limit explicit removal of query handlers to custom query handlers

[Armin Wolf]

According to the ACPI spec part 5.6.4.1.2, EC query handlers discovered
thru ACPI should not be removed when a driver removes his custom query
handler. On the Acer Travelmate 4002WLMi for example, such a query
handler is used as a fallback to handle the EC SMBus alert when no driver
is present.
Change acpi_ec_remove_query_handlers() so that only custom query
handlers are removed then remove_all is false. Query handlers discovered
thru ACPI will still get removed when remove_all is true, which happens
on device removal. Also add a simple check to ensure that
acpi_ec_add_query_handler() is always called with either handle or func
being set, since custom query handlers are detected based whether
handlers->func is set or not.

Tested on a Acer Travelmate 4002WLMi.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/ec.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 105d2e795afa..f84905dbd8ca 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1083,9 +1083,12 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data)
 {
-	struct acpi_ec_query_handler *handler =
-	    kzalloc(sizeof(struct acpi_ec_query_handler), GFP_KERNEL);
+	struct acpi_ec_query_handler *handler;
+
+	if (!handle && !func)
+		return -EINVAL;

+	handler = kzalloc(sizeof(*handler), GFP_KERNEL);
 	if (!handler)
 		return -ENOMEM;

@@ -1097,6 +1100,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 	kref_init(&handler->kref);
 	list_add(&handler->node, &ec->list);
 	mutex_unlock(&ec->mutex);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
@@ -1109,9 +1113,15 @@ static void acpi_ec_remove_query_handlers(struct acpi_ec *ec,

 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		if (remove_all || query_bit == handler->query_bit) {
+		/* When remove_all is false, we only remove custom query handlers
+		 * which have handler->func set. This is done to preserve query
+		 * handlers discovered thru ACPI, as they should continue handling
+		 * EC queries.
+		 */
+		if (remove_all || (handler->func && handler->query_bit == query_bit)) {
 			list_del_init(&handler->node);
 			list_add(&handler->node, &free_list);
+
 		}
 	}
 	mutex_unlock(&ec->mutex);
--
2.30.2

[PATCH v3 2/3] ACPI: EC: Fix oops when removing custom query handlers

[Armin Wolf]

When removing custom query handlers, the handler might still
be used inside the EC query workqueue, causing a kernel oops
if the module holding the callback function was already unloaded.

Fix this by flushing the EC query workqueue when removing
custom query handlers.

Tested on a Acer Travelmate 4002WLMi

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/ec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f84905dbd8ca..4ae017391533 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1132,6 +1132,7 @@ static void acpi_ec_remove_query_handlers(struct acpi_ec *ec,
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
 	acpi_ec_remove_query_handlers(ec, false, query_bit);
+	flush_workqueue(ec_query_wq);
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);

--
2.30.2

[PATCH v3 3/3] ACPI: SBS: Fix handling of Smart Battery Selectors

[Armin Wolf]

The "Smart Battery Selector" standard says that when writing
SelectorState (0x1), the nibbles which should not be modified
need to be masked with 0xff. This is necessary since in contrast
to a "Smart Battery Manager", the last three nibbles are writable.

Failing to do so might trigger the following cycle:
1. Host accidentally changes power source of the system (3rd nibble)
   when selecting a battery.
2. Power source is invalid, Selector changes to another power source.
3. Selector notifies host that it changed the power source.
4. Host re-reads some batteries.
5. goto 1 for each re-read battery.

This loop might also be entered when a battery which is not present
is selected for SMBus access. In the end some workqueues fill up,
which causes the system to lockup upon suspend/shutdown.

Fix this by correctly masking the value to be written, and avoid
selecting batteries which are absent.

Tested on a Acer Travelmate 4002WLMi.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/sbs.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index e90752d4f488..94e3c000df2e 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -473,23 +473,32 @@ static const struct device_attribute alarm_attr = {
    -------------------------------------------------------------------------- */
 static int acpi_battery_read(struct acpi_battery *battery)
 {
-	int result = 0, saved_present = battery->present;
+	int result, saved_present = battery->present;
 	u16 state;

 	if (battery->sbs->manager_present) {
 		result = acpi_smbus_read(battery->sbs->hc, SMBUS_READ_WORD,
 				ACPI_SBS_MANAGER, 0x01, (u8 *)&state);
-		if (!result)
-			battery->present = state & (1 << battery->id);
-		state &= 0x0fff;
+		if (result)
+			return result;
+
+		battery->present = state & (1 << battery->id);
+		if (!battery->present)
+			return 0;
+
+		/* Masking necessary for Smart Battery Selectors */
+		state = 0x0fff;
 		state |= 1 << (battery->id + 12);
 		acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD,
 				  ACPI_SBS_MANAGER, 0x01, (u8 *)&state, 2);
-	} else if (battery->id == 0)
-		battery->present = 1;
-
-	if (result || !battery->present)
-		return result;
+	} else {
+		if (battery->id == 0) {
+			battery->present = 1;
+		} else {
+			if (!battery->present)
+				return 0;
+		}
+	}

 	if (saved_present != battery->present) {
 		battery->update_time = 0;
--
2.30.2

Re: [PATCH v3 0/3] ACPI: SBS: Fix various issues

[Rafael J. Wysocki]

On Fri, Mar 24, 2023 at 9:26 PM Armin Wolf <W_Armin@gmx.de> wrote:
>
> On my Acer Travelmate 4002WLMi, the system locks up upon
> suspend/shutdown. After a lot of research, it turned out
> that the sbs module was the culprit. The driver would not
> correctly mask out the value used to select a battery using
> the "Smart Battery Selector" (subset of the "Smart Battery Manager").
> This accidentally caused a invalid power source to be selected,
> which was automatically corrected by the selector. Upon
> notifing the host about the corrected power source, some batteries
> would be selected for re-reading, causing a endless loop.
> This would lead to some workqueues filling up, which caused the
> lockup upon suspend/shutdown.
>
> The first patch fixes an issue inside the ec driver regarding the
> removal of query handlers discovered thru ACPI. The second patch fixes
> a kernel oops on module removal caused by a race condition when removing
> custom EC query handlers. The last patch finally fixes the
> suspend/shutdown issues.
>
> As a side note: This was the first machine on which i installed Linux,
> to finally fixing this took ~5 years of tinkering.
>
> Tested on a Acer Travelmate 4002WLMi.
> ---
> Changes in v3:
> - Rework solution for the kernel oops on module removal
> Changes in v2:
> - make acpi_ec_add_query_handler() static to fix warning
>
> Armin Wolf (3):
>   ACPI: EC: Limit explicit removal of query handlers to custom query
>     handlers
>   ACPI: EC: Fix oops when removing custom query handlers
>   ACPI: SBS: Fix handling of Smart Battery Selectors
>
>  drivers/acpi/ec.c  | 17 ++++++++++++++---
>  drivers/acpi/sbs.c | 27 ++++++++++++++++++---------
>  2 files changed, 32 insertions(+), 12 deletions(-)
>
> --

All applied as 6.4 material with a minor comment adjustment in the first patch.

Thanks!

Re: [PATCH v3 0/3] ACPI: SBS: Fix various issues

[Armin Wolf]

Am 30.03.23 um 19:02 schrieb Rafael J. Wysocki:

> On Fri, Mar 24, 2023 at 9:26 PM Armin Wolf <W_Armin@gmx.de> wrote:
>> On my Acer Travelmate 4002WLMi, the system locks up upon
>> suspend/shutdown. After a lot of research, it turned out
>> that the sbs module was the culprit. The driver would not
>> correctly mask out the value used to select a battery using
>> the "Smart Battery Selector" (subset of the "Smart Battery Manager").
>> This accidentally caused a invalid power source to be selected,
>> which was automatically corrected by the selector. Upon
>> notifing the host about the corrected power source, some batteries
>> would be selected for re-reading, causing a endless loop.
>> This would lead to some workqueues filling up, which caused the
>> lockup upon suspend/shutdown.
>>
>> The first patch fixes an issue inside the ec driver regarding the
>> removal of query handlers discovered thru ACPI. The second patch fixes
>> a kernel oops on module removal caused by a race condition when removing
>> custom EC query handlers. The last patch finally fixes the
>> suspend/shutdown issues.
>>
>> As a side note: This was the first machine on which i installed Linux,
>> to finally fixing this took ~5 years of tinkering.
>>
>> Tested on a Acer Travelmate 4002WLMi.
>> ---
>> Changes in v3:
>> - Rework solution for the kernel oops on module removal
>> Changes in v2:
>> - make acpi_ec_add_query_handler() static to fix warning
>>
>> Armin Wolf (3):
>>    ACPI: EC: Limit explicit removal of query handlers to custom query
>>      handlers
>>    ACPI: EC: Fix oops when removing custom query handlers
>>    ACPI: SBS: Fix handling of Smart Battery Selectors
>>
>>   drivers/acpi/ec.c  | 17 ++++++++++++++---
>>   drivers/acpi/sbs.c | 27 ++++++++++++++++++---------
>>   2 files changed, 32 insertions(+), 12 deletions(-)
>>
>> --
> All applied as 6.4 material with a minor comment adjustment in the first patch.
>
> Thanks!

Thank you!