Re: [PATCH v1] hwmon: (asus_atk0110) Check ACPI_COMPANION() against NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent sysfs reads cause data races and potentially unsafe ACPI evaluations due to lack of locking.
- [High] The logic for restoring the EC (Embedded Controller) state on driver unload is completely inverted, erroneously disabling the EC if it was previously enabled.
--

commit c755ab3a5c2ef6bd47ed195b3ed8df92657bc3c8
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

hwmon: (asus_atk0110) Check ACPI_COMPANION() against NULL
    
Platform drivers can be forced to match a device that doesn't match its
list of device IDs using device_match_driver_override(). This commit
adds a requisite check to ensure the ACPI companion object exists before
continuing with the probe.

> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 5688ff5f7c28d..109318b0434d9 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -1273,15 +1273,20 @@ static int atk_probe(struct platform_device *pdev)
>  	struct acpi_buffer buf;
>  	union acpi_object *obj;
>  	struct atk_data *data;
> +	acpi_handle handle;
>  
>  	dev_dbg(&pdev->dev, "adding...\n");
>  
> +	handle = ACPI_HANDLE(&pdev->dev);
> +	if (!handle)
> +		return -ENODEV;
> +
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
>  	data->dev = &pdev->dev;
> -	data->atk_handle = ACPI_HANDLE(&pdev->dev);
> +	data->atk_handle = handle;
>  	INIT_LIST_HEAD(&data->sensor_list);
>  	data->disable_ec = false;

This isn't a regression, but is the logic for restoring the EC state
inverted?

In atk_enumerate_new_hwmon(), atk_ec_enabled() returns 1 if the EC is
enabled and 0 if disabled, and this result is assigned directly to
data->disable_ec:

drivers/hwmon/asus_atk0110.c:atk_enumerate_new_hwmon() {
        ...
		err = atk_ec_enabled(data);
		if (err < 0)
			return err;
		/* If the EC was disabled we will disable it again on unload */
		data->disable_ec = err;
        ...
}

Later, during atk_remove() or error paths, the driver checks this flag
and disables the EC:

drivers/hwmon/asus_atk0110.c:atk_probe() {
        ...
out:
	if (data->disable_ec)
		atk_ec_ctl(data, 0);
	return err;
}

Will this incorrectly disable the EC on exit if it was running before
probe, and leave it enabled if it was initially stopped?

Also not a regression, but does this driver need to implement its own
locking to serialize sysfs reads?

The hardware monitoring subsystem requires drivers registering attributes
via hwmon_device_register_with_groups() to implement their own locking.
In atk_read_value(), fields like sensor->is_valid, sensor->last_updated,
and sensor->cached_value are accessed and modified concurrently:

drivers/hwmon/asus_atk0110.c:atk_read_value() {
	if (!sensor->is_valid ||
	    time_after(jiffies, sensor->last_updated + CACHE_TIME)) {
		...
		sensor->is_valid = true;
		sensor->last_updated = jiffies;
		sensor->cached_value = *value;
	} else {
		*value = sensor->cached_value;
	}
    ...
}

Could concurrent reads bypass the cache, trigger concurrent ACPI method
evaluations, and cause torn reads or writes on 64-bit cached values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2261594.irdbgypaU6@rafael.j.wysocki?part=1