RE: nct6694-hwmon: temp17_enable and above write into the fan enable bitmap

Hi Ali,

Thanks for the careful analysis, and good catch.

It's actually a fourth case. On the device side, tin_en[2] only covers the =
first 10 temperature channels (5 THR + 5 TDP, index 0-9), which are indexed=
 correctly today. The 16 DTIN channels (index 10-25) are enabled by the fir=
mware, so the driver should only expose their temperature input and the max=
/hyst limits - never their enable. Indexing tin_en past channel 15 spilling=
 into fin_en is exactly that missing distinction.

So the fix is to not advertise HWMON_T_ENABLE on the DTIN channels. I'd spl=
it the temp config:

#define NCT6694_HWMON_TEMP_CONFIG (HWMON_T_INPUT | HWMON_T_ENABLE |     \
                                                HWMON_T_MAX | HWMON_T_MAX_H=
YST |        \
                                                HWMON_T_MAX_ALARM)
#define NCT6694_HWMON_DTIN_CONFIG (HWMON_T_INPUT |                      \
                                               HWMON_T_MAX | HWMON_T_MAX_HY=
ST | \
                                               HWMON_T_MAX_ALARM)

and use NCT6694_HWMON_DTIN_CONFIG for the 16 DTIN entries. The core then ne=
ver creates tempX_enable for them, and fin_en stops getting clobbered. The =
first 10 channels are unaffected.

Feel free to write the patch, and add a Fixes: tag. Thanks again for the re=
port.

Best regards,
Ming

-----Original Message-----
From: Ali Ahmet Memis <[email protected]>
Sent: Sunday, August 2, 2026 8:48 PM
To: CS10 TMYu0 <[email protected]>
Cc: Ming Yu <[email protected]>; Guenter Roeck <[email protected]>; Je=
an Delvare <[email protected]>; [email protected]; linux-kernel@v=
ger.kernel.org
Subject: nct6694-hwmon: temp17_enable and above write into the fan enable b=
itmap

CAUTION - External Email: Do not click links or open attachments unless you=
 acknowledge the sender and content.


Hi,

While reading nct6694-hwmon.c I ran into something that looks wrong, but wh=
ich way to fix it depends on the device protocol, so I would rather ask tha=
n guess.

The driver registers 26 temperature channels, each with HWMON_T_ENABLE:

        HWMON_CHANNEL_INFO(temp,
                           NCT6694_HWMON_TEMP_CONFIG,   /* THR1 */
                           ...
                           NCT6694_HWMON_TEMP_CONFIG),  /* DTIN15 */

That is 5 THR plus 5 TDP plus 16 DTIN. The enable bitmap it indexes is two =
bytes:

        struct __packed nct6694_hwmon_control {
                u8 vin_en[2];
                u8 tin_en[2];
                u8 fin_en[2];
                u8 pwm_en[2];
                u8 reserved1[40];
                u8 pwm_freq[10];
                u8 reserved2[6];
        };

and both the read and the write path index it with the raw channel:

        temp_en =3D data->hwmon_en.tin_en[channel / 8];
        ...
        data->hwmon_en.tin_en[channel / 8] |=3D BIT(channel % 8);

nct6694_is_visible() returns 0644 for hwmon_temp_enable without looking at =
the channel number, so all 26 are writable. Since the structure is packed, =
channel 16 and above land past tin_en[]:

        channel  0-15 -> tin_en[0-1]
        channel 16-23 -> tin_en[2] =3D=3D fin_en[0]
        channel 24-25 -> tin_en[3] =3D=3D fin_en[1]

nct6694_hwmon_init() sends the whole control structure back to the device, =
so writing temp17_enable through temp26_enable ends up toggling the fan ena=
ble bits, and reading them reports fan state as temperature state. It stays=
 inside the structure, so this is not a memory safety problem, but on a boa=
rd using the fan channels it is not harmless.

What I cannot tell from here is which side is wrong, and the answers need d=
ifferent patches:

 1. If the control block really has two bytes of tin_en, then only 16
    temperature channels can be enabled and the last ten should not
    advertise HWMON_T_ENABLE.

 2. If the device has four bytes of tin_en, the structure is wrong, and
    fin_en and pwm_en are being read and written at the wrong offsets as
    well.

 3. If the bitmap only covers the digital inputs, the indexing is wrong
    in a third way: there are exactly 16 DTIN channels, which is exactly
    what tin_en[2] holds, and they sit at channel 10 and above.

The structure is 64 bytes as declared, and it would still be 64 bytes with =
tin_en[4] and reserved1[38], so the total size does not settle it.
Two things make me unsure rather than confident: the other three bitmaps ma=
tch their channel counts exactly, 16 in channels against vin_en[2] and
10 fan channels against fin_en[2], which argues for the first case; but the=
 16 DTIN channels also match tin_en[2] exactly, which is what made me think=
 of the third. I could not find a public datasheet to settle it.

Ming, could you say which one it is? I am happy to write the patch once I k=
now, I just do not want to guess at the device side.

I have no NCT6694 hardware, so this is from reading the driver against curr=
ent mainline rather than from an observed misbehaviour.

Thanks,
Ali
________________________________
________________________________
 The privileged confidential information contained in this email is intende=
d for use only by the addressees as indicated by the original sender of thi=
s email. If you are not the addressee indicated in this email or are not re=
sponsible for delivery of the email to such a person, please kindly reply t=
o the sender indicating this fact and delete all copies of it from your com=
puter and network server immediately. Your cooperation is highly appreciate=
d. It is advised that any unauthorized use of confidential information of N=
uvoton is strictly prohibited; and any information in this email irrelevant=
 to the official business of Nuvoton shall be deemed as neither given nor e=
ndorsed by Nuvoton.