Re: [RFC PATCH] hwmon: Add fan monitoring support for HONOR FMI-XX

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The function `honor_fmi_read_rpm` illegally mixes `goto`-based error handling with a scoped `guard(mutex)` lock.
- [Low] The new entry in the MAINTAINERS file is inserted out of alphabetical order.
--

--- Patch [1]: [RFC PATCH] hwmon: Add fan monitoring support for HONOR FMI-XX ---
commit 8d90c25d369a88a7d4700de11f8a23c2eaf15bbc
Author: Nikita Dubrovskih <[email protected]>

hwmon: Add fan monitoring support for HONOR FMI-XX

This patch introduces a read-only hwmon driver for HONOR FMI-XX laptops.
It queries fan speeds using the firmware-provided \GFNS ACPI method and 
exposes the results via sysfs.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8014b9f8253ed..d7355db80288e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11854,6 +11854,13 @@ F:	lib/test_hmm*
>  F:	mm/hmm*
>  F:	tools/testing/selftests/mm/*hmm*
>  
> +HONOR FMI-XX HARDWARE MONITOR DRIVER
> +M:	Nikita Dubrovskih <[email protected]>
> +L:	[email protected]
> +S:	Maintained
> +F:	Documentation/hwmon/honor-fmi.rst
> +F:	drivers/hwmon/honor-fmi.c
> +
>  HONEYWELL ABP2030PA PRESSURE SENSOR SERIES IIO DRIVER

[Severity: Low]
This isn't a bug, but should this new entry be placed after HONEYWELL?

Alphabetically, "HONOR" comes after "HONEYWELL" (O follows E), so inserting
it here breaks the alphabetical order of the MAINTAINERS file.

[ ... ]

> diff --git a/drivers/hwmon/honor-fmi.c b/drivers/hwmon/honor-fmi.c
> new file mode 100644
> index 0000000000000..225066e9add5f
> --- /dev/null
> +++ b/drivers/hwmon/honor-fmi.c
[ ... ]
> +static int honor_fmi_read_rpm(struct honor_fmi_data *data, int channel,
> +			      long *rpm)
> +{
> +	union acpi_object input = {
> +		.buffer = {
> +			.type = ACPI_TYPE_BUFFER,
> +			.length = 3,
> +		},
> +	};
> +	struct acpi_object_list arguments = {
> +		.count = 1,
> +		.pointer = &input,
> +	};
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *result;
> +	u8 input_bytes[3] = { 0, 0, channel };
> +	acpi_status status;
> +	int ret = 0;
> +
> +	input.buffer.pointer = input_bytes;
> +
> +	guard(mutex)(&data->lock);
> +	status = acpi_evaluate_object(data->gfns, NULL, &arguments, &output);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	result = output.pointer;
> +	if (!result || result->type != ACPI_TYPE_BUFFER ||
> +	    result->buffer.length < HONOR_FMI_GFNS_RESULT_SIZE) {
> +		ret = -EPROTO;
> +		goto out_free;
> +	}
> +
> +	if (result->buffer.pointer[0]) {
> +		ret = -EIO;
> +		goto out_free;
> +	}
> +
> +	*rpm = result->buffer.pointer[1] |
> +	       (result->buffer.pointer[2] << 8);
> +
> +out_free:
> +	kfree(output.pointer);
> +	return ret;
> +}

[Severity: Low]
Does this function mix goto-based error handling with scope-based cleanups?

The cleanup subsystem guidelines request that guard() helpers and goto-based
cleanups are not mixed in the same function. 

Could the output buffer be managed using __free(kfree) to completely
eliminate the out_free label?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1