lkml.org 
[lkml]   [2023]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH iwl-next v3 3/5] i40e: Add helpers to find VSI and VEB by SEID and use them
From

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.drewek@intel.com>
>> Signed-off-by: Ivan Vecera <ivecera@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

\
 
 \ /
  Last update: 2023-11-20 18:56    [W:0.049 / U:0.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site