linux-kernel - Re: [PATCH 0/4] coretemp: Fix spamming of ring buffer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aeed703c-5dbb-4a6f-a4d7-3c6a55d99e9e@roeck-us.net>
Date:   Wed, 29 Mar 2023 05:47:00 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Zhang, Rui" <rui.zhang@...el.com>
Cc:     "peter.ganzhorn@...il.com" <peter.ganzhorn@...il.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/4] coretemp: Fix spamming of ring buffer

On Wed, Mar 29, 2023 at 03:43:58AM +0000, Zhang, Rui wrote:
> Hi, Peter,
> 
> CC the list.
> 
> On Tue, 2023-03-28 at 22:37 +0200, Peter Ganzhorn wrote:
> > Dear Mr. Rui,
> > Dear Mr. Roeck,
> > 
> > please consider accepting the attached patches or
> > modifying the coretemp code to stop spamming my syslog.
> > I would appreciate it very much if you can accept the patches.
> > 
> > coretemp: Improve dynamic changes of TjMax
> > After introduction of dynamic TjMax changes in commit
> > c0c67f8761cec1fe36c21d85b1a5400ea7ac30cd
> > my syslog gets spammed with "TjMax is ... degrees C"
> > messages.
> > If TjMax is subject to change at any time, it won't be
> > set in tdata anymore and re-read every time from MSR.
> > This causes quite a lot of dev_dbg() messages to be issued.
> > 
> > The following patches change the code to read TjMax
> > from the MSRs into tdata->tjmax (again) but allow for a
> > dynamic update at any time as well. (Patches 1 and 2)
> > This way a message will only be issued after actual changes.
> > Also I replaced the dev_dbg() with dev_notice (Patch 3) and
> > added a additional dev_notice for the case where TjMax is
> > set based on assumptions. (Patch 4)
> > 
> > 
> > If you do not want to accept my patches, removing the
> > dev_dbg() in get_tjmax() would be the most simple
> > solution I guess.
> > 
> Please check if below patch solves your problem or not.
> 
> From 9370ee5163a85f65230b5222f1f4dece59ce078a Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@...el.com>
> Date: Wed, 29 Mar 2023 11:35:18 +0800
> Subject: [PATCH] hwmon: (coretemp) Avoid duplicate debug messages
> 
> Avoid duplicate dev_dbg messages when tjmax value retrieved from MSR
> does not change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@...el.com>
> ---
>  drivers/hwmon/coretemp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 30d77f451937..809456967b50 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -267,6 +267,7 @@ static int get_tjmax(struct temp_data *tdata, struct device *dev)
>  	int err;
>  	u32 eax, edx;
>  	u32 val;
> +	static u32 tjmax;

That would apply to every instance of this driver, meaning to every
CPU core. Is that really appropriate ?

Guenter

>  
>  	/* use static tjmax once it is set */
>  	if (tdata->tjmax)
> @@ -287,7 +288,10 @@ static int get_tjmax(struct temp_data *tdata, struct device *dev)
>  		 * will be used
>  		 */
>  		if (val) {
> -			dev_dbg(dev, "TjMax is %d degrees C\n", val);
> +			if (tjmax != val) {
> +				dev_dbg(dev, "TjMax is %d degrees C\n", val);
> +				tjmax = val;
> +			}
>  			return val * 1000;
>  		}
>  	}
> -- 
> 2.25.1
>