Re: [RFC PATCH 11/14] acpi: disable battery and adapter for x86 apple machines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] acpi: sbs: SMBus hang and probe failure in acpi_sbs_probe() on Apple x86
- [High] acpi: missing CONFIG_MACSMC_ACPI check disables battery and AC on x86 Macs
--

commit a9ef8c0ab923a26344436713c10dccfd42ea84d9
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

acpi: disable battery and adapter for x86 apple machines

This commit skips the ACPI SBS, battery, and AC adapter drivers on x86 Apple
machines when CONFIG_MACSMC_POWER is enabled to avoid registering duplicate
power supply devices.

> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index f10bbf13c242f..b12f2acc0a15d 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -634,6 +634,9 @@ static int acpi_sbs_probe(struct platform_device *pdev)
>  	int result = 0;
>  	int id;
>  
> +	if (IS_ENABLED(CONFIG_MACSMC_POWER) && x86_apple_machine)
> +		return -ENODEV;
> +
>  	device = ACPI_COMPANION(&pdev->dev);
>  	if (!device)
>  		return -ENODEV;
> @@ -655,14 +658,12 @@ static int acpi_sbs_probe(struct platform_device *pdev)
>  
>  	result = 0;
>  
> -	if (!x86_apple_machine) {
> -		result = acpi_manager_get_info(sbs);
> -		if (!result) {
> -			sbs->manager_present = 1;
> -			for (id = 0; id < MAX_SBS_BAT; ++id)
> -				if ((sbs->batteries_supported & (1 << id)))
> -					acpi_battery_add(sbs, id);
> -		}
> +	result = acpi_manager_get_info(sbs);
> +	if (!result) {
> +		sbs->manager_present = 1;
> +		for (id = 0; id < MAX_SBS_BAT; ++id)
> +			if ((sbs->batteries_supported & (1 << id)))
> +				acpi_battery_add(sbs, id);
>  	}

[Severity: High]
What happens on x86 Apple machines when CONFIG_MACSMC_POWER is disabled?

When CONFIG_MACSMC_POWER=n, acpi_sbs_probe() does not return early on
x86_apple_machine, and removing the !x86_apple_machine check causes it to
call acpi_manager_get_info(sbs) -> acpi_smbus_read(..., ACPI_SBS_MANAGER, ...).

Commit 9faf6136ff46 ("ACPI / SBS: Disable smart battery manager on Apple")
and commit 630b3aff8a51 guarded this call because accessing ACPI_SBS_MANAGER
on Apple hardware causes the SMBus and EC controller to hang for several
minutes when unplugging the AC adapter. Doesn't removing the
!x86_apple_machine check reintroduce this controller hang whenever
CONFIG_MACSMC_POWER is disabled?

In addition, by removing the !x86_apple_machine condition around
acpi_manager_get_info(sbs), the preceding result = 0 assignment becomes a
dead store, and sbs->manager_present remains 0 only when
acpi_manager_get_info(sbs) returns a non-zero error:

	result = acpi_manager_get_info(sbs);
	if (!result) {
		sbs->manager_present = 1;
		...
	}

	if (!sbs->manager_present)
		acpi_battery_add(sbs, 0);

	acpi_smbus_register_callback(sbs->hc, acpi_sbs_callback, sbs);
end:
	if (result)
		acpi_sbs_remove(pdev);
	return result;

When acpi_manager_get_info(sbs) fails with a non-zero result, the
!sbs->manager_present fallback registers battery 0 and the SMBus callback,
but because result is still non-zero at the end label, acpi_sbs_remove(pdev)
immediately tears everything back down and fails the probe. Should the
!x86_apple_machine check be kept?

> diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
> index 418951639f511..9f3b9b7d9c27c 100644
> --- a/drivers/acpi/x86/utils.c
> +++ b/drivers/acpi/x86/utils.c
> @@ -14,6 +14,7 @@
>  #include <linux/dmi.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/platform_data/x86/apple.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
>  #include "../internal.h"
> @@ -608,6 +609,9 @@ bool acpi_quirk_skip_acpi_ac_and_battery(void)
>  	long quirks = 0;
>  	int i;
>  
> +	if (IS_ENABLED(CONFIG_MACSMC_POWER) && x86_apple_machine)
> +		return true;

[Severity: High]
Should both acpi_quirk_skip_acpi_ac_and_battery() and acpi_sbs_probe() also
check IS_ENABLED(CONFIG_MACSMC_ACPI)?

In drivers/power/supply/Kconfig, MACSMC_POWER only depends on
MFD_MACSMC_CORE:

config MACSMC_POWER
	tristate "Apple SMC Battery and Power Driver"
	depends on MFD_MACSMC_CORE

However, on x86 Apple machines, the macsmc-power MFD device is only
registered when apple_smc_acpi_probe() in drivers/platform/x86/macsmc-acpi.c
probes the SMC device, which is controlled by CONFIG_MACSMC_ACPI in
drivers/platform/x86/Kconfig:

config MACSMC_ACPI
	tristate "Apple SMC (System Management Controller) ACPI Backend"
	depends on MFD_MACSMC_CORE && ACPI && !SENSORS_APPLESMC

If a kernel is built with CONFIG_MACSMC_POWER enabled and CONFIG_MACSMC_ACPI
disabled (for example, whenever CONFIG_SENSORS_APPLESMC is enabled), won't
checking only IS_ENABLED(CONFIG_MACSMC_POWER) disable the ACPI SBS, battery,
and AC adapter drivers on x86 Macs without any macsmc-acpi backend built to
instantiate macsmc-power?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=11