| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <76c4efa6-7765-42ee-a91b-dc8d5f9e9b3e@roeck-us.net>
Date: Mon, 6 Jan 2025 10:59:33 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Adrian DC <radian.dc@...il.com>, Jean Delvare <jdelvare@...e.com>,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] hwmon/adt7470: resolve faulty 'temp*_alarm' values
read output
On 1/5/25 11:55, Adrian DC wrote:
> Tested with the following script:
The patch description is supposed to explain what is changed, and why.
Test results are appropriate after "---", but not as patch description.
> ---
>
> {
> # Access hwmon
> cd /sys/class/hwmon/hwmon1/
>
> # Check alarms
> alarms() {
> echo -n ' [READ]'
> echo -n " fan[1234]_alarm = $(cat ./fan[1234]_alarm | tr '\n' ' ')"
> echo -n " temp{[123456789],10}_alarm = $(cat ./temp{[123456789],10}_alarm | tr '\n' ' ')"
> echo ''
> }
>
> # Configure hardware
> echo '0' >./alarm_mask
> echo '10' >./num_temp_sensors
>
> # Test fans
> for fan in $(seq 1 4); do
> echo " [TEST] fan${fan}_min : Min FAN speed ${fan} under limit"
> echo '65535' >"./fan${fan}_min"
> sleep 4
> alarms
> echo '0' >"./fan${fan}_min"
> sleep 5
> done
>
> # Test temperatures
> for temp in $(seq 1 10); do
> echo " [TEST] temp${temp}_max : Max temperature ${temp} over limit"
> echo '-126000' >"./temp${temp}_max"
> sleep 5
> alarms
> echo '127000' >"./temp${temp}_max"
> sleep 5
> done
>
> # Test clean
> echo ' [TEST] Final state'
> alarms
> }
> ---
>
> Faulty values:
> [TEST] temp1_max : Max temperature 1 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 1 0 1 0 1 0 1
> [TEST] temp2_max : Max temperature 2 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> ---
>
> Fixed values:
> [TEST] fan1_min : Min FAN speed 1 under limit
> [READ] fan[1234]_alarm = 1 0 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan2_min : Min FAN speed 2 under limit
> [READ] fan[1234]_alarm = 0 1 0 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan3_min : Min FAN speed 3 under limit
> [READ] fan[1234]_alarm = 0 0 1 0 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] fan4_min : Min FAN speed 4 under limit
> [READ] fan[1234]_alarm = 0 0 0 1 temp{[123456789],10}_alarm = 0 0 0 0 0 0 0 0 0 0
> [TEST] temp1_max : Max temperature 1 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 1 0 0 0 0 0 0 0 0 0
> [TEST] temp2_max : Max temperature 2 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 1 0 0 0 0 0 0 0 0
> [TEST] temp3_max : Max temperature 3 over limit
> [READ] fan[1234]_alarm = 0 0 0 0 temp{[123456789],10}_alarm = 0 0 1 0 0 0 0 0 0 0
> ...
> ---
>
> Signed-off-by: Adrian DC <radian.dc@...il.com>
> ---
> drivers/hwmon/adt7470.c | 52 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 712bc41b4a0d..afb881385dbb 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -551,7 +551,40 @@ static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *va
> *val = 1000 * data->temp_max[channel];
> break;
> case hwmon_temp_alarm:
> - *val = !!(data->alarm & channel);
> + switch (channel) {
> + case 0:
> + *val = !!(data->alarm & ADT7470_R1T_ALARM);
> + break;
> + case 1:
> + *val = !!(data->alarm & ADT7470_R2T_ALARM);
> + break;
> + case 2:
> + *val = !!(data->alarm & ADT7470_R3T_ALARM);
> + break;
> + case 3:
> + *val = !!(data->alarm & ADT7470_R4T_ALARM);
> + break;
> + case 4:
> + *val = !!(data->alarm & ADT7470_R5T_ALARM);
> + break;
> + case 5:
> + *val = !!(data->alarm & ADT7470_R6T_ALARM);
> + break;
> + case 6:
> + *val = !!(data->alarm & ADT7470_R7T_ALARM);
> + break;
> + case 7:
> + *val = !!(data->alarm & (ADT7470_R8T_ALARM << 8));
> + break;
> + case 8:
> + *val = !!(data->alarm & (ADT7470_R9T_ALARM << 8));
> + break;
> + case 9:
> + *val = !!(data->alarm & (ADT7470_R10T_ALARM << 8));
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
This can be simplified as
if (channel < 7)
*val = !!(data->alarm & BIT(channel));
else
*val = !!(data->alarm & BIT(channel + 1);
> break;
> default:
> return -EOPNOTSUPP;
> @@ -648,7 +681,22 @@ static int adt7470_fan_read(struct device *dev, u32 attr, int channel, long *val
> *val = 0;
> break;
> case hwmon_fan_alarm:
> - *val = !!(data->alarm & (1 << (12 + channel)));
> + switch (channel) {
> + case 0:
> + *val = !!(data->alarm & (ADT7470_FAN1_ALARM << 8));
> + break;
> + case 1:
> + *val = !!(data->alarm & (ADT7470_FAN2_ALARM << 8));
> + break;
> + case 2:
> + *val = !!(data->alarm & (ADT7470_FAN3_ALARM << 8));
> + break;
> + case 3:
> + *val = !!(data->alarm & (ADT7470_FAN4_ALARM << 8));
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
I don't immediately see the problem with the current code, nor do I see a functional
difference. Please explain. Either case, the switch statement is unnecessary.
> break;
> default:
> return -EOPNOTSUPP;
Powered by blists - more mailing lists