[bug report] EDAC/{skx_common,i10nm}: Refactor show_retry_rd_err_log()

[bug report] EDAC/{skx_common,i10nm}: Refactor show_retry_rd_err_log()

[Dan Carpenter]

Hello Qiuxu Zhuo,

Commit 126168fa2c3e ("EDAC/{skx_common,i10nm}: Refactor
show_retry_rd_err_log()") from Apr 17, 2025 (linux-next), leads to
the following Smatch static checker warning:

	drivers/edac/i10nm_base.c:364 show_retry_rd_err_log()
	warn: should bitwise negate be 'ullong'?

drivers/edac/i10nm_base.c
    325 static void show_retry_rd_err_log(struct decoded_addr *res, char *msg,
    326                                   int len, bool scrub_err)
    327 {
    328         int i, j, n, ch = res->channel, pch = res->cs & 1;
    329         struct skx_imc *imc = &res->dev->imc[res->imc];
    330         u32 offset, status_mask;

status_mask is u32.

    331         struct reg_rrl *rrl;
    332         u64 log, corr;

log is a u64.

    333         bool scrub;
    334         u8 width;
    335 
    336         if (!imc->mbase)
    337                 return;
    338 
    339         rrl = imc->hbm_mc ? res_cfg->reg_rrl_hbm[pch] : res_cfg->reg_rrl_ddr;
    340 
    341         if (!rrl)
    342                 return;
    343 
    344         status_mask = rrl->over_mask | rrl->uc_mask | rrl->v_mask;
    345 
    346         n = snprintf(msg, len, " retry_rd_err_log[");
    347         for (i = 0; i < rrl->set_num; i++) {
    348                 scrub = (rrl->modes[i] == FRE_SCRUB || rrl->modes[i] == LRE_SCRUB);
    349                 if (scrub_err != scrub)
    350                         continue;
    351 
    352                 for (j = 0; j < rrl->reg_num && len - n > 0; j++) {
    353                         offset = rrl->offsets[i][j];
    354                         width = rrl->widths[j];
    355                         log = read_imc_reg(imc, ch, offset, width);
    356 
    357                         if (width == 4)
    358                                 n += snprintf(msg + n, len - n, "%.8llx ", log);
    359                         else
    360                                 n += snprintf(msg + n, len - n, "%.16llx ", log);
    361 
    362                         /* Clear RRL status if RRL in Linux control mode. */
    363                         if (retry_rd_err_log == 2 && !j && (log & status_mask))
--> 364                                 write_imc_reg(imc, ch, offset, width, log & ~status_mask);

This will clear the high 32 bits of log.

    365                 }
    366         }
    367 
    368         /* Move back one space. */
    369         n--;
    370         n += snprintf(msg + n, len - n, "]");
    371 
    372         if (len - n > 0) {
    373                 n += snprintf(msg + n, len - n, " correrrcnt[");
    374                 for (i = 0; i < rrl->cecnt_num && len - n > 0; i++) {
    375                         offset = rrl->cecnt_offsets[i];
    376                         width = rrl->cecnt_widths[i];
    377                         corr = read_imc_reg(imc, ch, offset, width);
    378 
    379                         /* CPUs {ICX,SPR} encode two counters per 4-byte CORRERRCNT register. */
    380                         if (res_cfg->type <= SPR) {
    381                                 n += snprintf(msg + n, len - n, "%.4llx %.4llx ",
    382                                               corr & 0xffff, corr >> 16);
    383                         } else {
    384                         /* CPUs {GNR} encode one counter per CORRERRCNT register. */
    385                                 if (width == 4)
    386                                         n += snprintf(msg + n, len - n, "%.8llx ", corr);
    387                                 else
    388                                         n += snprintf(msg + n, len - n, "%.16llx ", corr);
    389                         }
    390                 }
    391 
    392                 /* Move back one space. */
    393                 n--;
    394                 n += snprintf(msg + n, len - n, "]");
    395         }
    396 }

regards,
dan carpenter

RE: [bug report] EDAC/{skx_common,i10nm}: Refactor show_retry_rd_err_log()

[Zhuo, Qiuxu]

Hi Dan,

Thanks for reporting this.

> From: Dan Carpenter <dan.carpenter@linaro.org>
> [...]
> Hello Qiuxu Zhuo,
> 
> Commit 126168fa2c3e ("EDAC/{skx_common,i10nm}: Refactor
> show_retry_rd_err_log()") from Apr 17, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/edac/i10nm_base.c:364 show_retry_rd_err_log()
> 	warn: should bitwise negate be 'ullong'?
> 
> drivers/edac/i10nm_base.c
> [...]
> 
> status_mask is u32.
> 
>     331         struct reg_rrl *rrl;
>     332         u64 log, corr;
> 
> log is a u64.
> 
> [...]
>     362                         /* Clear RRL status if RRL in Linux control mode. */
>     363                         if (retry_rd_err_log == 2 && !j && (log & status_mask))
> --> 364                                 write_imc_reg(imc, ch, offset, width, log &
> ~status_mask);
> 
> This will clear the high 32 bits of log.

It's OK to clear the high 32 bits of 'log', as we only write the low 32 bits of 'log' to
the 1st RRL register, which is a u32 type. 

To improve code sanity, it might be worthwhile to create a patch that changes 
'status_mask' to a u64 type. @Luck, Tony, should I create a fix patch on top of the
current patch series to fix this warning?

Thanks!
-Qiuxu

RE: [bug report] EDAC/{skx_common,i10nm}: Refactor show_retry_rd_err_log()

[Luck, Tony]

> >     362                         /* Clear RRL status if RRL in Linux control mode. */
> >     363                         if (retry_rd_err_log == 2 && !j && (log & status_mask))
> > --> 364                                 write_imc_reg(imc, ch, offset, width, log &
> > ~status_mask);
> >
> > This will clear the high 32 bits of log.
>
> It's OK to clear the high 32 bits of 'log', as we only write the low 32 bits of 'log' to
> the 1st RRL register, which is a u32 type.
>
> To improve code sanity, it might be worthwhile to create a patch that changes
> 'status_mask' to a u64 type. @Luck, Tony, should I create a fix patch on top of the
> current patch series to fix this warning?

Qiuxu,

Yes. Write a patch for this and give a Reported-by: credit to Dan.

Thanks

-Tony

Re: [bug report] EDAC/{skx_common,i10nm}: Refactor show_retry_rd_err_log()

[Dan Carpenter]

On Wed, Apr 23, 2025 at 04:26:21PM +0000, Luck, Tony wrote:
> > >     362                         /* Clear RRL status if RRL in Linux control mode. */
> > >     363                         if (retry_rd_err_log == 2 && !j && (log & status_mask))
> > > --> 364                                 write_imc_reg(imc, ch, offset, width, log &
> > > ~status_mask);
> > >
> > > This will clear the high 32 bits of log.
> >
> > It's OK to clear the high 32 bits of 'log', as we only write the low 32 bits of 'log' to
> > the 1st RRL register, which is a u32 type.
> >
> > To improve code sanity, it might be worthwhile to create a patch that changes
> > 'status_mask' to a u64 type. @Luck, Tony, should I create a fix patch on top of the
> > current patch series to fix this warning?
> 
> Qiuxu,
> 
> Yes. Write a patch for this and give a Reported-by: credit to Dan.
> 

Thanks.  But mention what you said that it's a false positive.

It's hard to silence a warning like this in Smatch because you'd need to
read into the write_imc_reg() to see that only 4 byte widths are
supported.

regards,
dan carpenter