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

[Jian Zhang]

> 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.


> 
> 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;