From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
Date: Thu, 14 Sep 2023 18:28:07 +0200
> On Thu, Sep 14, 2023 at 06:25:04PM +0200, Alexander Lobakin wrote:
>> From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
>> Date: Thu, 24 Aug 2023 21:26:49 +0200
[...]
>>> +static void
>>> +ice_set_rx_rings_vlan_proto(struct ice_vsi *vsi, __be16 vlan_ethertype)
>>
>> @vsi can be const (I hope).
>
> I will try to make it const.
>
>> Line can be broken on arguments, not type (I hope).
>>
>
> This is how we break the lines everywhere in this file though :/
I know and would really like us stop at least adding new such
occurrences when not needed :s
>
>>> +{
>>> + u16 i;
>>> +
>>> + ice_for_each_alloc_rxq(vsi, i)
>>> + vsi->rx_rings[i]->pkt_ctx.vlan_proto = vlan_ethertype;
>>> +}
>>> +
>>> /**
>>> * ice_set_vlan_offload_features - set VLAN offload features for the PF VSI
>>> * @vsi: PF's VSI
>>> @@ -6049,6 +6066,11 @@ ice_set_vlan_offload_features(struct ice_vsi *vsi, netdev_features_t features)
>>> if (strip_err || insert_err)
>>> return -EIO;
>>>
>>> + if (enable_stripping)
>>> + ice_set_rx_rings_vlan_proto(vsi, htons(vlan_ethertype));
>>> + else
>>> + ice_set_rx_rings_vlan_proto(vsi, 0);
>>
>> Ternary?
>
> Would look ugly in this particular case, I think, too long expressions and no
> return values.
ice_set_rx_rings_vlan_proto(vsi, strip ? htons(vlan_ethertype) : 0);
?
[...]
>>> - vlan_tag = ice_get_vlan_tag_from_rx_desc(rx_desc);
>>> + vlan_tci = ice_get_vlan_tci(rx_desc);
>>
>> Unrelated: I never was a fan of scattering rx_desc parsing across
>> several files, I remember I moved it to process_skb_fields() in both ice
>> (Hints series) and iavf (libie), maybe do that here as well? Or way too
>> out of context?
>
> A little bit too unrelated to the purpose of the series, but a thing we must do
> in the future.
Sure, +
>
>>
>>>
>>> /* pad the skb if needed, to make a valid ethernet frame */
>>> if (eth_skb_pad(skb))
>>
>> [...]
>>
>> Thanks,
>> Olek
Thanks,
Olek
