Re: [PATCH net-next 06/10] net: introduce a net_device_ops macsec helper

[Antoine Tenart]

Hi,

On Thu, Jan 24, 2019 at 10:23:49AM +0100, Antoine Tenart wrote:
> On Wed, Jan 23, 2019 at 12:16:08PM -0800, Florian Fainelli wrote:
> > On 1/23/19 7:56 AM, Antoine Tenart wrote:
> > > @@ -1441,6 +1445,10 @@ struct net_device_ops {
> > >  						u32 flags);
> > >  	int			(*ndo_xsk_async_xmit)(struct net_device *dev,
> > >  						      u32 queue_id);
> > > +#ifdef CONFIG_MACSEC
> > > +	int			(*ndo_macsec)(struct net_device *dev,
> > > +					      struct netdev_macsec *macsec);
> > 
> > You would really want to define an API which is more oriented towards
> > configuring/deconfiguring a MACsec association here, e.g.: similar to
> > what the IPsec offload ndos offer.
> 
> This means mostly moving from a single function using a command field to
> multiple specialized functions to add/remove each element of MACsec
> configuration.
> 
> I don't have strong opinion on the single helper vs a structure
> containing pointers to specialized ones, but out of curiosity what's the
> benefit of such a move? Future additions and maintainability?
> 
> > It is not clear to me whether after your patch series we still need to
> > create a macsec virtual device, and that gets offloaded onto its real
> > device/PHY device, or if we don't need that all?
> 
> After this series, we will still need the virtual MACsec interface. When
> using hardware offloading this interface isn't doing much, but it's the
> interface used to configure all the MACsec connexions.
> 
> This is because, and that's specific to MACsec (vs IPsec), a software
> implementation is already supported and it's using a virtual interface
> to perform all the MACsec related operations (vs hooks in the Rx/Tx
> paths). I really wanted to avoid having two interfaces and ways of
> configuring MACsec depending on if the offloading is used.
> 
> This should also allow in the future to disable at run-time the
> offloading on a given interface, and to still have MACsec working in
> software (or the opposite, with extra work). For this to work, the
> virtual interface still has to provide an Rx and a Tx functions so that
> programs can bind onto the same interface, regardless of if the
> offloading is enabled.

Do you need extra information and explanations about this? I believe
this point is very important as the design choices were influenced a lot
by reusing the s/w implementation logic and API.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com