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

[Steen Hegelund]

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