Skip to contentSkip to conversation

clk: stm32: Add clock driver for STM32F4[23]xxx devices

DISCUSSION Dan Carpenter 2 participants 3 messages Jun 26, 2015 Common clk

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Daniel Thompson,

The patch 358bdf892f6b: "clk: stm32: Add clock driver for STM32F4[23]xxx devices" from Jun 10, 2015, leads to the following static checker warning:

	drivers/clk/clk-stm32f4.c:271 stm32f4_rcc_lookup_clk_idx()
	warn: buffer overflow 'table' 3 <= 3

drivers/clk/clk-stm32f4.c
258 static int stm32f4_rcc_lookup_clk_idx(u8 primary, u8 secondary) 259 {
260 u64 table[ARRAY_SIZE(stm32f42xx_gate_map)];
261
262 if (primary == 1) {
263 if (WARN_ON(secondary > FCLK))

Is this off by one? >= instead of >?

264 return -EINVAL;
265 return secondary;
266 }
267
268 memcpy(table, stm32f42xx_gate_map, sizeof(table));
269
270 /* only bits set in table can be used as indices */
271 if (WARN_ON(secondary > 8 * sizeof(table) ||

This one is definitely off by one. Also it might be easier to read as:

		if (WARN_ON(secondary >= 64 * ARRAY_SIZE(table) ||
   272                      0 == (table[BIT_ULL_WORD(secondary)] &
   273                            BIT_ULL_MASK(secondary))))
   274                  return -EINVAL;
   275  
   276          /* mask out bits above our current index */
   277          table[BIT_ULL_WORD(secondary)] &=
   278              GENMASK_ULL(secondary % BITS_PER_LONG_LONG, 0);
   279  
   280          return FCLK + hweight64(table[0]) +
   281                 (BIT_ULL_WORD(secondary) >= 1 ? hweight64(table[1]) : 0) +
   282                 (BIT_ULL_WORD(secondary) >= 2 ? hweight64(table[2]) : 0);
   283  }

regards,
dan carpenter

#1 lore raw
Quoted 10 lines · Dan Carpenter

Hello Daniel Thompson,

The patch 358bdf892f6b: "clk: stm32: Add clock driver for STM32F4[23]xxx devices" from Jun 10, 2015, leads to the following static checker warning:

	drivers/clk/clk-stm32f4.c:271 stm32f4_rcc_lookup_clk_idx()
	warn: buffer overflow 'table' 3 <= 3

drivers/clk/clk-stm32f4.c

Quoted 8 lines
    258  static int stm32f4_rcc_lookup_clk_idx(u8 primary, u8 secondary)
    259  {
    260          u64 table[ARRAY_SIZE(stm32f42xx_gate_map)];
    261
    262          if (primary == 1) {
    263                  if (WARN_ON(secondary > FCLK))

Is this off by one? >= instead of >?

This one is ok (FCLK is a valid value for secondary when primary is 1).

Quoted 10 lines
    264                          return -EINVAL;
    265                  return secondary;
    266          }
    267
    268          memcpy(table, stm32f42xx_gate_map, sizeof(table));
    269
    270          /* only bits set in table can be used as indices */
    271          if (WARN_ON(secondary > 8 * sizeof(table) ||

This one is definitely off by one.

Agreed.

Quoted 2 lines

Also it might be easier to read as:

		if (WARN_ON(secondary >= 64 * ARRAY_SIZE(table) ||

I guess the thing we want to be clear is that we are comparing against the number of bits in table.

Maybe just "BITS_PER_BYTE * sizeof(table)" achieves that best?

BTW... you're happy for me to send a patch on this (with a Reported-by:)?

Daniel.

author
#2 lore raw
Quoted 2 lines · Daniel Thompson

Maybe just "BITS_PER_BYTE * sizeof(table)" achieves that best?

That works.

Quoted 1 line

BTW... you're happy for me to send a patch on this (with a Reported-by:)?

Yes, please.

regards,
dan carpenter