Messages in this thread | | | From | Emeel Hakim <> | Subject | RE: [PATCH net-next v3 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink | Date | Wed, 7 Dec 2022 15:52:15 +0000 |
| |
> -----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
| |