From mboxrd@z Thu Jan 1 00:00:00 1970 From: Venkataramanan, Anirudh Date: Tue, 17 Apr 2018 16:06:02 +0000 Subject: [Intel-wired-lan] [bug report] ice: Add support for VSI allocation and deallocation In-Reply-To: <20180413123837.GA17689@mwanda> References: <20180413123837.GA17689@mwanda> Message-ID: <1523981160.15348.84.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Fri, 2018-04-13 at 15:38 +0300, Dan Carpenter wrote: > [ These are unpublished Smatch warnings because they're often false > positives. I'm reviewing these and I'm like 80% some are false > positivies, but then I get to others and it feels 80% like they > look > buggy. And in the end I don't know the code very well so I'm just > going to forward all of them. -- dan ] > > Hello Anirudh Venkataramanan, > > The patch 3a858ba392c3: "ice: Add support for VSI allocation and > deallocation" from Mar 20, 2018, leads to the following static > checker warning: > > drivers/net/ethernet/intel/ice/ice_main.c:1360 > ice_set_dflt_vsi_ctx() warn: odd binop '0x0 & 0x18' > > drivers/net/ethernet/intel/ice/ice_main.c > 1344 static void ice_set_dflt_vsi_ctx(struct ice_vsi_ctx *ctxt) > 1345 { > 1346 u32 table = 0; > 1347 > 1348 memset(&ctxt->info, 0, sizeof(ctxt->info)); > 1349 /* VSI's should be allocated from shared pool */ > 1350 ctxt->alloc_from_pool = true; > 1351 /* Src pruning enabled by default */ > 1352 ctxt->info.sw_flags = ICE_AQ_VSI_SW_FLAG_SRC_PRUNE; > 1353 /* Traffic from VSI can be sent to LAN */ > 1354 ctxt->info.sw_flags2 = ICE_AQ_VSI_SW_FLAG_LAN_ENA; > 1355 /* Allow all packets untagged/tagged */ > 1356 ctxt->info.port_vlan_flags = > ((ICE_AQ_VSI_PVLAN_MODE_ALL & > > 1357 ICE_AQ_VSI_PVLAN_MODE_M) > >> > > 1358 ICE_AQ_VSI_PVLAN_MODE_S); > 1359 /* Show VLAN/UP from packets in Rx descriptors */ > 1360 ctxt->info.port_vlan_flags |= > ((ICE_AQ_VSI_PVLAN_EMOD_STR_BOTH & > > 1361 ICE_AQ_VSI_PVLAN_EMOD_M > ) >> > > This probably should be | instead of &. > > > 1362 ICE_AQ_VSI_PVLAN_EMOD_S) > ; > 1363 /* Have 1:1 UP mapping for both ingress/egress tables > */ > 1364 table |= ICE_UP_TABLE_TRANSLATE(0, 0); > 1365 table |= ICE_UP_TABLE_TRANSLATE(1, 1); > 1366 table |= ICE_UP_TABLE_TRANSLATE(2, 2); > 1367 table |= ICE_UP_TABLE_TRANSLATE(3, 3); > 1368 table |= ICE_UP_TABLE_TRANSLATE(4, 4); > 1369 table |= ICE_UP_TABLE_TRANSLATE(5, 5); > 1370 table |= ICE_UP_TABLE_TRANSLATE(6, 6); > 1371 table |= ICE_UP_TABLE_TRANSLATE(7, 7); > 1372 ctxt->info.ingress_table = cpu_to_le32(table); > 1373 ctxt->info.egress_table = cpu_to_le32(table); > 1374 /* Have 1:1 UP mapping for outer to inner UP table */ > 1375 ctxt->info.outer_up_table = cpu_to_le32(table); > 1376 /* No Outer tag support outer_tag_flags remains to > zero */ > 1377 } > > drivers/net/ethernet/intel/ice/ice_main.c:2066 > ice_req_irq_msix_misc() warn: odd binop '0x0 & 0x1800' > drivers/net/ethernet/intel/ice/ice_main.c:2072 > ice_req_irq_msix_misc() warn: odd binop '0x0 & 0x1800' > > drivers/net/ethernet/intel/ice/ice_main.c > 2058 ice_free_res(pf->irq_tracker, 1, > ICE_RES_MISC_VEC_ID); > 2059 return err; > 2060 } > 2061 > 2062 skip_req_irq: > 2063 ice_ena_misc_vector(pf); > 2064 > 2065 val = (pf->oicr_idx & PFINT_OICR_CTL_MSIX_INDX_M) | > 2066 (ICE_RX_ITR & PFINT_OICR_CTL_ITR_INDX_M) | > ^^^^^^^^^^ > This is 0. I had no idea what was intended here. Possibly this is > fine? > > 2067 PFINT_OICR_CTL_CAUSE_ENA_M; > 2068 wr32(hw, PFINT_OICR_CTL, val); > 2069 > 2070 /* This enables Admin queue Interrupt causes */ > 2071 val = (pf->oicr_idx & PFINT_FW_CTL_MSIX_INDX_M) | > 2072 (ICE_RX_ITR & PFINT_FW_CTL_ITR_INDX_M) | > ^^^^^^^^^^ > Same. > > 2073 PFINT_FW_CTL_CAUSE_ENA_M; > 2074 wr32(hw, PFINT_FW_CTL, val); > 2075 > 2076 itr_gran = hw->itr_gran_200; > 2077 > 2078 wr32(hw, GLINT_ITR(ICE_RX_ITR, pf->oicr_idx), > 2079 ITR_TO_REG(ICE_ITR_8K, itr_gran)); > 2080 > 2081 ice_flush(hw); > 2082 ice_irq_dynamic_ena(hw, NULL, NULL); > 2083 > 2084 return 0; > 2085 } > > drivers/net/ethernet/intel/ice/ice_common.c:1612 > __ice_aq_get_set_rss_lut() warn: odd binop '0x0 & 0xc' > > drivers/net/ethernet/intel/ice/ice_common.c > 1608 > 1609 /* LUT size is only valid for Global and PF table > types */ > 1610 if (lut_size == ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_128) > { > 1611 flags |= > (ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_128_FLAG << > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > ^^^^^ > This is zero. I'm going to say it looks intentional. (Feel free to > ignore these false positives. Never change the code just to please > the > static checker). > > 1612 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_S) > & > 1613 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_M; > 1614 } else if (lut_size == > ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_512) { > 1615 flags |= > (ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_512_FLAG << > 1616 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_S) > & > 1617 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_M; > 1618 } else if ((lut_size == > ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_2K) && > 1619 (lut_type == > ICE_AQC_GSET_RSS_LUT_TABLE_TYPE_PF)) { > 1620 flags |= > (ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_2K_FLAG << > 1621 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_S) > & > 1622 ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_M; > 1623 } else { > 1624 status = ICE_ERR_PARAM; > 1625 goto ice_aq_get_set_rss_lut_exit; > 1626 } > 1627 > > drivers/net/ethernet/intel/ice/ice_switch.c:648 ice_add_marker_act() > warn: odd binop '0x380000 & 0x7fff8' > drivers/net/ethernet/intel/ice/ice_switch.c:655 ice_add_marker_act() > warn: odd binop '0x0 & 0x7fff8' > > drivers/net/ethernet/intel/ice/ice_switch.c > 635 act = ICE_LG_ACT_VSI_FORWARDING | > ICE_LG_ACT_VALID_BIT; > 636 act |= (vsi_info << ICE_LG_ACT_VSI_LIST_ID_S) & > 637 ICE_LG_ACT_VSI_LIST_ID_M; > 638 if (m_ent->vsi_count > 1) > 639 act |= ICE_LG_ACT_VSI_LIST; > 640 lg_act->pdata.lg_act.act[0] = cpu_to_le32(act); > 641 > 642 /* Second action descriptor type */ > 643 act = ICE_LG_ACT_GENERIC; > 644 > 645 act |= (1 << ICE_LG_ACT_GENERIC_VALUE_S) & > ICE_LG_ACT_GENERIC_VALUE_M; > 646 lg_act->pdata.lg_act.act[1] = cpu_to_le32(act); > 647 > 648 act = (7 << ICE_LG_ACT_GENERIC_OFFSET_S) & > ICE_LG_ACT_GENERIC_VALUE_M; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > ^^^^^^^^^^^^^^^^ > This feels buggy, but I have no idea what was intended. > > 649 > 650 /* Third action Marker value */ > 651 act |= ICE_LG_ACT_GENERIC; > 652 act |= (sw_marker << ICE_LG_ACT_GENERIC_VALUE_S) & > 653 ICE_LG_ACT_GENERIC_VALUE_M; > 654 > 655 act |= (0 << ICE_LG_ACT_GENERIC_OFFSET_S) & > ICE_LG_ACT_GENERIC_VALUE_M; > ^ > > Obviously intended, but I feel like the literal 0 doesn't add > anything > in terms of making it more readable... > > 656 lg_act->pdata.lg_act.act[2] = cpu_to_le32(act); > 657 > 658 /* call the fill switch rule to fill the lookup tx rx > structure */ > 659 ice_fill_sw_rule(hw, &m_ent->fltr_info, rx_tx, > 660 ice_aqc_opc_update_sw_rules); > 661 > 662 /* Update the action to point to the large action id > */ > 663 rx_tx->pdata.lkup_tx_rx.act = > 664 cpu_to_le32(ICE_SINGLE_ACT_PTR | > 665 ((l_id << > ICE_SINGLE_ACT_PTR_VAL_S) & > 666 ICE_SINGLE_ACT_PTR_VAL_M)); > > regards, > dan carpenter Thanks for reporting, Dan. I am looking into this. - Ani -------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/x-pkcs7-signature Size: 3302 bytes Desc: not available URL: