[net-next,v2,0/8] Add support for two classes of VCAP rules

  This adds support for two classes of VCAP rules:

- Permanent rules (added e.g. for PTP support)
- TC user rules (added by the TC userspace tool)

For this to work the VCAP Loopups must be enabled from boot, so that the
"internal" clients like PTP can add rules that are always active.

When the TC tool add a flower filter the VCAP rule corresponding to this
filter will be disabled (kept in memory) until a TC matchall filter creates
a link from chain 0 to the chain (lookup) where the flower filter was
added.

When the flower filter is enabled it will be written to the appropriate
VCAP lookup and become active in HW.

Likewise the flower filter will be disabled if there is no link from chain
0 to the chain of the filter (lookup), and when that happens the
corresponding VCAP rule will be read from the VCAP instance and stored in
memory until it is deleted or enabled again.

Version History:
================
v2      Adding a missing goto exit in vcap_add_rule (Dan Carpenter).
        Added missing checks for error returns in vcap_enable_rule.

v1      Initial version

Steen Hegelund (8):
  net: microchip: vcap api: Erase VCAP cache before encoding rule
  net: microchip: sparx5: Reset VCAP counter for new rules
  net: microchip: vcap api: Always enable VCAP lookups
  net: microchip: vcap api: Convert multi-word keys/actions when
    encoding
  net: microchip: vcap api: Use src and dst chain id to chain VCAP
    lookups
  net: microchip: vcap api: Check chains when adding a tc flower filter
  net: microchip: vcap api: Add a storage state to a VCAP rule
  net: microchip: vcap api: Enable/Disable rules via chains in VCAP HW

 .../ethernet/microchip/lan966x/lan966x_goto.c |  10 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |   3 +-
 .../microchip/lan966x/lan966x_tc_flower.c     |  30 +-
 .../microchip/lan966x/lan966x_tc_matchall.c   |  16 +-
 .../microchip/lan966x/lan966x_vcap_impl.c     |  24 +-
 .../microchip/sparx5/sparx5_tc_flower.c       |  28 +-
 .../microchip/sparx5/sparx5_tc_matchall.c     |  16 +-
 .../microchip/sparx5/sparx5_vcap_debugfs.c    |   2 +-
 .../microchip/sparx5/sparx5_vcap_impl.c       |  29 +-
 .../net/ethernet/microchip/vcap/vcap_api.c    | 762 +++++++++++++-----
 .../net/ethernet/microchip/vcap/vcap_api.h    |   5 -
 .../ethernet/microchip/vcap/vcap_api_client.h |   8 +-
 .../microchip/vcap/vcap_api_debugfs.c         |  57 +-
 .../microchip/vcap/vcap_api_debugfs_kunit.c   |  10 +-
 .../ethernet/microchip/vcap/vcap_api_kunit.c  |  32 +-
 .../microchip/vcap/vcap_api_private.h         |  12 +-
 16 files changed, 694 insertions(+), 350 deletions(-)
  

On Fri, Jan 06, 2023 at 09:53:09AM +0100, Steen Hegelund wrote:
> Version History:
> ================
> v2      Adding a missing goto exit in vcap_add_rule (Dan Carpenter).

Thanks!

regards,
dan carpenter
  

Hi Steen,

thanks for adding me on CC :) I was just about to reply on your v1.

Am 2023-01-06 09:53, schrieb Steen Hegelund:
> This adds support for two classes of VCAP rules:
> 
> - Permanent rules (added e.g. for PTP support)
> - TC user rules (added by the TC userspace tool)
> 
> For this to work the VCAP Loopups must be enabled from boot, so that 
> the
> "internal" clients like PTP can add rules that are always active.
> 
> When the TC tool add a flower filter the VCAP rule corresponding to 
> this
> filter will be disabled (kept in memory) until a TC matchall filter 
> creates
> a link from chain 0 to the chain (lookup) where the flower filter was
> added.
> 
> When the flower filter is enabled it will be written to the appropriate
> VCAP lookup and become active in HW.
> 
> Likewise the flower filter will be disabled if there is no link from 
> chain
> 0 to the chain of the filter (lookup), and when that happens the
> corresponding VCAP rule will be read from the VCAP instance and stored 
> in
> memory until it is deleted or enabled again.

