Re: [PATCH 3/5] hwmon: (nct6683) Add pwm[1-8]_enable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The sysfs callbacks `show_pwm_enable()` and `store_pwm_enable()` incorrectly cast their attribute argument to `struct sensor_device_attribute_2 *`, leading to type confusion with `struct sensor_device_attribute` and causing failures on Big-Endian architectures.
--

--- Patch [3]: [PATCH 3/5] hwmon: (nct6683) Add pwm[1-8]_enable ---
commit ae1360636538f417a87b23c1e1c204ef9329c018
Author: Johan Dahlin <jdahlin@gmail.com>

hwmon: (nct6683) Add pwm[1-8]_enable

This commit exposes the manual-control bitmap as pwm[1-8]_enable for fan
control on supported boards, falling back to read-only hardware control
on unsupported variants.

Link: https://github.com/Fred78290/nct6687d
Link: https://github.com/LibreHardwareMonitor/LibreHardwareMonitor

> diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
> index b43e915f9d0fb..0324ace707a53 100644
> --- a/drivers/hwmon/nct6683.c
> +++ b/drivers/hwmon/nct6683.c
[ ... ]
> @@ -962,25 +963,93 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
[ ... ]
> +static ssize_t
> +show_pwm_enable(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);

[Severity: Medium]
Does this cast cause type confusion with struct sensor_device_attribute?

The template initializes the attribute as a struct sensor_device_attribute,
which uses an int for the index field, but it is cast here to a struct
sensor_device_attribute_2, which uses a u8 for the index.

On Big-Endian architectures, reading a u8 from an int field aliases the MSB.
Would this result in an index of 0, effectively mapping all manual fan
control operations to channel 0 and breaking functionality for fans 1-7?

> +	struct nct6683_data *data = dev_get_drvdata(dev);
> +	u8 mode;
> +
> +	mutex_lock(&data->update_lock);
> +	mode = nct6683_read(data, NCT6683_REG_FAN_CTRL_MODE);
> +	mutex_unlock(&data->update_lock);
> +
> +	return sysfs_emit(buf, "%d\n", (mode & BIT(sattr->index)) ? 1 : 2);
> +}
> +
> +static ssize_t
> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
> +		 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);

[Severity: Medium]
Is there a similar type confusion issue on the store path?

Casting to struct sensor_device_attribute_2 and reading the u8 index might
similarly fail on Big-Endian systems during store operations, causing writes
to unintended channels.

> +	struct nct6683_data *data = dev_get_drvdata(dev);
> +	u8 bit = BIT(sattr->index);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825092546.669450-1-jdahlin@gmail.com?part=3