Currently whenever a new rule id is generated, it picks up the next
number bigger than previous id. So it would always be 1, 2, 3, etc.
When the rule with id 1 will be deleted and a new rule will be added,
it will have the id 4 and not id 1.
In theory this can be a problem if at some point a rule will be added
and removed ~0 times. Then no more rules can be added because there
are no more ids.
Change this such that when a new rule is added, search for an empty
rule id starting with value of 1 as value 0 is reserved.
Fixes: c9da1ac1c212 ("net: microchip: sparx5: Adding initial tc flower support for VCAP API")
Signed-off-by: Horatiu Vultur <[email protected]>
---
drivers/net/ethernet/microchip/vcap/vcap_api.c | 7 +------
drivers/net/ethernet/microchip/vcap/vcap_api.h | 1 -
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index b50d002b646dc..b65819f3a927f 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -974,17 +974,12 @@ static u32 vcap_next_rule_addr(u32 addr, struct vcap_rule_internal *ri)
/* Assign a unique rule id and autogenerate one if id == 0 */
static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
{
- u32 next_id;
-
if (ri->data.id != 0)
return ri->data.id;
- next_id = ri->vctrl->rule_id + 1;
-
- for (next_id = ri->vctrl->rule_id + 1; next_id < ~0; ++next_id) {
+ for (u32 next_id = 1; next_id < ~0; ++next_id) {
if (!vcap_lookup_rule(ri->vctrl, next_id)) {
ri->data.id = next_id;
- ri->vctrl->rule_id = next_id;
break;
}
}
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
index ca4499838306f..689c7270f2a89 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
@@ -268,7 +268,6 @@ struct vcap_operations {
/* VCAP API Client control interface */
struct vcap_control {
- u32 rule_id; /* last used rule id (unique across VCAP instances) */
struct vcap_operations *ops; /* client supplied operations */
const struct vcap_info *vcaps; /* client supplied vcap models */
const struct vcap_statistics *stats; /* client supplied vcap stats */
--
2.38.0
From: Horatiu Vultur <[email protected]>
Date: Mon, 28 Nov 2022 15:29:59 +0100
> Currently whenever a new rule id is generated, it picks up the next
> number bigger than previous id. So it would always be 1, 2, 3, etc.
> When the rule with id 1 will be deleted and a new rule will be added,
> it will have the id 4 and not id 1.
> In theory this can be a problem if at some point a rule will be added
> and removed ~0 times. Then no more rules can be added because there
> are no more ids.
>
> Change this such that when a new rule is added, search for an empty
> rule id starting with value of 1 as value 0 is reserved.
>
> Fixes: c9da1ac1c212 ("net: microchip: sparx5: Adding initial tc flower support for VCAP API")
> Signed-off-by: Horatiu Vultur <[email protected]>
> ---
> drivers/net/ethernet/microchip/vcap/vcap_api.c | 7 +------
> drivers/net/ethernet/microchip/vcap/vcap_api.h | 1 -
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index b50d002b646dc..b65819f3a927f 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> @@ -974,17 +974,12 @@ static u32 vcap_next_rule_addr(u32 addr, struct vcap_rule_internal *ri)
> /* Assign a unique rule id and autogenerate one if id == 0 */
> static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
> {
> - u32 next_id;
> -
> if (ri->data.id != 0)
> return ri->data.id;
>
> - next_id = ri->vctrl->rule_id + 1;
> -
> - for (next_id = ri->vctrl->rule_id + 1; next_id < ~0; ++next_id) {
> + for (u32 next_id = 1; next_id < ~0; ++next_id) {
> if (!vcap_lookup_rule(ri->vctrl, next_id)) {
Or you can simply use IDA/IDR/XArray which takes care of all this :)
> ri->data.id = next_id;
> - ri->vctrl->rule_id = next_id;
> break;
> }
> }
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> index ca4499838306f..689c7270f2a89 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> @@ -268,7 +268,6 @@ struct vcap_operations {
>
> /* VCAP API Client control interface */
> struct vcap_control {
> - u32 rule_id; /* last used rule id (unique across VCAP instances) */
> struct vcap_operations *ops; /* client supplied operations */
> const struct vcap_info *vcaps; /* client supplied vcap models */
> const struct vcap_statistics *stats; /* client supplied vcap stats */
> --
> 2.38.0
Thanks,
Olek
The 11/28/2022 15:40, Alexander Lobakin wrote:
Hi Olek,
>
> From: Horatiu Vultur <[email protected]>
> Date: Mon, 28 Nov 2022 15:29:59 +0100
>
> > Currently whenever a new rule id is generated, it picks up the next
> > number bigger than previous id. So it would always be 1, 2, 3, etc.
> > When the rule with id 1 will be deleted and a new rule will be added,
> > it will have the id 4 and not id 1.
> > In theory this can be a problem if at some point a rule will be added
> > and removed ~0 times. Then no more rules can be added because there
> > are no more ids.
> >
> > Change this such that when a new rule is added, search for an empty
> > rule id starting with value of 1 as value 0 is reserved.
> >
> > Fixes: c9da1ac1c212 ("net: microchip: sparx5: Adding initial tc flower support for VCAP API")
> > Signed-off-by: Horatiu Vultur <[email protected]>
> > ---
> > drivers/net/ethernet/microchip/vcap/vcap_api.c | 7 +------
> > drivers/net/ethernet/microchip/vcap/vcap_api.h | 1 -
> > 2 files changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > index b50d002b646dc..b65819f3a927f 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > @@ -974,17 +974,12 @@ static u32 vcap_next_rule_addr(u32 addr, struct vcap_rule_internal *ri)
> > /* Assign a unique rule id and autogenerate one if id == 0 */
> > static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
> > {
> > - u32 next_id;
> > -
> > if (ri->data.id != 0)
> > return ri->data.id;
> >
> > - next_id = ri->vctrl->rule_id + 1;
> > -
> > - for (next_id = ri->vctrl->rule_id + 1; next_id < ~0; ++next_id) {
> > + for (u32 next_id = 1; next_id < ~0; ++next_id) {
> > if (!vcap_lookup_rule(ri->vctrl, next_id)) {
>
> Or you can simply use IDA/IDR/XArray which takes care of all this :)
Thanks for the great suggestion. From what I can see IDR would be a
great match for this. Then we can also change the function
'vcap_lookup_rule' to use IDR.
On the other side, we will not continuously add/remove entries from the
VCAP, so I was not sure if it is worth bringing the IDR(with the radix
tree) for this.
Anyway I am open for IDR suggestion.
But lets wait for the VCAP maintainer to see what he thinks.
>
>
> > ri->data.id = next_id;
> > - ri->vctrl->rule_id = next_id;
> > break;
> > }
> > }
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > index ca4499838306f..689c7270f2a89 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > @@ -268,7 +268,6 @@ struct vcap_operations {
> >
> > /* VCAP API Client control interface */
> > struct vcap_control {
> > - u32 rule_id; /* last used rule id (unique across VCAP instances) */
> > struct vcap_operations *ops; /* client supplied operations */
> > const struct vcap_info *vcaps; /* client supplied vcap models */
> > const struct vcap_statistics *stats; /* client supplied vcap stats */
> > --
> > 2.38.0
>
> Thanks,
> Olek
--
/Horatiu
Hello:
This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
On Mon, 28 Nov 2022 15:29:59 +0100 you wrote:
> Currently whenever a new rule id is generated, it picks up the next
> number bigger than previous id. So it would always be 1, 2, 3, etc.
> When the rule with id 1 will be deleted and a new rule will be added,
> it will have the id 4 and not id 1.
> In theory this can be a problem if at some point a rule will be added
> and removed ~0 times. Then no more rules can be added because there
> are no more ids.
>
> [...]
Here is the summary with links:
- [net-next] net: microchip: vcap: Change how the rule id is generated
https://git.kernel.org/netdev/net-next/c/c1d8e3fb1a3b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html