Skip to contentSkip to conversation

Add support of Cavium Liquidio ethernet adapters

DISCUSSION Dan Carpenter 1 participant 3 messages Jun 23, 2015 – Jun 26, 2015 Kernel janitors

lore
Share & tools
lore
#0 lore raw

Hello Raghu Vatsavayi,

The patch f21fb3ed364b: "Add support of Cavium Liquidio ethernet adapters" from Jun 9, 2015, leads to the following static checker warning:

drivers/net/ethernet/cavium/liquidio/lio_main.c:2530 handle_timestamp()
warn: suspicious bitop condition

drivers/net/ethernet/cavium/liquidio/lio_main.c
2529
2530 if (unlikely((skb_shinfo(skb)->tx_flags | SKBTX_IN_PROGRESS) != 0)) {

Don't use unlikely() unless you have benchmark data to prove that it's worth the extra difficulty in reading.

Double negatives like != 0 don't not make things more difficult to read.

This condition is always true, so that's probably a bug. & vs |.

  2531                  struct skb_shared_hwtstamps ts;
  2532                  u64 ns = resp->timestamp;

regards,
dan carpenter

#1 lore raw

Hello Raghu Vatsavayi,

The patch f21fb3ed364b: "Add support of Cavium Liquidio ethernet adapters" from Jun 9, 2015, leads to the following static checker warning:

drivers/net/ethernet/cavium/liquidio/lio_main.c:2267 liquidio_set_mcast_list()
warn: buffer overflow 'nctrl.udd' 32 <= 32

drivers/net/ethernet/cavium/liquidio/lio_main.c
2236 static void liquidio_set_mcast_list(struct net_device *netdev) 2237 {

  2238          struct lio *lio = GET_LIO(netdev);
  2239          struct octeon_device *oct = lio->oct_dev;
  2240          struct octnic_ctrl_pkt nctrl;
  2241          struct octnic_ctrl_params nparams;
  2242          struct netdev_hw_addr *ha;
  2243          u64 *mc;
  2244          int ret, i;
  2245          int mc_count = min(netdev_mc_count(netdev), MAX_OCTEON_MULTICAST_ADDR);
  2246  
  2247          memset(&nctrl, 0, sizeof(struct octnic_ctrl_pkt));
  2248  
  2249          /* Create a ctrl pkt command to be sent to core app. */
  2250          nctrl.ncmd.u64 = 0;
  2251          nctrl.ncmd.s.cmd = OCTNET_CMD_SET_MULTI_LIST;
  2252          nctrl.ncmd.s.param1 = lio->linfo.ifidx;
  2253          nctrl.ncmd.s.param2 = get_new_flags(netdev);
  2254          nctrl.ncmd.s.param3 = mc_count;
  2255          nctrl.ncmd.s.more = mc_count;
  2256          nctrl.netpndev = (u64)netdev;
  2257          nctrl.cb_fn = liquidio_link_ctrl_cmd_completion;
  2258  
  2259          /* copy all the addresses into the udd */
  2260          i = 0;
  2261          mc = &nctrl.udd[0];
  2262          netdev_for_each_mc_addr(ha, netdev) {
  2263                  *mc = 0;
  2264                  memcpy(((u8 *)mc) + 2, ha->addr, ETH_ALEN);
  2265                  /* no need to swap bytes */

2266
2267 if (++mc > &nctrl.udd[mc_count])

The static checker will still complain, but I think the ">" should be ">=". The address "&nctrl.udd[mc_count]" is one space beyond the end of the array.

2268 break;
2269 }

regards,
dan carpenter

#2 lore raw
In reply to Dan Carpenter

Hello Raghu Vatsavayi,

The patch f21fb3ed364b: "Add support of Cavium Liquidio ethernet adapters" from Jun 9, 2015, leads to the following static checker warning:

drivers/net/ethernet/cavium/liquidio/octeon_console.c:326 cvmx_bootmem_phy_named_block_find()
error: potential null dereference 'name_tmp'. (kmalloc returns null)

drivers/net/ethernet/cavium/liquidio/octeon_console.c
319 for (i = 0; i < num_blocks; i++) {
320 u64 named_size 321 CVMX_BOOTMEM_NAMED_GET_FIELD(oct, named_addr,
322 size);
323 if (name && named_size) {
324 char *name_tmp 325 kmalloc(name_length + 1, GFP_KERNEL);

kmalloc can fail.

   326                                  CVMX_BOOTMEM_NAMED_GET_NAME(oct, named_addr,
   327                                                              name_tmp,
   328                                                              name_length);
   329                                  if (!strncmp(name, name_tmp, name_length)) {
   330                                          result = named_addr;
   331                                          kfree(name_tmp);
   332                                          break;
   333                                  }
   334                                  kfree(name_tmp);
   335                          } else if (!name && !named_size) {
   336                                  result = named_addr;
   337                                  break;
   338                          }
   339  
   340                          named_addr +   341                                  sizeof(struct cvmx_bootmem_named_block_desc);
   342                  }

regards,
dan carpenter