| Message ID | 20230213092426.1331379-3-steen.hegelund@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Adding Sparx5 ES0 VCAP support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1 this patch: 1 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 29 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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
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
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
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
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 --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);
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(-)