Re: [PATCH v1 2/3] EDAC, pnd2: Apply bit macros and helpers where it makes sense

From: Qiuxu Zhuo
Date: Thu Nov 16 2023 - 03:29:28 EST


> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ...
>
> Apply bit macros (BIT()/BIT_ULL()/GENMASK()/etc) and helpers
> (is_power_of_2()/for_each_set_bit()/etc) where it makes sense.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/edac/pnd2_edac.c | 39 ++++++++++++++++++---------------------
> 1 file changed, 18 insertions(+), 21 deletions(-)
>
> ...
> @@ -328,7 +328,7 @@ static void mk_region_mask(char *name, struct region *rp, u64 base, u64 mask)
> pr_info(FW_BUG "MOT mask cannot be zero\n");
> return;
> }
> - if (mask != GENMASK_ULL(PND_MAX_PHYS_BIT, __ffs(mask))) {
> + if (is_power_of_2(mask)) {

The original code verifies whether the 'mask' contains consecutive 1s between the
most significant bit 'PND_MAX_PHYS_BIT' and the least significant bit '__ffs(mask)'.
It isn't the check whether the 'mask' is power of 2.

> pr_info(FW_BUG "MOT mask not power of two\n");

This original pr_info "MOT mask not power of two" is incorrect.
May need to update it like:

pr_info(FW_BUG "MOT mask is invalid\n");

> ...
> /* addrdec values */
> #define AMAP_1KB 0
> @@ -1061,12 +1061,13 @@ static int check_channel(int ch)
>
> static int apl_check_ecc_active(void)
> {
> - int i, ret = 0;
> + unsigned int i;
> + int ret;

Need to initialize the 'ret' to 0.

The LKP reported this warnning as well:
https://lore.kernel.org/all/202311160352.PfYDQfkU-lkp@xxxxxxxxx/

>
> /* Check dramtype and ECC mode for each present DIMM */
> - for (i = 0; i < APL_NUM_CHANNELS; i++)
> - if (chan_mask & BIT(i))
> - ret += check_channel(i);
> + for_each_set_bit(i, &chan_mask, APL_NUM_CHANNELS)
> + ret += check_channel(i);
> +
> return ret ? -EINVAL : 0;
> }
> ...

Thanks!
-Qiuxu