RE: [PATCH net-next v3 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink

[Emeel Hakim]

> -----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