RE: [PATCH net-next V2 07/17] net/mlx5: Add MACsec offload Tx command support

From: Raed Salem <raeds@nvidia.com>
To: sundeep subbaraya <sundeep.lkml@gmail.com>,
	Antoine Tenart <atenart@kernel.org>
Cc: Saeed Mahameed <saeedm@nvidia.com>,
	Saeed Mahameed <saeed@kernel.org>,
	Lior Nahmanson <liorna@nvidia.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Tariq Toukan <tariqt@nvidia.com>,
	Subbaraya Sundeep <sbhatta@marvell.com>,
	"naveenm@marvell.com" <naveenm@marvell.com>,
	Sunil Kovvuri Goutham <sgoutham@marvell.com>,
	Geetha sowjanya <gakula@marvell.com>,
	"andrew@lunn.ch" <andrew@lunn.ch>
Subject: RE: [PATCH net-next V2 07/17] net/mlx5: Add MACsec offload Tx command support
Date: Mon, 19 Sep 2022 13:26:26 +0000	[thread overview]
Message-ID: <DM4PR12MB53579A35887680D5211AC282C94D9@DM4PR12MB5357.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CALHRZurLscR15y48fzJXC4pAWe+wen8JZVCwk2fwT4wujqSdRQ@mail.gmail.com>


>-----Original Message-----
>From: sundeep subbaraya <sundeep.lkml@gmail.com>
>Sent: Monday, 19 September 2022 12:02
>To: Antoine Tenart <atenart@kernel.org>
>Cc: Saeed Mahameed <saeedm@nvidia.com>; Saeed Mahameed
><saeed@kernel.org>; Lior Nahmanson <liorna@nvidia.com>; David S. Miller
><davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
><pabeni@redhat.com>; Eric Dumazet <edumazet@google.com>;
>netdev@vger.kernel.org; Tariq Toukan <tariqt@nvidia.com>; Raed Salem
><raeds@nvidia.com>; Subbaraya Sundeep <sbhatta@marvell.com>;
>naveenm@marvell.com; Sunil Kovvuri Goutham <sgoutham@marvell.com>;
>Geetha sowjanya <gakula@marvell.com>; andrew@lunn.ch
>Subject: Re: [PATCH net-next V2 07/17] net/mlx5: Add MACsec offload Tx
>command support
>
>External email: Use caution opening links or attachments
>
>
>On Thu, Sep 15, 2022 at 1:32 PM Antoine Tenart <atenart@kernel.org> wrote:
>>
>> Quoting sundeep subbaraya (2022-09-15 07:20:05)
>> > On Thu, Sep 15, 2022 at 10:44 AM sundeep subbaraya
>> > <sundeep.lkml@gmail.com> wrote:
>> > > On Thu, Sep 15, 2022 at 2:08 AM Saeed Mahameed
><saeedm@nvidia.com> wrote:
>> > > > On 14 Sep 20:09, sundeep subbaraya wrote:
>> > > > >Hi Saeed and Lior,
>> > > > >
>> > > > >Your mdo_ops can fail in the commit phase and do not validate
>> > > > >input params in the prepare phase.
>> > > > >Is that okay? I am developing MACSEC offload driver for Marvell
>> > > > >CN10K
>> > > >
>> > > > It's ok since i think there is no reason to have the two steps
>> > > > system ! it doesn't make any sense to me ! prepare and commit
>> > > > are invoked consecutively one after the other for all mdo_ops
>> > > > and in every offload flow, with no extra step in between! so it's totally
>redundant.
>> > > >
>> > > > when i reviewed the series initially i was hesitant to check
>> > > > params on prepare step but i didn't see any reason since commit
>> > > > can still fail in the firmware anyways and there is nothing we can do
>about it !
>> > >
>> > > Yes, same with us where messages sent to the AF driver can fail in
>> > > the commit phase.
>> > >
>> > > > so we've decide to keep all the flows in one context for better
>> > > > readability and since the prepare/commit phases are confusing.
>>
>> > > > >and I had to write some clever code to honour that :). Please
>> > > > >someone help me understand why two phase init was needed for
>> > > > >offloading.
>> > > >
>> > > > I don't know, let's ask the original author, Antoine ?
>>
>> This two steps configuration wasn't part of the initial RFC and there
>> was a suggestion to go this way as it could allow the hardware to
>> reject some configurations and have an easier s/w fallback (w/ phase 1
>> error being ignored but not phase 2). This mapped ~quite well to the
>> first device supporting this so I tried it. But looking back, this
>> wasn't discussed anymore nor improved and stayed this way. As you can
>> see the offloading doesn't fallback to s/w currently and I'd say if we
>> want that we should discuss it first; not sure if that is wanted after all.
>>
>I could not think of advantages we have with two phase init for software
>fallback.
>As of now we will send the new driver to do all the init in the prepare phase
>and commit phase will return 0 always.
>
>Thanks,
>Sundeep
I think it is better to do all the init in commit phase and not in the prepare to align with
most drivers that already implemented macsec offload (both aquantia and mlx5 and most of mscc implementation),
this will make it easier to deprecate the prepare stage in future refactor of the macsec driver in stack.

Thanks,
Raed
>
>
>> If in the end all drivers ignore the first phase, or can't do much,
>> it's probably an indication the pattern doesn't fit well. We can still
>> change this, especially considering there are not that many drivers
>> implementing MACsec h/w offload for now. Now is a good time to discuss
>> this, thanks for raising that point.
>>
>> [Adding Andrew who IIRC looked at the initial RFC; in case he wants to
>> add something].
>>
>> Thanks,
>> Antoine