Re: [PATCH] hwmon: peci: dimmtemp: Retry unstable DIMM mask detection

[Winiarska, Iwona]

On Sun, 2026-09-20 at 10:59 +0800, Jian Zhang wrote:
> > 2026年9月19日 00:59，Guenter Roeck <linux@xxxxxxxxxxxx> 写道：
> > 
> > On Fri, Sep 18, 2026 at 11:18:31AM +0800, Jian Zhang wrote:
> > > Add a stable DIMM mask check around PECI DIMM discovery.
> > > 
> > > During host DIMM initialization, PECI can answer the DIMM temperature
> > > configuration reads before every DIMM has reached a stable ready state.
> > > There is no separate all-DIMM-ready signal for this driver to wait on,
> > > so a single early read can capture only part of the populated DIMM mask.
> > > Retrying the mask check avoids locking that transient state into the
> > > hwmon device layout.
> > > 
> > > The first successful scan is saved, and the driver immediately scans
> > > again before registering hwmon channels. If the two masks differ, the
> > > driver returns -EAGAIN so the existing delayed detection work can retry
> > > instead of exposing a partial channel set.
> > > 
> > > Signed-off-by: Jian Zhang <zhangjian.3032@xxxxxxxxxxxxx>
> > > ---
> > > 
> > > This was tested on an affected platform where probing during
> > > host DIMM initialization could expose an incomplete DIMM mask. With the
> > > extra read, the driver observed the completed DIMM mask and exported the
> > > expected hwmon channels.
> > > 
> > > This is intentionally a conservative retry of the existing detection
> > > path. It does not add a sleep or force another delayed-work interval
> > > between the two reads, so it avoids adding an unconditional probe delay.
> > > However, this also means it is not a full proof that the host DIMM
> > > initialization has completed.
> > > 
> > > I am not aware of an Intel-provided all-DIMM-ready indication for this
> > > path. Feedback would be appreciated if there is a better way to decide
> > > that the PECI DIMM mask is stable before registering the hwmon device.
> > > 
> > > drivers/hwmon/peci/dimmtemp.c | 28 +++++++++++++++++++++++++++-
> > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hwmon/peci/dimmtemp.c b/drivers/hwmon/peci/dimmtemp.c
> > > index bd3e8715dfec..983e1e415ec2 100644
> > > --- a/drivers/hwmon/peci/dimmtemp.c
> > > +++ b/drivers/hwmon/peci/dimmtemp.c
> > > @@ -285,6 +285,32 @@ static int check_populated_dimms(struct peci_dimmtemp
> > > *priv)
> > > return 0;
> > > }
> > > 
> > > +static int check_populated_dimms_stable(struct peci_dimmtemp *priv)
> > > +{
> > > + DECLARE_BITMAP(dimm_mask, DIMM_NUMS_MAX);
> > > + int ret;
> > > +
> > > + ret = check_populated_dimms(priv);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + bitmap_copy(dimm_mask, priv->dimm_mask, DIMM_NUMS_MAX);
> > > +
> > > + ret = check_populated_dimms(priv);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!bitmap_equal(dimm_mask, priv->dimm_mask, DIMM_NUMS_MAX)) {
> > > + dev_dbg(priv->dev,
> > > + "Deferred unstable DIMM mask: %*pbl -> %*pbl\n",
> > > + DIMM_NUMS_MAX, dimm_mask, DIMM_NUMS_MAX,
> > > + priv->dimm_mask);
> > > + return -EAGAIN;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > 
> > I agree with the idea, but check_populated_dimms() already has retry
> > support. I don't see why it would make sense to add retry detection
> > (because additional DIMMs were detected) on top of retry detection
> > (because no DIMMs were detected).
> 
> Right, the existing retry in check_populated_dimms() does not cover this
> case.
> 
> It only retries when no usable DIMM mask is available, for example when
> all channel/rank reads return -EINVAL or when the resulting mask is
> empty.
> 
> The failure I observed is a partial-success case: the PECI reads succeed
> and produce a non-empty mask, so check_populated_dimms() returns 0, but
> the mask may still miss DIMMs while host DIMM initialization is still in
> progress. Once hwmon is registered with that partial mask, the missing
> channels are not added later.
> 

Please refactor the existing check_populated_dimms() to cover both cases,
reusing the counter that's present there instead of wrapping it with another
retry layer. You might need to extract some of the logic inside the function to
a helper to make it readable.

Thanks
-Iwona

> 
> > 
> > Thanks,
> > Guenter
> > 
> > > +
> > > static int create_dimm_temp_label(struct peci_dimmtemp *priv, int chan)
> > > {
> > > int rank = chan / priv->gen_info->dimm_idx_max;
> > > @@ -322,7 +348,7 @@ static int create_dimm_temp_info(struct peci_dimmtemp
> > > *priv)
> > > * All other states mean that the platform never reached the state that
> > > * allows to check DIMM state - causing us to retry later on.
> > > */
> > > - ret = check_populated_dimms(priv);
> > > + ret = check_populated_dimms_stable(priv);
> > > if (ret == -ENODEV) {
> > > dev_dbg(priv->dev, "No DIMMs found\n");
> > > return 0;