linux-kernel - Re: [PATCH] hwmon: (peci/dimmtemp) Do not provide fake thresholds data

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: <71b63aa1646af4ae30b59f6d70f3daaeb983b6f8.camel@intel.com>
Date: Mon, 27 Jan 2025 16:40:52 +0000
From: "Winiarska, Iwona" <iwona.winiarska@...el.com>
To: "linux@...ck-us.net" <linux@...ck-us.net>, "jae.hyun.yoo@...ux.intel.com"
	<jae.hyun.yoo@...ux.intel.com>, "Rudolph, Patrick"
	<patrick.rudolph@...ements.com>, "pierre-louis.bossart@...ux.dev"
	<pierre-louis.bossart@...ux.dev>, "Solanki, Naresh"
	<naresh.solanki@...ements.com>, "fercerpav@...il.com" <fercerpav@...il.com>,
	"jdelvare@...e.com" <jdelvare@...e.com>
CC: "fr0st61te@...il.com" <fr0st61te@...il.com>, "linux-hwmon@...r.kernel.org"
	<linux-hwmon@...r.kernel.org>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "joel@....id.au" <joel@....id.au>
Subject: Re: [PATCH] hwmon: (peci/dimmtemp) Do not provide fake thresholds
 data

On Thu, 2025-01-23 at 15:20 +0300, Paul Fertser wrote:
> When an Icelake or Sapphire Rapids CPU isn't providing the maximum and
> critical thresholds for particular DIMM the driver should return an
> error to the userspace instead of giving it stale (best case) or wrong
> (the structure contains all zeros after kzalloc() call) data.
> 
> The issue can be reproduced by binding the peci driver while the host is
> fully booted and idle, this makes PECI interaction unreliable enough.
> 
> Fixes: 73bc1b885dae ("hwmon: peci: Add dimmtemp driver")
> Fixes: 621995b6d795 ("hwmon: (peci/dimmtemp) Add Sapphire Rapids support")
> Cc: stable@...r.kernel.org
> Signed-off-by: Paul Fertser <fercerpav@...il.com>

Hi!

Thank you for the patch.
Did you have a chance to test it with OpenBMC dbus-sensors?
In general, the change looks okay to me, but since it modifies the behavior
(applications will need to handle this, and returning an error will happen more
often) we need to confirm that it does not cause any regressions for userspace.

Once we are able to confirm that:

Reviewed-by: Iwona Winiarska <iwona.winiarska@...el.com>

Thanks
-Iwona

> ---
>  drivers/hwmon/peci/dimmtemp.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/peci/dimmtemp.c b/drivers/hwmon/peci/dimmtemp.c
> index d6762259dd69..fbe82d9852e0 100644
> --- a/drivers/hwmon/peci/dimmtemp.c
> +++ b/drivers/hwmon/peci/dimmtemp.c
> @@ -127,8 +127,6 @@ static int update_thresholds(struct peci_dimmtemp *priv,
> int dimm_no)
>  		return 0;
>  
>  	ret = priv->gen_info->read_thresholds(priv, dimm_order, chan_rank,
> &data);
> -	if (ret == -ENODATA) /* Use default or previous value */
> -		return 0;
>  	if (ret)
>  		return ret;
>  
> @@ -509,11 +507,11 @@ read_thresholds_icx(struct peci_dimmtemp *priv, int
> dimm_order, int chan_rank, u
>  
>  	ret = peci_ep_pci_local_read(priv->peci_dev, 0, 13, 0, 2, 0xd4,
> &reg_val);
>  	if (ret || !(reg_val & BIT(31)))
> -		return -ENODATA; /* Use default or previous value */
> +		return -ENODATA;
>  
>  	ret = peci_ep_pci_local_read(priv->peci_dev, 0, 13, 0, 2, 0xd0,
> &reg_val);
>  	if (ret)
> -		return -ENODATA; /* Use default or previous value */
> +		return -ENODATA;
>  
>  	/*
>  	 * Device 26, Offset 224e0: IMC 0 channel 0 -> rank 0
> @@ -546,11 +544,11 @@ read_thresholds_spr(struct peci_dimmtemp *priv, int
> dimm_order, int chan_rank, u
>  
>  	ret = peci_ep_pci_local_read(priv->peci_dev, 0, 30, 0, 2, 0xd4,
> &reg_val);
>  	if (ret || !(reg_val & BIT(31)))
> -		return -ENODATA; /* Use default or previous value */
> +		return -ENODATA;
>  
>  	ret = peci_ep_pci_local_read(priv->peci_dev, 0, 30, 0, 2, 0xd0,
> &reg_val);
>  	if (ret)
> -		return -ENODATA; /* Use default or previous value */
> +		return -ENODATA;
>  
>  	/*
>  	 * Device 26, Offset 219a8: IMC 0 channel 0 -> rank 0