Hi Stephen, Thank you for your review. Regarding the changes to the rte_bitops API, should these updates be implemented in V3 or deferred to the next release version?
Regards Wenbo > -----Original Message----- > From: Stephen Hemminger <step...@networkplumber.org> > Sent: 2025年6月30日 23:07 > To: Wenbo Cao <caowe...@mucse.com> > Cc: Ferruh Yigit <ferruh.yi...@amd.com>; dev@dpdk.org; yao...@mucse.com; > sta...@dpdk.org > Subject: Re: [PATCH v2 1/3] net/rnp: add check firmware respond info > > On Mon, 30 Jun 2025 13:57:51 +0800 > Wenbo Cao <caowe...@mucse.com> wrote: > > > Add logic checks at critical points to detect potentially illegal > > firmware information, preventing subsequent logic exceptions. > > > > Fixes: 52aae4ed4ffb ("net/rnp: add device capabilities") > > Fixes: 52dfb84e14be ("net/rnp: add device init and uninit") > > Cc: sta...@dpdk.org > > > > Signed-off-by: Wenbo Cao <caowe...@mucse.com> > > Reviewed-by: Stephen Hemminger <step...@networkplumber.org> > > > int rnp_mbx_fw_get_capability(struct rnp_eth_port *port) { > > struct rnp_phy_abilities_rep ability; @@ -252,17 +253,29 @@ int > > rnp_mbx_fw_get_capability(struct rnp_eth_port *port) > > hw->nic_mode = ability.nic_mode; > > /* get phy<->lane mapping info */ > > lane_cnt = rte_popcount32(hw->lane_mask); > > + if (lane_cnt > RNP_MAX_PORT_OF_PF) { > > + RNP_PMD_LOG(ERR, "firmware invalid lane_mask"); > > + return -EINVAL; > > + } > > temp_mask = hw->lane_mask; > > + if (temp_mask == 0 || temp_mask > RNP_MAX_LANE_MASK) { > > + RNP_PMD_LOG(ERR, "lane_mask is invalid 0x%.2x", > temp_mask); > > + return -EINVAL; > > + } > > if (ability.e.ports_is_sgmii_valid) > > is_sgmii_bits = ability.e.lane_is_sgmii; > > for (idx = 0; idx < lane_cnt; idx++) { > > hw->phy_port_ids[idx] = port_ids[idx]; > > + if (temp_mask == 0) { > > + RNP_PMD_LOG(ERR, "temp_mask is zero at > idx=%d", idx); > > + return -EINVAL; > > + } > > lane_bit = ffs(temp_mask) - 1; > > lane_idx = port_ids[idx] % lane_cnt; > > hw->lane_of_port[lane_idx] = lane_bit; > > is_sgmii = lane_bit & is_sgmii_bits ? 1 : 0; > > hw->lane_is_sgmii[lane_idx] = is_sgmii; > > - temp_mask &= ~RTE_BIT32(lane_bit); > > + temp_mask &= ~(1ULL << lane_bit); > > Rather than using ffs directly better to consistently use rte_bitops which has > rte_ffs32(). You can address it in a future version. >