I've just done a very quick smoke test and looked at my lan9668 board
that the following error isn't printed anymore. No functional testing.
   vcap_val_rule:1678: keyset was not updated: -22

And it is indeed gone. But I have a few questions regarding how these
patches are applied. They were first sent for net, but now due to
a remark that they are too invasive they are targeted at net-next.
But they have a Fixes: tag. Won't they be eventually backported to
later kernels in any case? What's the difference between net and
net-next then?

Also patches 3-8 (the one with the fixes tags) don't apply without
patch 1-2 (which don't have fixes tags). IMHO they should be
reordered.

Wouldn't it make more sense, to fix the regression via net (and
a Fixes: tag) and then make that stuff work without tc? Maybe
the fix is just reverting the commits.

-michael
  

Hi Michael,


On Fri, 2023-01-06 at 10:07 +0100, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Steen,
> 
> thanks for adding me on CC :) I was just about to reply on your v1.
> 
> Am 2023-01-06 09:53, schrieb Steen Hegelund:
> > This adds support for two classes of VCAP rules:
> > 
> > - Permanent rules (added e.g. for PTP support)
> > - TC user rules (added by the TC userspace tool)
> > 
> > For this to work the VCAP Loopups must be enabled from boot, so that
> > the
> > "internal" clients like PTP can add rules that are always active.
> > 
> > When the TC tool add a flower filter the VCAP rule corresponding to
> > this
> > filter will be disabled (kept in memory) until a TC matchall filter
> > creates
> > a link from chain 0 to the chain (lookup) where the flower filter was
> > added.
> > 
> > When the flower filter is enabled it will be written to the appropriate
> > VCAP lookup and become active in HW.
> > 
> > Likewise the flower filter will be disabled if there is no link from
> > chain
> > 0 to the chain of the filter (lookup), and when that happens the
> > corresponding VCAP rule will be read from the VCAP instance and stored
> > in
> > memory until it is deleted or enabled again.
> 
> I've just done a very quick smoke test and looked at my lan9668 board
> that the following error isn't printed anymore. No functional testing.
>    vcap_val_rule:1678: keyset was not updated: -22

Good to hear.

> 
> And it is indeed gone. But I have a few questions regarding how these
> patches are applied. They were first sent for net, but now due to
> a remark that they are too invasive they are targeted at net-next.
> But they have a Fixes: tag. Won't they be eventually backported to
> later kernels in any case? What's the difference between net and
> net-next then?

I am not sure I can answer that.

> 
> Also patches 3-8 (the one with the fixes tags) don't apply without
> patch 1-2 (which don't have fixes tags). IMHO they should be
> reordered.

Right.

> 
> Wouldn't it make more sense, to fix the regression via net (and
> a Fixes: tag) and then make that stuff work without tc? Maybe
> the fix is just reverting the commits.

I have discussed this again with Horatiu and I have the following suggestion of
how to proceed:

1) Create a small LAN966x specific patch for net (see below for the two possible
   variants).

2) Continue with a net-next V3 without any 'Fixes' tags on top of the patch in
   (1) when it becomes available in net-next.


The LAN966x patch for net (with a Fixes tag) could contain either:

a) No check on enabled lookup

   Removal of the check for enabled lookups:
   
   -	if (!ANA_VCAP_S2_CFG_ENA_GET(val))
   -		return -ENOENT;
   
   This will remove the error that you have seen, but  will still require a
   matchall rule to enable the PTP rules.  This is compatible with the TC
   framework.
   
b) Always enable lookups

   Enable the lookups at startup.
   Remove the lookup enable check as above.
   
   This will make the PTP rules (and any other rules) work even without the
   matchall rule to enable them.  It its not ideal, but solves the problem that
   you have been experiencing without the 'TC magic'
   
   The V3 in net-next will provide the full solution.

I expect that you might prefer the b) version.

> 
> -michael

BR
Steen
  

Hi,

>> Wouldn't it make more sense, to fix the regression via net (and
>> a Fixes: tag) and then make that stuff work without tc? Maybe
>> the fix is just reverting the commits.
> 
> I have discussed this again with Horatiu and I have the following 
> suggestion of
> how to proceed:
> 
> 1) Create a small LAN966x specific patch for net (see below for the two 
> possible
>    variants).
> 
> 2) Continue with a net-next V3 without any 'Fixes' tags on top of the 
> patch in
>    (1) when it becomes available in net-next.

