Re: [PATCH net 0/5] macsec: offload-related fixes

[Sabrina Dubroca <sd@queasysnail.net>]

2022-10-24, 11:43:26 +0300, Leon Romanovsky wrote:
> On Mon, Oct 24, 2022 at 10:24:28AM +0200, Sabrina Dubroca wrote:
> > 2022-10-23, 10:52:56 +0300, Leon Romanovsky wrote:
> > > On Thu, Oct 20, 2022 at 03:54:28PM +0200, Sabrina Dubroca wrote:
> > > > 2022-10-18, 09:28:08 +0300, Leon Romanovsky wrote:
> > > > > On Fri, Oct 14, 2022 at 04:03:56PM +0200, Antoine Tenart wrote:
> > > > > > Quoting Leon Romanovsky (2022-10-14 13:03:57)
> > > > > > > On Fri, Oct 14, 2022 at 09:43:45AM +0200, Sabrina Dubroca wrote:
> > > > > > > > 2022-10-14, 09:13:39 +0300, Leon Romanovsky wrote:
> > > > > > > > > On Thu, Oct 13, 2022 at 04:15:38PM +0200, Sabrina Dubroca wrote:
> > > 
> > > <...>
> > > 
> > > > > > - With the revert: IPsec and MACsec can be offloaded to the lower dev.
> > > > > >   Some features might not propagate to the MACsec dev, which won't allow
> > > > > >   some performance optimizations in the MACsec data path.
> > > > > 
> > > > > My concern is related to this sentence: "it's not possible to offload macsec
> > > > > to lower devices that also support ipsec offload", because our devices support
> > > > > both macsec and IPsec offloads at the same time.
> > > > > 
> > > > > I don't want to see anything (even in commit messages) that assumes that IPsec
> > > > > offload doesn't exist.
> > > > 
> > > > I don't understand what you're saying here. Patch #1 from this series
> > > > is exactly about the macsec device acknowledging that ipsec offload
> > > > exists. The rest of the patches is strictly macsec stuff and says
> > > > nothing about ipsec. Can you point out where, in this series, I'm
> > > > claiming that ipsec offload doesn't exist?
> > > 
> > > All this conversation is about one sentence, which I cited above - "it's not possible
> > > to offload macsec to lower devices that also support ipsec offload". From the comments,
> > > I think that you wanted to say "macsec offload is not working due to performance
> > > optimization, where IPsec offload feature flag was exposed from lower device." Did I get
> > > it correctly, now?
> > 
> > Yes. "In the current state" (that I wrote in front of the sentence you
> > quoted) refers to the changes introduced by commit c850240b6c41. The
> > details are present in the commit message for patch 1.
> > 
> > Do you object to the revert, if I rephrase the justification, and then
> > re-add the features that make sense in net-next?
> 
> I don't have any objections.

Would this be ok for the cover letter?

    ----
    The first patch is a revert of commit c850240b6c41 ("net: macsec:
    report real_dev features when HW offloading is enabled"). That
    commit tried to improve the performance of macsec offload by
    taking advantage of some of the NIC's features, but in doing so,
    broke macsec offload when the lower device supports both macsec
    and ipsec offload, as the ipsec offload feature flags were copied
    from the real device. Since the macsec device doesn't provide
    xdo_* ops, the XFRM core rejects the registration of the new
    macsec device in xfrm_api_check.

    I'm working on re-adding those feature flags when offload is
    available, but I haven't fully solved that yet. I think it would
    be safer to do that second part in net-next considering how
    complex feature interactions tend to be.
    ----

Do you want something added to the commit message of the revert as
well?

Thanks.

-- 
Sabrina