* [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() @ 2021-03-06 13:47 Jia-Ju Bai 2021-03-09 8:20 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Jia-Ju Bai @ 2021-03-06 13:47 UTC (permalink / raw) To: saeedm, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel, Jia-Ju Bai When mlx5e_tc_get_counter() returns NULL to counter or mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return code of mlx5e_stats_flower() is assigned. To fix this bug, err is assigned with -EINVAL in these cases. Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 0da69b98f38f..1f2c9da7bd35 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { counter = mlx5e_tc_get_counter(flow); - if (!counter) + if (!counter) { + err = -EINVAL; goto errout; + } mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); } @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, * un-offloaded while the other rule is offloaded. */ peer_esw = mlx5_devcom_get_peer_data(devcom, MLX5_DEVCOM_ESW_OFFLOADS); - if (!peer_esw) + if (!peer_esw) { + err = -EINVAL; goto out; + } if (flow_flag_test(flow, DUP) && flow_flag_test(flow->peer_flow, OFFLOADED)) { @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, u64 lastuse2; counter = mlx5e_tc_get_counter(flow->peer_flow); - if (!counter) + if (!counter) { + err = -EINVAL; goto no_peer_counter; + } mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); bytes += bytes2; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-06 13:47 [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() Jia-Ju Bai @ 2021-03-09 8:20 ` Roi Dayan 2021-03-09 8:24 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2021-03-09 8:20 UTC (permalink / raw) To: Jia-Ju Bai, saeedm, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: > When mlx5e_tc_get_counter() returns NULL to counter or > mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return > code of mlx5e_stats_flower() is assigned. > To fix this bug, err is assigned with -EINVAL in these cases. > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 0da69b98f38f..1f2c9da7bd35 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > > if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { > counter = mlx5e_tc_get_counter(flow); > - if (!counter) > + if (!counter) { > + err = -EINVAL; > goto errout; > + } > > mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); > } > @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > * un-offloaded while the other rule is offloaded. > */ > peer_esw = mlx5_devcom_get_peer_data(devcom, MLX5_DEVCOM_ESW_OFFLOADS); > - if (!peer_esw) > + if (!peer_esw) { > + err = -EINVAL; note here it's not an error. it could be there is no peer esw so just continue with the stats update. > goto out; > + } > > if (flow_flag_test(flow, DUP) && > flow_flag_test(flow->peer_flow, OFFLOADED)) { > @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > u64 lastuse2; > > counter = mlx5e_tc_get_counter(flow->peer_flow); > - if (!counter) > + if (!counter) { > + err = -EINVAL; > goto no_peer_counter; > + } > mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); > > bytes += bytes2; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-09 8:20 ` Roi Dayan @ 2021-03-09 8:24 ` Roi Dayan 2021-03-09 8:32 ` Jia-Ju Bai 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2021-03-09 8:24 UTC (permalink / raw) To: Jia-Ju Bai, saeedm, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel On 2021-03-09 10:20 AM, Roi Dayan wrote: > > > On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >> When mlx5e_tc_get_counter() returns NULL to counter or >> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >> code of mlx5e_stats_flower() is assigned. >> To fix this bug, err is assigned with -EINVAL in these cases. >> >> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> --- >> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> index 0da69b98f38f..1f2c9da7bd35 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >> counter = mlx5e_tc_get_counter(flow); >> - if (!counter) >> + if (!counter) { >> + err = -EINVAL; >> goto errout; >> + } >> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >> } >> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> * un-offloaded while the other rule is offloaded. >> */ >> peer_esw = mlx5_devcom_get_peer_data(devcom, >> MLX5_DEVCOM_ESW_OFFLOADS); >> - if (!peer_esw) >> + if (!peer_esw) { >> + err = -EINVAL; > > note here it's not an error. it could be there is no peer esw > so just continue with the stats update. > >> goto out; >> + } >> if (flow_flag_test(flow, DUP) && >> flow_flag_test(flow->peer_flow, OFFLOADED)) { >> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> u64 lastuse2; >> counter = mlx5e_tc_get_counter(flow->peer_flow); >> - if (!counter) >> + if (!counter) { >> + err = -EINVAL; this change is problematic. the current goto is to do stats update with the first counter stats we got but if you now want to return an error then you probably should not do any update at all. >> goto no_peer_counter; >> + } >> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >> bytes += bytes2; >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-09 8:24 ` Roi Dayan @ 2021-03-09 8:32 ` Jia-Ju Bai 2021-03-09 9:44 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Jia-Ju Bai @ 2021-03-09 8:32 UTC (permalink / raw) To: Roi Dayan, saeedm, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel On 2021/3/9 16:24, Roi Dayan wrote: > > > On 2021-03-09 10:20 AM, Roi Dayan wrote: >> >> >> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>> When mlx5e_tc_get_counter() returns NULL to counter or >>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >>> code of mlx5e_stats_flower() is assigned. >>> To fix this bug, err is assigned with -EINVAL in these cases. >>> >>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>> --- >>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> index 0da69b98f38f..1f2c9da7bd35 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >>> counter = mlx5e_tc_get_counter(flow); >>> - if (!counter) >>> + if (!counter) { >>> + err = -EINVAL; >>> goto errout; >>> + } >>> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >>> } >>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> * un-offloaded while the other rule is offloaded. >>> */ >>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>> MLX5_DEVCOM_ESW_OFFLOADS); >>> - if (!peer_esw) >>> + if (!peer_esw) { >>> + err = -EINVAL; >> >> note here it's not an error. it could be there is no peer esw >> so just continue with the stats update. >> >>> goto out; >>> + } >>> if (flow_flag_test(flow, DUP) && >>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> u64 lastuse2; >>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>> - if (!counter) >>> + if (!counter) { >>> + err = -EINVAL; > > this change is problematic. the current goto is to do stats update with > the first counter stats we got but if you now want to return an error > then you probably should not do any update at all. Thanks for your reply :) I am not sure whether an error code should be returned here? If so, flow_stats_update(...) should not be called here? Best wishes, Jia-Ju Bai > >>> goto no_peer_counter; >>> + } >>> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >>> bytes += bytes2; >>> >> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-09 8:32 ` Jia-Ju Bai @ 2021-03-09 9:44 ` Roi Dayan 2021-03-11 22:47 ` Saeed Mahameed 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2021-03-09 9:44 UTC (permalink / raw) To: Jia-Ju Bai, saeedm, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: > > > On 2021/3/9 16:24, Roi Dayan wrote: >> >> >> On 2021-03-09 10:20 AM, Roi Dayan wrote: >>> >>> >>> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>>> When mlx5e_tc_get_counter() returns NULL to counter or >>>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >>>> code of mlx5e_stats_flower() is assigned. >>>> To fix this bug, err is assigned with -EINVAL in these cases. >>>> >>>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>>> --- >>>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> index 0da69b98f38f..1f2c9da7bd35 100644 >>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >>>> counter = mlx5e_tc_get_counter(flow); >>>> - if (!counter) >>>> + if (!counter) { >>>> + err = -EINVAL; >>>> goto errout; >>>> + } >>>> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >>>> } >>>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> * un-offloaded while the other rule is offloaded. >>>> */ >>>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>>> MLX5_DEVCOM_ESW_OFFLOADS); >>>> - if (!peer_esw) >>>> + if (!peer_esw) { >>>> + err = -EINVAL; >>> >>> note here it's not an error. it could be there is no peer esw >>> so just continue with the stats update. >>> >>>> goto out; >>>> + } >>>> if (flow_flag_test(flow, DUP) && >>>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> u64 lastuse2; >>>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>>> - if (!counter) >>>> + if (!counter) { >>>> + err = -EINVAL; >> >> this change is problematic. the current goto is to do stats update with >> the first counter stats we got but if you now want to return an error >> then you probably should not do any update at all. > > Thanks for your reply :) > I am not sure whether an error code should be returned here? > If so, flow_stats_update(...) should not be called here? > > > Best wishes, > Jia-Ju Bai > basically flow and peer_flow should be valid and protected from changes, and counter should be valid. it looks like the check here is more of a sanity check if something goes wrong but shouldn't. you can just let it be, update the stats from the first queried counter. >> >>>> goto no_peer_counter; >>>> + } >>>> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >>>> bytes += bytes2; >>>> >>> >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-09 9:44 ` Roi Dayan @ 2021-03-11 22:47 ` Saeed Mahameed 2021-03-14 9:50 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Saeed Mahameed @ 2021-03-11 22:47 UTC (permalink / raw) To: Roi Dayan, Jia-Ju Bai, leon, davem, kuba; +Cc: netdev, linux-rdma, linux-kernel On Tue, 2021-03-09 at 11:44 +0200, Roi Dayan wrote: > > > On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: > > > > > > On 2021/3/9 16:24, Roi Dayan wrote: > > > > > > > > > On 2021-03-09 10:20 AM, Roi Dayan wrote: > > > > > > > > > > > > On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: > > > > > When mlx5e_tc_get_counter() returns NULL to counter or > > > > > mlx5_devcom_get_peer_data() returns NULL to peer_esw, no > > > > > error return > > > > > code of mlx5e_stats_flower() is assigned. > > > > > To fix this bug, err is assigned with -EINVAL in these cases. > > > > > > > > > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Hey Jia-Ju, What are the conditions for this robot to raise a flag? sometimes it is totally normal to abort a function and return 0.. i am just curious to know ? > > > > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > > > > > --- > > > > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 > > > > > +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > index 0da69b98f38f..1f2c9da7bd35 100644 > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > if (mlx5e_is_offloaded_flow(flow) || > > > > > flow_flag_test(flow, CT)) { > > > > > counter = mlx5e_tc_get_counter(flow); > > > > > - if (!counter) > > > > > + if (!counter) { > > > > > + err = -EINVAL; > > > > > goto errout; > > > > > + } > > > > > mlx5_fc_query_cached(counter, &bytes, &packets, > > > > > &lastuse); > > > > > } > > > > > @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > * un-offloaded while the other rule is offloaded. > > > > > */ > > > > > peer_esw = mlx5_devcom_get_peer_data(devcom, > > > > > MLX5_DEVCOM_ESW_OFFLOADS); > > > > > - if (!peer_esw) > > > > > + if (!peer_esw) { > > > > > + err = -EINVAL; > > > > This is not an error flow, i am curious what are the thresholds of this robot ? > > > > note here it's not an error. it could be there is no peer esw > > > > so just continue with the stats update. > > > > > > > > > goto out; > > > > > + } > > > > > if (flow_flag_test(flow, DUP) && > > > > > flow_flag_test(flow->peer_flow, OFFLOADED)) { > > > > > @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > u64 lastuse2; > > > > > counter = mlx5e_tc_get_counter(flow->peer_flow); > > > > > - if (!counter) > > > > > + if (!counter) { > > > > > + err = -EINVAL; > > > > > > this change is problematic. the current goto is to do stats > > > update with > > > the first counter stats we got but if you now want to return an > > > error > > > then you probably should not do any update at all. > > > > Thanks for your reply :) > > I am not sure whether an error code should be returned here? > > If so, flow_stats_update(...) should not be called here? > > > > > > Best wishes, > > Jia-Ju Bai > > > > basically flow and peer_flow should be valid and protected from > changes, > and counter should be valid. > it looks like the check here is more of a sanity check if something > goes > wrong but shouldn't. you can just let it be, update the stats from > the > first queried counter. > Roi, let's consider returning an error code here, we shouldn't be silently returning if we are not expecting these errors, why would mlx5e_stats_flower() be called if stats are not offloaded ? Thanks, Saeed. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() 2021-03-11 22:47 ` Saeed Mahameed @ 2021-03-14 9:50 ` Roi Dayan 0 siblings, 0 replies; 7+ messages in thread From: Roi Dayan @ 2021-03-14 9:50 UTC (permalink / raw) To: Saeed Mahameed, Jia-Ju Bai, leon, davem, kuba Cc: netdev, linux-rdma, linux-kernel On 2021-03-12 12:47 AM, Saeed Mahameed wrote: > On Tue, 2021-03-09 at 11:44 +0200, Roi Dayan wrote: >> >> >> On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: >>> >>> >>> On 2021/3/9 16:24, Roi Dayan wrote: >>>> >>>> >>>> On 2021-03-09 10:20 AM, Roi Dayan wrote: >>>>> >>>>> >>>>> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>>>>> When mlx5e_tc_get_counter() returns NULL to counter or >>>>>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no >>>>>> error return >>>>>> code of mlx5e_stats_flower() is assigned. >>>>>> To fix this bug, err is assigned with -EINVAL in these cases. >>>>>> >>>>>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > > Hey Jia-Ju, What are the conditions for this robot to raise a flag? > sometimes it is totally normal to abort a function and return 0.. i am > just curious to know ? > > >>>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>>>>> --- >>>>>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 >>>>>> +++++++++--- >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> index 0da69b98f38f..1f2c9da7bd35 100644 >>>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> if (mlx5e_is_offloaded_flow(flow) || >>>>>> flow_flag_test(flow, CT)) { >>>>>> counter = mlx5e_tc_get_counter(flow); >>>>>> - if (!counter) >>>>>> + if (!counter) { >>>>>> + err = -EINVAL; >>>>>> goto errout; >>>>>> + } >>>>>> mlx5_fc_query_cached(counter, &bytes, &packets, >>>>>> &lastuse); >>>>>> } >>>>>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> * un-offloaded while the other rule is offloaded. >>>>>> */ >>>>>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>>>>> MLX5_DEVCOM_ESW_OFFLOADS); >>>>>> - if (!peer_esw) >>>>>> + if (!peer_esw) { >>>>>> + err = -EINVAL; >>>>> > > This is not an error flow, i am curious what are the thresholds of this > robot ? > >>>>> note here it's not an error. it could be there is no peer esw >>>>> so just continue with the stats update. >>>>> >>>>>> goto out; >>>>>> + } >>>>>> if (flow_flag_test(flow, DUP) && >>>>>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>>>>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> u64 lastuse2; >>>>>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>>>>> - if (!counter) >>>>>> + if (!counter) { >>>>>> + err = -EINVAL; >>>> >>>> this change is problematic. the current goto is to do stats >>>> update with >>>> the first counter stats we got but if you now want to return an >>>> error >>>> then you probably should not do any update at all. >>> >>> Thanks for your reply :) >>> I am not sure whether an error code should be returned here? >>> If so, flow_stats_update(...) should not be called here? >>> >>> >>> Best wishes, >>> Jia-Ju Bai >>> >> >> basically flow and peer_flow should be valid and protected from >> changes, >> and counter should be valid. >> it looks like the check here is more of a sanity check if something >> goes >> wrong but shouldn't. you can just let it be, update the stats from >> the >> first queried counter. >> > > Roi, let's consider returning an error code here, we shouldn't be > silently returning if we are not expecting these errors, > > why would mlx5e_stats_flower() be called if stats are not offloaded ? > > Thanks, > Saeed. > > yes we can return an error if peer counter missing. I just pointed out we should probably not call flow_stats_update() if we do return an error. the other option, as today, is updating the stats with first counter stats and because of that we didn't return an error. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-14 9:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-03-06 13:47 [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() Jia-Ju Bai 2021-03-09 8:20 ` Roi Dayan 2021-03-09 8:24 ` Roi Dayan 2021-03-09 8:32 ` Jia-Ju Bai 2021-03-09 9:44 ` Roi Dayan 2021-03-11 22:47 ` Saeed Mahameed 2021-03-14 9:50 ` Roi Dayan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox