Messages in this thread | | | Date | Wed, 7 Dec 2022 16:45:34 +0100 | From | Sabrina Dubroca <> | Subject | Re: [PATCH net-next v3 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink |
| |
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
[...] > +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 ())
> + > + 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)
> + 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
| |