On 20. 11. 23 12:42, Simon Horman wrote:
On Thu, Nov 16, 2023 at 04:21:12PM +0100, Ivan Vecera wrote:
Add two helpers i40e_(veb|vsi)_get_by_seid() to find corresponding
VEB or VSI by their SEID value and use these helpers to replace
existing open-coded loops.
Reviewed-by: Wojciech Drewek <wojciech.dre...@intel.com>
Signed-off-by: Ivan Vecera <ivec...@redhat.com>
Hi Ivan,
some minor feedback from my side.
...
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
b/drivers/net/ethernet/intel/i40e/i40e.h
index 1e9266de270b..ca8997d29c02 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -1360,4 +1360,38 @@ static inline struct i40e_pf *i40e_hw_to_pf(struct
i40e_hw *hw)
struct device *i40e_hw_to_dev(struct i40e_hw *hw);
+/**
+ * i40e_pf_get_vsi_by_seid - find VSI by SEID
+ * @pf: pointer to a PF
nit: @seid is missing here
+ **/
+static inline struct i40e_vsi *
+i40e_pf_get_vsi_by_seid(struct i40e_pf *pf, u16 seid)
+{
+ struct i40e_vsi *vsi;
+ int i;
+
+ i40e_pf_for_each_vsi(pf, i, vsi)
+ if (vsi->seid == seid)
+ return vsi;
+
+ return NULL;
+}
+
+/**
+ * i40e_pf_get_veb_by_seid - find VEB by SEID
+ * @pf: pointer to a PF
Ditto
...
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
...
@@ -14848,23 +14831,16 @@ struct i40e_veb *i40e_veb_setup(struct i40e_pf *pf,
u16 flags,
}
/* make sure there is such a vsi and uplink */
- i40e_pf_for_each_vsi(pf, vsi_idx, vsi)
- if (vsi->seid == vsi_seid)
- break;
-
- if (vsi_idx == pf->num_alloc_vsi && vsi_seid != 0) {
- dev_info(&pf->pdev->dev, "vsi seid %d not found\n",
- vsi_seid);
- return NULL;
+ if (vsi_seid) {
+ vsi = i40e_pf_get_vsi_by_seid(pf, vsi_seid);
+ if (!vsi) {
+ dev_err(&pf->pdev->dev, "vsi seid %d not found\n",
+ vsi_seid);
+ return NULL;
+ }
}
-
if (uplink_seid && uplink_seid != pf->mac_seid) {
- i40e_pf_for_each_veb(pf, veb_idx, veb) {
- if (veb->seid == uplink_seid) {
- uplink_veb = veb;
- break;
- }
- }
+ uplink_veb = i40e_pf_get_veb_by_seid(pf, uplink_seid);
if (!uplink_veb) {
dev_info(&pf->pdev->dev,
"uplink seid %d not found\n", uplink_seid);
The next part of this function looks like this:
if (!uplink_veb) {
dev_info(&pf->pdev->dev,
"uplink seid %d not found\n", uplink_seid);
return NULL;
}
}
/* get veb sw struct */
veb_idx = i40e_veb_mem_alloc(pf);
if (veb_idx < 0)
goto err_alloc;
veb = pf->veb[veb_idx];
veb->flags = flags;
veb->uplink_seid = uplink_seid;
veb->veb_idx = (uplink_veb ? uplink_veb->idx : I40E_NO_VEB);
veb->enabled_tc = (enabled_tc ? enabled_tc : 0x1);
/* create the VEB in the switch */
ret = i40e_add_veb(veb, vsi);
Smatch complains that vsi may be used uninitialised here.
Which does seem possible to me if vsi_seid is 0.
Yes, the support for floating VEBs is and WAS broken prior this patch
and it is fixed by the following patch.
Prior this patch... Let's vsi_seid == 0:
/* make sure there is such a vsi and uplink */
i40e_pf_for_each_vsi(pf, vsi_idx, vsi)
if (vsi->seid == vsi_seid)
break;
-> here vsi_idx == pf->num_alloc_vsi because there cannot be VSI with
SEID == 0... and VSI points after the pf->vsi[] array.
if (vsi_idx == pf->num_alloc_vsi && vsi_seid != 0) {
dev_info(&pf->pdev->dev, "vsi seid %d not found\n",
vsi_seid);
return NULL;
}
This condition is not met, although vsi_idx == pf->num_alloc_vsi but
vsi_seid == 0 so the rest of code ended by:
ret = i40e_add_veb(veb, vsi);
and vsi pointer points to invalid area (item after the last one from
pf->vsi).
As I mentioned the broken floating VEB functionality (where vsi_seid ==
0 and uplink_seid == 0) is fixed by the following patch.
Thanks,
Ivan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
