Re: [PATCH net-next 02/10] net: microchip: sparx5: Clear rule counter even if lookup is disabled

[Steen Hegelund <steen.hegelund@microchip.com>]

Hi Dan,

On Mon, 2023-02-13 at 18:06 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Mon, Feb 13, 2023 at 01:44:35PM +0100, Steen Hegelund wrote:
> > > > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > > b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > > index b2753aac8ad2..0a1d4d740567 100644
> > > > --- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > > @@ -1337,8 +1337,8 @@ static void vcap_api_encode_rule_test(struct kunit
> > > > *test)
> > > >       u32 port_mask_rng_mask = 0x0f;
> > > >       u32 igr_port_mask_value = 0xffabcd01;
> > > >       u32 igr_port_mask_mask = ~0;
> > > > -     /* counter is written as the last operation */
> > > > -     u32 expwriteaddr[] = {792, 793, 794, 795, 796, 797, 792};
> > > > +     /* counter is written as the first operation */
> > > > +     u32 expwriteaddr[] = {792, 792, 793, 794, 795, 796, 797};
> > > 
> > > So this moves 792 from the last to the first.  I would have expected
> > > that that would mean that we had to do something like this as well:
> > > 
> > > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > index b2753aac8ad2..4d36fad0acab 100644
> > > --- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > > @@ -1400,7 +1400,7 @@ static void vcap_api_encode_rule_test(struct kunit
> > > *test)
> > >         /* Add rule with write callback */
> > >         ret = vcap_add_rule(rule);
> > >         KUNIT_EXPECT_EQ(test, 0, ret);
> > > -       KUNIT_EXPECT_EQ(test, 792, is2_admin.last_used_addr);
> > > +       KUNIT_EXPECT_EQ(test, 797, is2_admin.last_used_addr);
> > >         for (idx = 0; idx < ARRAY_SIZE(expwriteaddr); ++idx)
> > >                 KUNIT_EXPECT_EQ(test, expwriteaddr[idx],
> > > test_updateaddr[idx]);
> > > 
> > > 
> > > But I couldn't really figure out how the .last_used_addr stuff works.
> > > And presumably fixing this unit test is the point of the patch...
> > 
> > It is just the array of addresses written to in the order that they are
> > written,
> > so for the visibility I would like to keep it as an array.
> > 
> 
> My question was likely noise to begin with, but it's not clear that I
> phrased it well.  I'm asking that since 797 is now the last element in
> the array, I expected that the KUNIT_EXPECT_EQ() test for last_used_addr
> would have to be changed to 797 as well.

There are two writes to the 792 address as the counter recides with the start of
the rule (the lowest address of the rule).  Instead of being written after the
rule, it is now being written before the rule, so the test array that records
the order of the write operations gets changed.

The is2_admin.last_used_addr on the other hand records the "low water mark" of
used addresses in the VCAP instance, so it does not change as the rule size is
the same.

> 
> regards,
> dan carpenter
> 

BR
Steen