RE: [PATCH net-next v3 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink
From: Emeel Hakim
Date: Wed Dec 07 2022 - 10:52:43 EST
> -----Original Message-----
> From: Sabrina Dubroca <sd@xxxxxxxxxxxxxxx>
> Sent: Wednesday, 7 December 2022 17:46
> To: Emeel Hakim <ehakim@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Raed Salem <raeds@xxxxxxxxxx>;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; atenart@xxxxxxxxxx; jiri@xxxxxxxxxxx
> Subject: Re: [PATCH net-next v3 1/2] macsec: add support for
> IFLA_MACSEC_OFFLOAD in macsec_changelink
>
> External email: Use caution opening links or attachments
>
>
> 2022-12-07, 12:10:16 +0200, ehakim@xxxxxxxxxx wrote:
> > From: Emeel Hakim <ehakim@xxxxxxxxxx>
> >
> > Add support for changing Macsec offload selection through the netlink
> > layer by implementing the relevant changes in macsec_change link.
>
> nit: macsec_changelink
Ack
> [...]
> > +static int macsec_update_offload(struct macsec_dev *macsec, enum
> > +macsec_offload offload) {
> > + enum macsec_offload prev_offload;
> > + const struct macsec_ops *ops;
> > + struct macsec_context ctx;
> > + int ret = 0;
> > +
> > + prev_offload = macsec->offload;
> > +
> > + /* Check if the device already has rules configured: we do not support
> > + * rules migration.
> > + */
> > + if (macsec_is_configured(macsec))
> > + return -EBUSY;
> > +
> > + ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload :
> offload,
> > + macsec, &ctx);
> > + if (!ops)
> > + return -EOPNOTSUPP;
> > +
> > + macsec->offload = offload;
> > +
> > + ctx.secy = &macsec->secy;
> > + ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops-
> >mdo_del_secy, &ctx) :
> > + macsec_offload(ops->mdo_add_secy, &ctx);
>
> I think aligning the two macsec_offload(...) calls would make this a bit easier to
> read:
>
> ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops-
> >mdo_del_secy, &ctx)
> : macsec_offload(ops->mdo_add_secy, &ctx);
>
> (and remove the unnecessary ())
Ack
> > +
> > + if (ret)
> > + macsec->offload = prev_offload;
> > +
> > + return ret;
> > +}
> > +
>
> [...]
> > +static int macsec_changelink_upd_offload(struct net_device *dev,
> > +struct nlattr *data[]) {
> > + enum macsec_offload offload;
> > + struct macsec_dev *macsec;
> > +
> > + macsec = macsec_priv(dev);
> > + offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);
>
> All those checks are also present in macsec_upd_offload, why not move them into
> macsec_update_offload as well? (and then you don't really need
> macsec_changelink_upd_offload anymore)
>
Right, I thought about it , but I realized that those checks are done before holding the lock in macsec_upd_offload
and if I move them to macsec_update_offload I will hold the lock for a longer time , I want to minimize the time
of holding the lock.
> > + if (macsec->offload == offload)
> > + return 0;
> > +
> > + /* Check if the offloading mode is supported by the underlying layers */
> > + if (offload != MACSEC_OFFLOAD_OFF &&
> > + !macsec_check_offload(offload, macsec))
> > + return -EOPNOTSUPP;
> > +
> > + /* Check if the net device is busy. */
> > + if (netif_running(dev))
> > + return -EBUSY;
> > +
> > + return macsec_update_offload(macsec, offload); }
> > +
>
> --
> Sabrina