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

[TMYU0@xxxxxxxxxxx]

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 firmware, 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 split the temp config:

#define NCT6694_HWMON_TEMP_CONFIG (HWMON_T_INPUT | HWMON_T_ENABLE |     \
                                                HWMON_T_MAX | HWMON_T_MAX_HYST |        \
                                                HWMON_T_MAX_ALARM)
#define NCT6694_HWMON_DTIN_CONFIG (HWMON_T_INPUT |                      \
                                               HWMON_T_MAX | HWMON_T_MAX_HYST | \
                                               HWMON_T_MAX_ALARM)

and use NCT6694_HWMON_DTIN_CONFIG for the 16 DTIN entries. The core then never 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 report.

Best regards,
Ming

-----Original Message-----
From: Ali Ahmet Memis <ali@xxxxxxxxxxxxxx>
Sent: Sunday, August 2, 2026 8:48 PM
To: CS10 TMYu0 <TMYU0@xxxxxxxxxxx>
Cc: Ming Yu <a0282524688@xxxxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>; Jean Delvare <jdelvare@xxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: nct6694-hwmon: temp17_enable and above write into the fan enable bitmap

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 which way to fix it depends on the device protocol, so I would rather ask than 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 = data->hwmon_en.tin_en[channel / 8];
        ...
        data->hwmon_en.tin_en[channel / 8] |= 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] == fin_en[0]
        channel 24-25 -> tin_en[3] == 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 enable 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 board using the fan channels it is not harmless.

What I cannot tell from here is which side is wrong, and the answers need different 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 match 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 know, I just do not want to guess at the device side.

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

Thanks,
Ali
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.