The sizes.h provides a set of common size definitions, use it.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/edac/pnd2_edac.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c
index 2b306f2cc605..45e3c2913d51 100644
--- a/drivers/edac/pnd2_edac.c
+++ b/drivers/edac/pnd2_edac.c
@@ -24,6 +24,7 @@
#include <linux/delay.h>
#include <linux/edac.h>
#include <linux/mmzone.h>
+#include <linux/sizes.h>
#include <linux/smp.h>
#include <linux/bitmap.h>
#include <linux/math64.h>
@@ -109,7 +110,6 @@ static struct mem_ctl_info *pnd2_mci;
#define MOT_CHAN_INTLV_BIT_1SLC_2CH 12
#define MOT_CHAN_INTLV_BIT_2SLC_2CH 13
#define SELECTOR_DISABLED (-1)
-#define _4GB (1ul << 32)
#define PMI_ADDRESS_WIDTH 31
#define PND_MAX_PHYS_BIT 39
@@ -587,7 +587,7 @@ static int get_registers(void)
/* Get a contiguous memory address (remove the MMIO gap) */
static u64 remove_mmio_gap(u64 sys)
{
- return (sys < _4GB) ? sys : sys - (_4GB - top_lm);
+ return (sys < SZ_4G) ? sys : sys - (SZ_4G - top_lm);
}
/* Squeeze out one address bit, shift upper part down to fill gap */
@@ -643,7 +643,7 @@ static int sys2pmi(const u64 addr, u32 *pmiidx, u64 *pmiaddr, char *msg)
/* Give up if address is out of range, or in MMIO gap */
if (addr >= (1ul << PND_MAX_PHYS_BIT) ||
- (addr >= top_lm && addr < _4GB) || addr >= top_hm) {
+ (addr >= top_lm && addr < SZ_4G) || addr >= top_hm) {
snprintf(msg, PND2_MSG_SIZE, "Error address 0x%llx is not DRAM", addr);
return -EINVAL;
}
--
2.43.0.rc1.1.gbec44491f096
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 <[email protected]>
---
drivers/edac/pnd2_edac.c | 39 ++++++++++++++++++---------------------
1 file changed, 18 insertions(+), 21 deletions(-)
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;
--
2.43.0.rc1.1.gbec44491f096
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> From: Andy Shevchenko <[email protected]>
> ...
>
> The sizes.h provides a set of common size definitions, use it.
>
This patch LGTM. Thanks!
Reviewed-by: Qiuxu Zhuo <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/edac/pnd2_edac.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> ...
> From: Andy Shevchenko <[email protected]>
> ...
>
> 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 <[email protected]>
> ---
> 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/[email protected]/
>
> /* 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