Sounds good.

[coming back to this after writing the response below, so see there
for more context]
When do the patches from net become available in net-next? Only after a
merge window? If so, depending on the solution for (1) you'd have two
"in-between" kernel versions (v6.2 and v6.3).

> The LAN966x patch for net (with a Fixes tag) could contain either:
> 
> a) No check on enabled lookup
> 
>    Removal of the check for enabled lookups:
> 
>    -	if (!ANA_VCAP_S2_CFG_ENA_GET(val))
>    -		return -ENOENT;
> 
>    This will remove the error that you have seen, but  will still 
> require a
>    matchall rule to enable the PTP rules.  This is compatible with the 
> TC
>    framework.
> 
> b) Always enable lookups
> 
>    Enable the lookups at startup.
>    Remove the lookup enable check as above.
> 
>    This will make the PTP rules (and any other rules) work even without 
> the
>    matchall rule to enable them.  It its not ideal, but solves the 
> problem that
>    you have been experiencing without the 'TC magic'
> 
>    The V3 in net-next will provide the full solution.
> 
> I expect that you might prefer the b) version.

I *assume* linuxptp would have worked in my case (no bridge interface)
before Horatiu patches. As mentioned before, I haven't really tested it.
Does that mean with a) the error is gone and linuxptp is working as
before? If so, I'm also fine with a).

Honestly, now that there is a good solution in future kernels, I
don't care toooo much about that one particular kernel. Other
users might disagree though ;)

I just want to point out that right now you have some kind of
in-between kernel with 6.2:

  <=6.1 linuxptp working (but not on bridged ports)
  6.2   linuxptp working only with tc magic
  6.3   linuxptp working

Therefore, I've raised the question if it's also viable to just
revert the former changes for 6.2. The you'd have a clean
transition.

-michael
  

Hi Michael,

On Fri, 2023-01-06 at 11:46 +0100, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,
> 
> > > Wouldn't it make more sense, to fix the regression via net (and
> > > a Fixes: tag) and then make that stuff work without tc? Maybe
> > > the fix is just reverting the commits.
> > 
> > I have discussed this again with Horatiu and I have the following
> > suggestion of
> > how to proceed:
> > 
> > 1) Create a small LAN966x specific patch for net (see below for the two
> > possible
> >    variants).
> > 
> > 2) Continue with a net-next V3 without any 'Fixes' tags on top of the
> > patch in
> >    (1) when it becomes available in net-next.
> 
> Sounds good.
> 
> [coming back to this after writing the response below, so see there
> for more context]
> When do the patches from net become available in net-next? Only after a
> merge window? If so, depending on the solution for (1) you'd have two
> "in-between" kernel versions (v6.2 and v6.3).

According to our own experience the changes in net are usually merged into net-
next the following Thursday: so not too much delay, before we can continue.

> 
> > The LAN966x patch for net (with a Fixes tag) could contain either:
> > 
> > a) No check on enabled lookup
> > 
> >    Removal of the check for enabled lookups:
> > 
> >    -  if (!ANA_VCAP_S2_CFG_ENA_GET(val))
> >    -          return -ENOENT;
> > 
> >    This will remove the error that you have seen, but  will still
> > require a
> >    matchall rule to enable the PTP rules.  This is compatible with the
> > TC
> >    framework.
> > 
> > b) Always enable lookups
> > 
> >    Enable the lookups at startup.
> >    Remove the lookup enable check as above.
> > 
> >    This will make the PTP rules (and any other rules) work even without
> > the
> >    matchall rule to enable them.  It its not ideal, but solves the
> > problem that
> >    you have been experiencing without the 'TC magic'
> > 
> >    The V3 in net-next will provide the full solution.
> > 
> > I expect that you might prefer the b) version.
> 
> I *assume* linuxptp would have worked in my case (no bridge interface)
> before Horatiu patches. As mentioned before, I haven't really tested it.
> Does that mean with a) the error is gone and linuxptp is working as
> before? If so, I'm also fine with a).

Yes this is the result: So I also suggest to go for solution a).

This will still allow LinuxPTP to work (without the error that you have seen),
but the bridged interface PTP support must be enabled with a TC matchall rule.

