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

[Ali Ahmet Memis]

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