* [Intel-wired-lan] [bug report] ice: Add support for VSI allocation and deallocation
@ 2018-04-13 12:38 Dan Carpenter
2018-04-17 16:06 ` Venkataramanan, Anirudh
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2018-04-13 12:38 UTC (permalink / raw)
To: intel-wired-lan
[ 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
^ permalink raw reply [flat|nested] 2+ messages in thread
* [Intel-wired-lan] [bug report] ice: Add support for VSI allocation and deallocation
2018-04-13 12:38 [Intel-wired-lan] [bug report] ice: Add support for VSI allocation and deallocation Dan Carpenter
@ 2018-04-17 16:06 ` Venkataramanan, Anirudh
0 siblings, 0 replies; 2+ messages in thread
From: Venkataramanan, Anirudh @ 2018-04-17 16:06 UTC (permalink / raw)
To: intel-wired-lan
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: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180417/0608565a/attachment.bin>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-04-17 16:06 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-13 12:38 [Intel-wired-lan] [bug report] ice: Add support for VSI allocation and deallocation Dan Carpenter
2018-04-17 16:06 ` Venkataramanan, Anirudh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.