Messages in this thread | | | Date | Wed, 16 Aug 2023 13:12:26 +0300 | Subject | Re: [RFC net-next v1 4/5] net: macsec: introduce mdo_insert_tx_tag | From | "Radu Pirea (OSS)" <> |
| |
On 11.08.2023 20:42, Andrew Lunn wrote: > On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote: > >> + if (macsec->offload == MACSEC_OFFLOAD_OFF) { >> + dev->needed_headroom -= ops->needed_headroom; >> + dev->needed_headroom += MACSEC_NEEDED_HEADROOM; >> + dev->needed_tailroom -= ops->needed_tailroom; >> + dev->needed_tailroom += MACSEC_NEEDED_TAILROOM; >> + } else { >> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM; >> + dev->needed_headroom += ops->needed_headroom; >> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM; >> + dev->needed_tailroom += ops->needed_tailroom; >> + } > > It is not obvious to me what this is doing. Should this actually be in > macsec_dev_init()? My main problem is why there is an else condition?
The user can enable/disable offloading after the interface is created, that's why the else condition is needed.
>> +static struct sk_buff *insert_tx_tag(struct sk_buff *skb, >> + struct net_device *dev) >> +{ >> + struct macsec_dev *macsec = macsec_priv(dev); >> + const struct macsec_ops *ops; >> + struct macsec_context ctx; >> + int err; >> + >> + if (!macsec_is_offloaded(macsec)) >> + return ERR_PTR(-EINVAL); > > Hasn't this already been checked in macsec_start_xmit() Yes. This check is useless.
> >> + >> + ops = macsec_get_ops(macsec, &ctx); >> + if (!ops) >> + return ERR_PTR(-EINVAL); >> + >> + if (!ops->mdo_insert_tx_tag) >> + return skb; > > You are in the hot path here. You don't expect this to change from > frame to frame. So could you evaluate this once and store it > somewhere? Maybe in macsec_dev ?
The macsec_dev struct seems to be the right place.
> > Andrew
-- Radu P.
| |