linux-kernel - Re: [PATCH v3 1/2] power: supply: Add macsmc-power driver for Apple Silicon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260118124941.GB55832@robin.jannau.net>
Date: Sun, 18 Jan 2026 13:49:41 +0100
From: Janne Grunau <j@...nau.net>
To: Nick Chan <towinchenmi@...il.com>
Cc: michael.reeves077@...il.com, Sebastian Reichel <sre@...nel.org>,
	Sven Peter <sven@...nel.org>, Neal Gompa <neal@...pa.dev>,
	Lee Jones <lee@...nel.org>, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, asahi@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	Hector Martin <marcan@...can.st>
Subject: Re: [PATCH v3 1/2] power: supply: Add macsmc-power driver for Apple
 Silicon

On Sun, Jan 18, 2026 at 08:19:38PM +0800, Nick Chan wrote:
> 
> 
> On 17/1/2026 20:26, Janne Grunau wrote:
> > On Thu, Jan 15, 2026 at 06:08:15PM +1100, Michael Reeves via B4 Relay wrote:
> >> From: Michael Reeves <michael.reeves077@...il.com>
> > 
> > I think the driver is overall similar to the downstream AsahiLinux
> > driver so please keep Hector as author.
> > 
> >> This driver provides battery and AC status monitoring for Apple Silicon
> >> Macs via the SMC (System Management Controller). It supports
> >> reporting capacity, voltage, current, and charging status.
> >>
> >> Co-developed-by: Hector Martin <marcan@...can.st>
> >> Signed-off-by: Hector Martin <marcan@...can.st>
> > 
> > The downstream driver a quite a few more Co-developed-by:/Sobs. When I
> > squashed the commits I decided to err on the safe side and included
> > commit authors from incremental patches as Co-developed-by: Why did you
> > drop those?
> > 
> >> Reviewed-by: Neal Gompa <neal@...pa.dev>
> >> Signed-off-by: Michael Reeves <michael.reeves077@...il.com>
> >> ---
> >>  MAINTAINERS                         |   1 +
> >>  drivers/power/supply/Kconfig        |  11 +
> >>  drivers/power/supply/Makefile       |   1 +
> >>  drivers/power/supply/macsmc-power.c | 834 ++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 847 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/macsmc-power.c b/drivers/power/supply/macsmc-power.c
> >> new file mode 100644
> >> index 000000000000..9b3faefe7a45
> >> --- /dev/null
> >> +++ b/drivers/power/supply/macsmc-power.c
> >> @@ -0,0 +1,834 @@

...

> >> +static int macsmc_power_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
> >> +	struct power_supply_config psy_cfg = {};
> >> +	struct macsmc_power *power;
> >> +	bool has_battery = false;
> >> +	bool has_ac_adapter = false;
> >> +	int ret = -ENODEV;
> >> +	bool flag;
> >> +	u16 vu16;
> >> +	u32 val32;
> >> +	enum power_supply_property *props;
> >> +	size_t nprops;
> >> +
> >> +	if (!smc)
> >> +		return -ENODEV;
> >> +
> >> +	power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
> >> +	if (!power)
> >> +		return -ENOMEM;
> >> +
> >> +	power->dev = dev;
> >> +	power->smc = smc;
> >> +	dev_set_drvdata(dev, power);
> >> +
> >> +	INIT_WORK(&power->critical_work, macsmc_power_critical_work);
> >> +	ret = devm_work_autocancel(dev, &power->critical_work, macsmc_power_critical_work);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/*
> >> +	 * Check for battery presence.
> >> +	 * B0AV is a fundamental key.
> >> +	 */
> >> +	if (apple_smc_read_u16(power->smc, SMC_KEY(B0AV), &vu16) == 0 &&
> >> +	    macsmc_battery_get_status(power) > POWER_SUPPLY_STATUS_UNKNOWN)
> >> +		has_battery = true;
> >> +
> >> +	/*
> >> +	 * Check for AC adapter presence.
> >> +	 * CHIS is a fundamental key.
> >> +	 */
> >> +	if (apple_smc_key_exists(smc, SMC_KEY(CHIS)))
> >> +		has_ac_adapter = true;
> >> +
> > 
> > I think a short circuit check for !(has_battery || has_ac_adapter) would
> > make sense here. The setup code for props is quite long. It should
> > return -ENODEV. ret is not -ENODEV. anymore since it was overwritten
> > with the return value of devm_work_autocancel()
> > 
> >> +	if (has_battery) {
> >> +		power->batt_desc = macsmc_battery_desc_template;
> >> +		props = devm_kcalloc(dev, MACSMC_MAX_BATT_PROPS,
> >> +				     sizeof(enum power_supply_property),
> >> +				     GFP_KERNEL);
> > 
> > I don't like the dynamic allocation for the props. I think we can
> > currently get way with static property arrays and so we should do that.
> > See my comments below
> 
> Dynamic allocation is needed to properly add support pre-M1 support,
> as the SMCs in them misses a few keys, yet in iOS 16 the charge limit
> (CHWA) keys are added. In a similar way, there really is no sane way
> to add any new properties for M1/M2/M3 with static allocation. So I
> would prefer if dynamic allocation is kept.

ack, I wasn't really thinking beyond the code in front of me. Let's go
with dynamic allocations. A check that nprops doesn't exceed
MACSMC_MAX_BATT_PROPS should be added then.

Janne