On Mon, Nov 20, 2023 at 06:55:42PM +0100, Ivan Vecera wrote:
>
> 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,
I see that I flagged a false positive, sorry about that.
I understand things quite a bit better after reading your explanation
above.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
