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 14:29 +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 10:24:18AM +0100, Steen Hegelund wrote:
> > The rule counter must be cleared when creating a new rule, even if the VCAP
> > lookup is currently disabled.
> > 
> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> 
> Is this a bugfix?  If so what are the user visible effects of this bug
> and please add a Fixes tag.  If not then could you explain more what
> this patch is for?

Yes this is a bugfix of a side effect introduced by my mid-January series "Add
support for two classes of VCAP rules" where this counter change should have
been added too.

The counter problem is only present on VCAP that has external counters, so it
only affects the IS2 and ES0 VCAP on Sparx5 and none of the LAN966x VCAPs.

I will add a Fixes tag to the next series.

> 
> > ---
> >  drivers/net/ethernet/microchip/vcap/vcap_api.c       | 7 +++++--
> >  drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c | 4 ++--
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > index 6307d59f23da..68e04d47f6fd 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > @@ -2246,6 +2246,11 @@ int vcap_add_rule(struct vcap_rule *rule)
> >       if (move.count > 0)
> >               vcap_move_rules(ri, &move);
> > 
> > +     /* Set the counter to zero */
> > +     ret = vcap_write_counter(ri, &ctr);
> > +     if (ret)
> > +             goto out;
> > +
> >       if (ri->state == VCAP_RS_DISABLED) {
> >               /* Erase the rule area */
> >               ri->vctrl->ops->init(ri->ndev, ri->admin, ri->addr, ri->size);
> > @@ -2264,8 +2269,6 @@ int vcap_add_rule(struct vcap_rule *rule)
> >               pr_err("%s:%d: rule write error: %d\n", __func__, __LINE__,
> > ret);
> >               goto out;
> >       }
> > -     /* Set the counter to zero */
> > -     ret = vcap_write_counter(ri, &ctr);
> >  out:
> >       mutex_unlock(&ri->admin->lock);
> >       return ret;
> > 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.

> 
> regards,
> dan carpenter

Thanks for the comments!

BR
Steen