> 
> Honestly, now that there is a good solution in future kernels, I
> don't care toooo much about that one particular kernel. Other
> users might disagree though ;)
> 
> I just want to point out that right now you have some kind of
> in-between kernel with 6.2:
> 
>   <=6.1 linuxptp working (but not on bridged ports)
>   6.2   linuxptp working only with tc magic
>   6.3   linuxptp working

So with the LAN966x patch the second line would change to:

6.2   linuxptp working. PTP on bridged interfaces: needs TC matchall rule

> 
> Therefore, I've raised the question if it's also viable to just
> revert the former changes for 6.2. The you'd have a clean
> transition.
> 
> -michael

TLDR Summary:

1) LAN966x patch for net to ensure PTP is working without errors
2) A V3 net-next VCAP series with the improvements for enabled/disable/permanent
rules (both LAN966x and Sparx5)

I will move forward with this.

BR
Steen
  

Hi Steen,

>> > > Wouldn't it make more sense, to fix the regression via net (and
>> > > a Fixes: tag) and then make that stuff work without tc? Maybe
>> > > the fix is just reverting the commits.
>> >
>> > I have discussed this again with Horatiu and I have the following
>> > suggestion of
>> > how to proceed:
>> >
>> > 1) Create a small LAN966x specific patch for net (see below for the two
>> > possible
>> >    variants).
>> >
>> > 2) Continue with a net-next V3 without any 'Fixes' tags on top of the
>> > patch in
>> >    (1) when it becomes available in net-next.
>> 
>> Sounds good.
>> 
>> [coming back to this after writing the response below, so see there
>> for more context]
>> When do the patches from net become available in net-next? Only after 
>> a
>> merge window? If so, depending on the solution for (1) you'd have two
>> "in-between" kernel versions (v6.2 and v6.3).
> 
> According to our own experience the changes in net are usually merged 
> into net-
> next the following Thursday: so not too much delay, before we can 
> continue.

TIL :)

>> > The LAN966x patch for net (with a Fixes tag) could contain either:
>> >
>> > a) No check on enabled lookup
>> >
>> >    Removal of the check for enabled lookups:
>> >
>> >    -  if (!ANA_VCAP_S2_CFG_ENA_GET(val))
>> >    -          return -ENOENT;
>> >
>> >    This will remove the error that you have seen, but  will still
>> > require a
>> >    matchall rule to enable the PTP rules.  This is compatible with the
>> > TC
>> >    framework.
>> >
>> > b) Always enable lookups
>> >
>> >    Enable the lookups at startup.
>> >    Remove the lookup enable check as above.
>> >
>> >    This will make the PTP rules (and any other rules) work even without
>> > the
>> >    matchall rule to enable them.  It its not ideal, but solves the
>> > problem that
>> >    you have been experiencing without the 'TC magic'
>> >
>> >    The V3 in net-next will provide the full solution.
>> >
>> > I expect that you might prefer the b) version.
>> 
>> I *assume* linuxptp would have worked in my case (no bridge interface)
>> before Horatiu patches. As mentioned before, I haven't really tested 
>> it.
>> Does that mean with a) the error is gone and linuxptp is working as
>> before? If so, I'm also fine with a).
> 
> Yes this is the result: So I also suggest to go for solution a).
> 
> This will still allow LinuxPTP to work (without the error that you have 
> seen),
> but the bridged interface PTP support must be enabled with a TC 
> matchall rule.
> 
>> 
>> Honestly, now that there is a good solution in future kernels, I
>> don't care toooo much about that one particular kernel. Other
>> users might disagree though ;)
>> 
>> I just want to point out that right now you have some kind of
>> in-between kernel with 6.2:
>> 
>>   <=6.1 linuxptp working (but not on bridged ports)
>>   6.2   linuxptp working only with tc magic
>>   6.3   linuxptp working
> 
> So with the LAN966x patch the second line would change to:
> 
> 6.2   linuxptp working. PTP on bridged interfaces: needs TC matchall 
> rule
> 
>> 
>> Therefore, I've raised the question if it's also viable to just
>> revert the former changes for 6.2. The you'd have a clean
>> transition.
>> 
>> -michael
> 
> TLDR Summary:
> 
> 1) LAN966x patch for net to ensure PTP is working without errors
> 2) A V3 net-next VCAP series with the improvements for 
> enabled/disable/permanent
> rules (both LAN966x and Sparx5)
> 
> I will move forward with this.

Sounds perfect, thanks!

-michael