diff mbox series

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

Message ID 20231115154940.664664-2-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/3] EDAC, pnd2: Replace custom definition by one from sizes.h |

Commit Message

Andy Shevchenko Nov. 15, 2023, 3:49 p.m. UTC 
  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@linux.intel.com>
---
 drivers/edac/pnd2_edac.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Comments

kernel test robot Nov. 15, 2023, 7:23 p.m. UTC | #1 
Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ras/edac-for-next]
[also build test WARNING on linus/master v6.7-rc1 next-20231115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/EDAC-pnd2-Apply-bit-macros-and-helpers-where-it-makes-sense/20231115-235225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git edac-for-next
patch link:    https://lore.kernel.org/r/20231115154940.664664-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 2/3] EDAC, pnd2: Apply bit macros and helpers where it makes sense
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231116/202311160352.PfYDQfkU-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231116/202311160352.PfYDQfkU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311160352.PfYDQfkU-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/edac/pnd2_edac.c:1069:3: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   ret += check_channel(i);
                   ^~~
   drivers/edac/pnd2_edac.c:1065:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +/ret +1069 drivers/edac/pnd2_edac.c

5c71ad17f97e84d Tony Luck       2017-03-09  1061  
5c71ad17f97e84d Tony Luck       2017-03-09  1062  static int apl_check_ecc_active(void)
5c71ad17f97e84d Tony Luck       2017-03-09  1063  {
f29c26c5b4c5b02 Andy Shevchenko 2023-11-15  1064  	unsigned int i;
f29c26c5b4c5b02 Andy Shevchenko 2023-11-15  1065  	int ret;
5c71ad17f97e84d Tony Luck       2017-03-09  1066  
5c71ad17f97e84d Tony Luck       2017-03-09  1067  	/* Check dramtype and ECC mode for each present DIMM */
f29c26c5b4c5b02 Andy Shevchenko 2023-11-15  1068  	for_each_set_bit(i, &chan_mask, APL_NUM_CHANNELS)
5c71ad17f97e84d Tony Luck       2017-03-09 @1069  		ret += check_channel(i);
f29c26c5b4c5b02 Andy Shevchenko 2023-11-15  1070  
5c71ad17f97e84d Tony Luck       2017-03-09  1071  	return ret ? -EINVAL : 0;
5c71ad17f97e84d Tony Luck       2017-03-09  1072  }
5c71ad17f97e84d Tony Luck       2017-03-09  1073
Qiuxu Zhuo Nov. 16, 2023, 8:28 a.m. UTC | #2 
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ...
> 
> 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@linux.intel.com>
> ---
>  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@intel.com/

>  
>  	/* 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
diff mbox series

Patch

diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 45e3c2913d51..676e02c8dc43 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -183,7 +183,7 @@  static int _apl_rd_reg(int port, int off, int op, u32 *data)
 	}
 
 	P2SB_READ(dword, P2SB_DATA_OFF, data);
-	ret = (status >> 1) & 0x3;
+	ret = (status >> 1) & GENMASK(1, 0);
 out:
 	/* Hide the P2SB device, if it was hidden before */
 	if (hidden)
@@ -307,7 +307,7 @@  static bool two_channels; /* Both PMI channels in one slice enabled */
 
 static u8 sym_chan_mask;
 static u8 asym_chan_mask;
-static u8 chan_mask;
+static unsigned long chan_mask;
 
 static int slice_selector = -1;
 static int chan_selector = -1;
@@ -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)) {
 		pr_info(FW_BUG "MOT mask not power of two\n");
 		return;
 	}
@@ -598,7 +598,7 @@  static void remove_addr_bit(u64 *addr, int bitidx)
 	if (bitidx == -1)
 		return;
 
-	mask = (1ull << bitidx) - 1;
+	mask = BIT_ULL(bitidx) - 1;
 	*addr = ((*addr >> 1) & ~mask) | (*addr & mask);
 }
 
@@ -642,7 +642,7 @@  static int sys2pmi(const u64 addr, u32 *pmiidx, u64 *pmiaddr, char *msg)
 	int sym_chan_shift = sym_channels >> 1;
 
 	/* Give up if address is out of range, or in MMIO gap */
-	if (addr >= (1ul << PND_MAX_PHYS_BIT) ||
+	if (addr >= BIT(PND_MAX_PHYS_BIT) ||
 	   (addr >= top_lm && addr < SZ_4G) || addr >= top_hm) {
 		snprintf(msg, PND2_MSG_SIZE, "Error address 0x%llx is not DRAM", addr);
 		return -EINVAL;
@@ -727,10 +727,10 @@  static int sys2pmi(const u64 addr, u32 *pmiidx, u64 *pmiaddr, char *msg)
 }
 
 /* Translate PMI address to memory (rank, row, bank, column) */
-#define C(n) (0x10 | (n))	/* column */
-#define B(n) (0x20 | (n))	/* bank */
-#define R(n) (0x40 | (n))	/* row */
-#define RS   (0x80)			/* rank */
+#define C(n) (BIT(4) | (n))	/* column */
+#define B(n) (BIT(5) | (n))	/* bank */
+#define R(n) (BIT(6) | (n))	/* row */
+#define RS   (BIT(7))		/* rank */
 
 /* 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;
 
 	/* 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;
 }
 
@@ -1203,12 +1204,9 @@  static void apl_get_dimm_config(struct mem_ctl_info *mci)
 	struct dimm_info *dimm;
 	struct d_cr_drp0 *d;
 	u64	capacity;
-	int	i, g;
-
-	for (i = 0; i < APL_NUM_CHANNELS; i++) {
-		if (!(chan_mask & BIT(i)))
-			continue;
+	unsigned int i, g;
 
+	for_each_set_bit(i, &chan_mask, APL_NUM_CHANNELS) {
 		dimm = edac_get_dimm(mci, i, 0, 0);
 		if (!dimm) {
 			edac_dbg(0, "No allocated DIMM for channel %d\n", i);
@@ -1228,8 +1226,7 @@  static void apl_get_dimm_config(struct mem_ctl_info *mci)
 		}
 
 		pvt->dimm_geom[i] = g;
-		capacity = (d->rken0 + d->rken1) * 8 * (1ul << dimms[g].rowbits) *
-				   (1ul << dimms[g].colbits);
+		capacity = (d->rken0 + d->rken1) * 8 * BIT(dimms[g].rowbits + dimms[g].colbits);
 		edac_dbg(0, "Channel %d: %lld MByte DIMM\n", i, capacity >> (20 - 3));
 		dimm->nr_pages = MiB_TO_PAGES(capacity >> (20 - 3));
 		dimm->grain = 32;
@@ -1295,7 +1292,7 @@  static void dnv_get_dimm_config(struct mem_ctl_info *mci)
 				continue;
 			}
 
-			capacity = ranks_of_dimm[j] * banks * (1ul << rowbits) * (1ul << colbits);
+			capacity = ranks_of_dimm[j] * banks * BIT(rowbits + colbits);
 			edac_dbg(0, "Channel %d DIMM %d: %lld MByte DIMM\n", i, j, capacity >> (20 - 3));
 			dimm->nr_pages = MiB_TO_PAGES(capacity >> (20 - 3));
 			dimm->grain = 32;