Re: [PATCH 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Double-checked locking on `cache->valid` in `applesmc_get_entry_by_index()` lacks memory barriers, leading to a TOCTOU data race.
--

--- Patch [1]: [PATCH 1/1] hwmon: (applesmc) Convert to hwmon_device_register_with_info ---
commit 28324860025f1b3d6c23bec77f8d59b368d416c8
Author: Shih-Yuan Lee <fourdollars@debian.org>

hwmon: (applesmc) Convert to hwmon_device_register_with_info
    
This commit converts the applesmc driver to use the modern
hwmon_device_register_with_info API to resolve a deprecation warning.
It dynamically creates sysfs attributes and implements a visibility callback
returning 0 to hide a required minimal temperature channel, ensuring backwards
compatibility.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 90a14a7f2c4c0..2b10bef24d8d6 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c

[ ... ]

> @@ -1363,7 +1383,8 @@ static int __init applesmc_init(void)
>  	if (ret)
>  		goto out_light_sysfs;
>  
> -	hwmon_dev = hwmon_device_register(&pdev->dev);
> +	hwmon_dev = hwmon_device_register_with_info(&pdev->dev, "applesmc", NULL,
> +						    &applesmc_chip_info, NULL);
>  	if (IS_ERR(hwmon_dev)) {
>  		ret = PTR_ERR(hwmon_dev);
>  		goto out_light_ledclass;

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race in
applesmc_get_entry_by_index() while reviewing this driver.

Can a reader thread observe cache->valid as true before the compiler has
fully committed the cache entry contents to memory?

drivers/hwmon/applesmc.c:applesmc_get_entry_by_index() {
	...
	if (cache->valid)
		return cache;

	mutex_lock(&smcreg.mutex);
	...
	memcpy(cache->key, key, 4);
	cache->len = info[0];
	memcpy(cache->type, &info[1], 4);
	cache->flags = info[5];
	cache->valid = true;
	...
}

Because this lockless read and write of cache->valid lacks memory barriers
like smp_load_acquire() and smp_store_release(), is it possible for a
reader querying sysfs attributes to act on uninitialized data, potentially
sending invalid keys to the Apple SMC hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710123236.10508-2-fourdollars@debian.org?part=1