lkml.org 
[lkml]   [2022]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next v3 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink
Date


> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Wednesday, 7 December 2022 17:46
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; atenart@kernel.org; jiri@resnulli.us
> 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@nvidia.com wrote:
> > From: Emeel Hakim <ehakim@nvidia.com>
> >
> > 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

\
 
 \ /
  Last update: 2022-12-07 16:53    [W:0.057 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site