Re: MACsec hardware offloading

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hi Andrew,

On Fri, Jul 13, 2018 at 06:20:02PM +0200, Andrew Lunn wrote:
> On Fri, Jul 13, 2018 at 04:46:08PM +0200, Antoine Tenart wrote:
> 
> > One important point about adding MACsec offloading in Linux is this can
> > be done in either the MAC or the PHY. While I'll be working on making
> > this work in a PHY, I know for sure some MAC do have the same capability
> > (including for example the Intel ixgbe NIC).
> 
> I see in your current code, you check if the netdev has a phydev, and
> if the phydev supports macsec, and then go straight to the phy. That
> means there is no need to modify the MAC driver, it should just work.
> If not, you call the MAC.

Yes. I'm just not sure if the PHY implementation should have precedence
over the MAC one, or the opposite, or if the user should be able to
select the provider to use.

> I would add support for the PHY to return -EOPNOTSUPP and then fall
> back to trying the MAC.

Good idea!

> Also, your current checks are
> inconsistent. macsec_hw_offload_capable() checks for
> phydev->drv->macsec, where as macsec_hw_offload() just checks for
> dev->real_dev->phydev->drv. It looks like
> dev->real_dev->phydev->drv->macsec could be a NULL pointer and bad
> things happen.

OK, I'll be careful and fix this.

> For switchdev, when we offload to a switch, the switch nearly always
> has the option to say it could not accept the offload. We then fall
> back to performing the needed action in software. At the moment, i
> don't see you checking the return code of macsec_hw_offload(). So it
> is not clear to me if this software fallback works.

Right, that should be supported. Although I'm not sure all offloading
helpers can have a fallback (especially if they depend on a previous
one), but I'll try to have this kind of behaviour in place whenever
possible. Or maybe it'll just work for all helpers, as the MACsec state
will always be kept in the software implementation part.

> The Switchdev API is also transaction based, with a prepare and a
> commit phase. All resource allocation happens in the prepare phase and
> at this stage, you can return errors indicating offload is not
> possible. The commit phase is not allowed to fail. You probably want
> to go look at the mailing list archive and look at why this
> architecture was decided on.

Thanks for the hint, I'll have a look at this design pattern.

> Maybe the MAC part of this should actually use the switchdev API?  You
> then get a lot of infrastructure for free.

I'll have a look at this as well. With the prepare/commit logic already
in place, it could be easier to implement.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com