On Thu, Sep 14, 2023 at 06:38:04PM +0200, Alexander Lobakin wrote:
> 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
I think with such minor stuff, it is more important to keep style consistent in
files.
>
> >
> >>> +{
> >>> + 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
