Hi Iwona,
Thank you for the review. Please see inline.
On Mon, Jan 27, 2025 at 04:40:52PM +0000, Winiarska, Iwona wrote:
> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Paul Fertser <fercerpav@xxxxxxxxx>
>
> Did you have a chance to test it with OpenBMC dbus-sensors?
Using OpenBMC dbus-sensors is exactly the reason why I'm sending this
patch, so yes, I tested it before and after the change.
> 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.
The change is prompted by the current behaviour which is unacceptably
bad: every now and then while powering on the host for the first time
BMC happens to request one of the memory thresholds at a wrong time
(e.g. when UEFI is busy doing something which prevents normal PECI
operation); this leads to the unfixed kernel code returning zero and
dbus-sensors happily using that as a threshold value which later
results in bogus critical over temperature events for the affected
DIMM (as their normal temperatures are always above zero). It was
relatively easy to reproduce on an IceLake-based system.
I consider the current behaviour (in case of PECI timeouts when
requesting DIMM temperature thresholds) to be so broken that changing
it to do the right thing can only do good. The non-failure case is not
affected by this patch.
That said, for sensible operation a dbus-sensors change is indeed
needed and I now have a patch pending upstream review[0] to handle
those errors by retrying until success. Without the patch the daemon
would just load with those thresholds missing but it's better to have
thresholds missing than to have them at zero producing a critical error
right away I think.
[0] https://gerrit.openbmc.org/c/openbmc/dbus-sensors/+/77500/
--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@xxxxxxxxx
