linux-kernel - Re: [PATCH] hwmon: (adm9240): Don't re-read config/limits

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7b90ff0c-6772-6ecf-d15d-ff5ff09c5628@roeck-us.net>
Date:   Tue, 9 Mar 2021 20:59:08 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Chris Packham <chris.packham@...iedtelesis.co.nz>,
        jdelvare@...e.com
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (adm9240): Don't re-read config/limits

On 3/9/21 7:36 PM, Chris Packham wrote:
> The hwmon chip is configured either via sysfs or by an earlier boot
> stage (e.g. bootloader/bios). In the sysfs case we already store the
> configuration values before it's written to the device. Reading in the
> configuration only needs to be done once at probe time to cover the
> second case.
> 
> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> ---
> 
> This doesn't resolve my ongoing i2c issues[0] but avoiding unnecessary
> i2c reads will help a bit (it'll certainly avoid errors where the
> threshold spontaneously changes).
> 
> [0] - https://lore.kernel.org/lkml/8e0a88ba-01e9-9bc1-c78b-20f26ce27d12@alliedtelesis.co.nz/
> 
>  drivers/hwmon/adm9240.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index cc3e0184e720..7e1258b20b35 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -128,7 +128,6 @@ struct adm9240_data {
>  	struct mutex update_lock;
>  	char valid;
>  	unsigned long last_updated_measure;
> -	unsigned long last_updated_config;
>  
>  	u8 in[6];		/* ro	in0_input */
>  	u8 in_max[6];		/* rw	in0_max */
> @@ -282,21 +281,11 @@ static struct adm9240_data *adm9240_update_device(struct device *dev)
>  			return ERR_PTR(err);
>  		}
>  		data->last_updated_measure = jiffies;
> -	}
> -
> -	/* minimum config reading cycle: 300 seconds */
> -	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
> -			|| !data->valid) {

This is a bit more invasive than desirable. Problem is that
it takes away the "!data->valid" part of the code as well,
and there may be valid reasons to re-read the limits after
an error.

I'd suggest to just drop the time_after and last_updated_config
and just check for data->valid. This also makes the change
in the probe function unnecessary.

I you _really_ want to make a difference, you could drop
caching from the driver entirely and let regmap handle it
(ie tell regmap which registers are hot). Caching isn't
really that valuable, and is often more expensive than
worth it.

Even better, of course, would be to convert the driver to
use devm_hwmon_device_register_with_info and drop all the
sysfs code from it. Or, rather, almost all of it, since we'd
have that non-standard aout_output attribute. Unfortunately,
neither of those changes would solve your problem.

> -		err = adm9240_update_config(data);
> -		if (err < 0) {
> -			data->valid = 0;
> -			mutex_unlock(&data->update_lock);
> -			return ERR_PTR(err);
> -		}
> -		data->last_updated_config = jiffies;
>  		data->valid = 1;
>  	}
> +
>  	mutex_unlock(&data->update_lock);
> +

Please refrain from making whitespace changes. You may prefer
the code that way, but next week I'll get someone else
submitting a different patch and removing the added empty lines.

>  	return data;
>  }
>  
> @@ -855,7 +844,15 @@ static int adm9240_probe(struct i2c_client *new_client)
>  							   new_client->name,
>  							   data,
>  							   adm9240_groups);
> -	return PTR_ERR_OR_ZERO(hwmon_dev);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	/* pull in configuration from an earlier boot stage */
> +	err = adm9240_update_config(data);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;

If at all, this call should be made before registering the hwmon device.

>  }
>  
>  static const struct i2c_device_id adm9240_id[] = {
>