diff mbox series

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

Message ID 20230213092426.1331379-3-steen.hegelund@microchip.com
State New
Headers
Series Adding Sparx5 ES0 VCAP support |

Commit Message

Steen Hegelund Feb. 13, 2023, 9:24 a.m. UTC 
  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>
---
 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(-)

Comments

Dan Carpenter Feb. 13, 2023, 11:29 a.m. UTC | #1 
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?

> ---
>  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...

regards,
dan carpenter
Steen Hegelund Feb. 13, 2023, 12:44 p.m. UTC | #2 
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
Dan Carpenter Feb. 13, 2023, 3:06 p.m. UTC | #3 
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.

regards,
dan carpenter
Steen Hegelund Feb. 13, 2023, 3:32 p.m. UTC | #4 
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
Dan Carpenter Feb. 13, 2023, 3:38 p.m. UTC | #5 
On Mon, Feb 13, 2023 at 04:32:21PM +0100, Steen Hegelund wrote:
> 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.

That explains it.  Thanks!

regards,
dan carpenter
diff mbox series

Patch

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};
 	int idx;
 
 	vcap_test_api_init(&is2_admin);