[PATCH 0/3] bugfix for ethdev telemetry

[PATCH 0/3] bugfix for ethdev telemetry

[Chengwen Feng]

This patch set contains three bugfix patch for ethdev telemetry.

Chengwen Feng (3):
  ethdev: fix telemetry xstats return null with some PMDs
  ethdev: fix memory leak when telemetry xstats
  net/cnxk: fix telemetry possible null pointer access

 drivers/net/cnxk/cnxk_ethdev_telemetry.c | 2 ++
 lib/ethdev/rte_ethdev.c                  | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[Chengwen Feng]

Currently the telemetry xstats uses rte_eth_xstats_get() to retrieve
the number of elements. But the value to be returned when the parameter
'xstats' is NULL is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/
axgbe) return zero while others return the required number of elements.

This patch uses rte_eth_xstats_get_names() instead of
rte_eth_xstats_get() because it returns the required number of elements
when the parameter 'xstats_name' is NULL.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..615383bde2 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5558,7 +5558,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
-	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
+	num_xstats = rte_eth_xstats_get_names(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
 
-- 
2.33.0

[PATCH 2/3] ethdev: fix memory leak when telemetry xstats

[Chengwen Feng]

The 'eth_xstats' should be freed after setup telemetry dictionary. This
patch fixes it.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 615383bde2..df20433c2d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	for (i = 0; i < num_xstats; i++)
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	free(eth_xstats);
 	return 0;
 }
 
-- 
2.33.0

[PATCH 3/3] net/cnxk: fix telemetry possible null pointer access

[Chengwen Feng]

The return value of rte_tel_data_alloc() may be null pointer, in this
patch, the null check function is added.

Fixes: 5ea354a1f2cc ("net/cnxk: support telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/cnxk/cnxk_ethdev_telemetry.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/cnxk/cnxk_ethdev_telemetry.c b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
index 83bc65848c..4fd9048643 100644
--- a/drivers/net/cnxk/cnxk_ethdev_telemetry.c
+++ b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
@@ -49,6 +49,8 @@ ethdev_tel_handle_info(const char *cmd __rte_unused,
 	rte_tel_data_add_dict_int(d, "n_ports", n_ports);
 
 	i_data = rte_tel_data_alloc();
+	if (i_data == NULL)
+		return -ENOMEM;
 	rte_tel_data_start_array(i_data, RTE_TEL_U64_VAL);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.33.0

Re: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[Stephen Hemminger]

On Sat, 16 Apr 2022 09:07:45 +0800
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Currently the telemetry xstats uses rte_eth_xstats_get() to retrieve
> the number of elements. But the value to be returned when the parameter
> 'xstats' is NULL is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/
> axgbe) return zero while others return the required number of elements.

Lets fix the PMD's this impacts other code as well.

Re: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[Andrew Rybchenko]

On 4/16/22 04:38, Stephen Hemminger wrote:
> On Sat, 16 Apr 2022 09:07:45 +0800
> Chengwen Feng <fengchengwen@huawei.com> wrote:
> 
>> Currently the telemetry xstats uses rte_eth_xstats_get() to retrieve
>> the number of elements. But the value to be returned when the parameter
>> 'xstats' is NULL is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/
>> axgbe) return zero while others return the required number of elements.
> 
> Lets fix the PMD's this impacts other code as well.

+1

Re: [PATCH 2/3] ethdev: fix memory leak when telemetry xstats

[Andrew Rybchenko]

On 4/16/22 04:07, Chengwen Feng wrote:
> The 'eth_xstats' should be freed after setup telemetry dictionary. This
> patch fixes it.
> 
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH 2/3] ethdev: fix memory leak when telemetry xstats

[David Marchand]

On Sat, Apr 16, 2022 at 3:14 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> The 'eth_xstats' should be freed after setup telemetry dictionary. This
> patch fixes it.
>
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 615383bde2..df20433c2d 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>         for (i = 0; i < num_xstats; i++)
>                 rte_tel_data_add_dict_u64(d, xstat_names[i].name,
>                                 eth_xstats[i].value);
> +       free(eth_xstats);
>         return 0;
>  }
>

We need some minimal testing for telemetry commands.

It could be a test automatically calling all available /ethdev/
commands on a running testpmd.
This test could be really simple, not even checking what is returned.
It would just try every command sequentially with no parameter first,
then with port 0 and finally with port 1.


Coupled with ASan in the CI, this current issue would have been caught.
For example, I tested manually with testpmd + net/null ports:

==3825787==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1040 byte(s) in 1 object(s) allocated from:
    #0 0x7f7048a2d91f in __interceptor_malloc (/usr/lib64/libasan.so.6+0xae91f)
    #1 0x32859e9 in eth_dev_handle_port_xstats
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x32859e9)
    #2 0x3346ac9 in perform_command
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x3346ac9)
    #3 0x3347a8e in client_handler
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x3347a8e)
    #4 0x7f7045751b19 in start_thread
/usr/src/debug/glibc-2.34-25.fc35.x86_64/nptl/pthread_create.c:443



Opinions?


-- 
David Marchand

Re: [PATCH 2/3] ethdev: fix memory leak when telemetry xstats

[Bruce Richardson]

On Thu, Apr 21, 2022 at 10:09:56AM +0200, David Marchand wrote:
> On Sat, Apr 16, 2022 at 3:14 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
> >
> > The 'eth_xstats' should be freed after setup telemetry dictionary. This
> > patch fixes it.
> >
> > Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  lib/ethdev/rte_ethdev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index 615383bde2..df20433c2d 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
> >         for (i = 0; i < num_xstats; i++)
> >                 rte_tel_data_add_dict_u64(d, xstat_names[i].name,
> >                                 eth_xstats[i].value);
> > +       free(eth_xstats);
> >         return 0;
> >  }
> >
> 
> We need some minimal testing for telemetry commands.
> 
> It could be a test automatically calling all available /ethdev/
> commands on a running testpmd.
> This test could be really simple, not even checking what is returned.
> It would just try every command sequentially with no parameter first,
> then with port 0 and finally with port 1.
> 

That seems reasonable. However, I'd go a little further and have all
available commands called as an initial sanity check. Then we can use some
heuristics to go further, with the *dev/stats commands or xstats commands
all being called with numeric parameters as you suggest.

/Bruce

Re: [PATCH 2/3] ethdev: fix memory leak when telemetry xstats

[David Marchand]

On Thu, Apr 21, 2022 at 11:04 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > We need some minimal testing for telemetry commands.
> >
> > It could be a test automatically calling all available /ethdev/
> > commands on a running testpmd.
> > This test could be really simple, not even checking what is returned.
> > It would just try every command sequentially with no parameter first,
> > then with port 0 and finally with port 1.
> >
>
> That seems reasonable. However, I'd go a little further and have all
> available commands called as an initial sanity check. Then we can use some
> heuristics to go further, with the *dev/stats commands or xstats commands
> all being called with numeric parameters as you suggest.

Ok, lgtm too.
Just to be clear, I don't have the time to work on this, so this is
open to volunteers.


-- 
David Marchand

Re: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[fengchengwen]

The root cause is: when the xstats is NULL and n less than required number of
elements is zero, the return value of rte_eth_xstats_get is ambiguous from
rte_ethdev.h's declaration.

But the implementation of rte_eth_xstats_get return required number of elements
when xstats is NULL and n less than required number of elements.

There are two modification schemes:
a) the value of xstats cannot be NULL, and the value of n must be greater than
   or equal to the required number, otherwise, an error code is returned.
b) define the behavior as the same as rte_eth_xstats_get_names, which means return
   required number of elelement when xstats is NULL and n less than required number
   of elements.

I prefer the scheme a because rte_eth_xstats_get and rte_eth_xstats_get_name are
symbiotic, and it's not necessary to both implement the same logic.

Also for scheme a, there is no need to modify the PMD implementation.

What about your opinions ?

On 2022/4/21 14:49, Andrew Rybchenko wrote:
> On 4/16/22 04:38, Stephen Hemminger wrote:
>> On Sat, 16 Apr 2022 09:07:45 +0800
>> Chengwen Feng <fengchengwen@huawei.com> wrote:
>>
>>> Currently the telemetry xstats uses rte_eth_xstats_get() to retrieve
>>> the number of elements. But the value to be returned when the parameter
>>> 'xstats' is NULL is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/
>>> axgbe) return zero while others return the required number of elements.
>>
>> Lets fix the PMD's this impacts other code as well.
> 
> +1
> 
> .

RE: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[Morten Brørup]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Sunday, 24 April 2022 05.45
> 
> The root cause is: when the xstats is NULL and n less than required
> number of
> elements is zero, the return value of rte_eth_xstats_get is ambiguous
> from
> rte_ethdev.h's declaration.
> 
> But the implementation of rte_eth_xstats_get return required number of
> elements
> when xstats is NULL and n less than required number of elements.
> 
> There are two modification schemes:
> a) the value of xstats cannot be NULL, and the value of n must be
> greater than
>    or equal to the required number, otherwise, an error code is
> returned.
> b) define the behavior as the same as rte_eth_xstats_get_names, which
> means return
>    required number of elelement when xstats is NULL and n less than
> required number
>    of elements.
> 
> I prefer the scheme a because rte_eth_xstats_get and
> rte_eth_xstats_get_name are
> symbiotic, and it's not necessary to both implement the same logic.
> 
> Also for scheme a, there is no need to modify the PMD implementation.
> 
> What about your opinions ?

This is an excellent proposal.

And if the documentation for rte_eth_xstats_get() refers to rte_eth_xstats_get_names() for getting the number of elements, it is perfect.

> 
> On 2022/4/21 14:49, Andrew Rybchenko wrote:
> > On 4/16/22 04:38, Stephen Hemminger wrote:
> >> On Sat, 16 Apr 2022 09:07:45 +0800
> >> Chengwen Feng <fengchengwen@huawei.com> wrote:
> >>
> >>> Currently the telemetry xstats uses rte_eth_xstats_get() to
> retrieve
> >>> the number of elements. But the value to be returned when the
> parameter
> >>> 'xstats' is NULL is not specified, some PMDs (eg.
> hns3/ipn3ke/mvpp2/
> >>> axgbe) return zero while others return the required number of
> elements.
> >>
> >> Lets fix the PMD's this impacts other code as well.
> >
> > +1
> >
> > .
> 

[PATCH v2 0/9] bugfix for ethdev telemetry

[Chengwen Feng]

This patch set contains nine bugfix for ethdev telemetry.

Chengwen Feng (9):
  ethdev: define retval when xstats is null of get xstats
  net/hns3: adjust retval when xstats is null of get xstats
  net/ipn3ke: adjust retval when xstats is null of get xstats
  net/mvpp2: adjust retval when xstats is null of get xstats
  net/axgbe: adjust retval when xstats is null of get xstats
  ethdev: fix memory leak when telemetry xstats
  ethdev: support auto-filled flag when telemetry stats
  ethdev: fix possible null pointer access
  net/cnxk: fix telemetry possible null pointer access

---
v2:
* define return value when xstats is null of get xstats.
* fix more bugs when deepin ethdev telemetry.

 drivers/net/axgbe/axgbe_ethdev.c         |  2 +-
 drivers/net/cnxk/cnxk_ethdev_telemetry.c |  2 ++
 drivers/net/hns3/hns3_stats.c            |  7 ++-----
 drivers/net/ipn3ke/ipn3ke_representor.c  |  5 +----
 drivers/net/mvpp2/mrvl_ethdev.c          |  2 +-
 lib/ethdev/rte_ethdev.c                  | 17 ++++++++++++-----
 lib/ethdev/rte_ethdev.h                  |  2 +-
 7 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.33.0

[PATCH v2 1/9] ethdev: define retval when xstats is null of get xstats

[Chengwen Feng]

Currently the value returned when xstats is NULL of rte_eth_xstats_get()
is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/axgbe) return zero
while others return the required number of elements.

This patch defines that the return value should be the required number of
elements when xstats is NULL of rte_eth_xstats_get().

Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 04cff8ee10..0b18297c95 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3174,7 +3174,7 @@ int rte_eth_xstats_get_names(uint16_t port_id,
  * @param xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   If set to NULL, the function returns the required number of elements.
  * @param n
  *   The size of the xstats array (number of elements).
  * @return
-- 
2.33.0

[PATCH v2 2/9] net/hns3: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently hns3 PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 8839c5e202f3 ("net/hns3: support device stats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_stats.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
index 806720faff..4165e22f7f 100644
--- a/drivers/net/hns3/hns3_stats.c
+++ b/drivers/net/hns3/hns3_stats.c
@@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
  * @praram xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   If set to NULL, the function returns the required number of elements.
  * @param n
  *   The size of the xstats array (number of elements).
  * @return
@@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	int count;
 	int ret;
 
-	if (xstats == NULL)
-		return 0;
-
 	count = hns3_xstats_calc_num(dev);
-	if ((int)n < count)
+	if (xstats == NULL || (int)n < count)
 		return count;
 
 	count = 0;
-- 
2.33.0

[PATCH v2 3/9] net/ipn3ke: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently ipn3ke PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 5a6d883878db ("net/ipn3ke: implement statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/ipn3ke/ipn3ke_representor.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_representor.c b/drivers/net/ipn3ke/ipn3ke_representor.c
index c9dde1d82e..9646370b5d 100644
--- a/drivers/net/ipn3ke/ipn3ke_representor.c
+++ b/drivers/net/ipn3ke/ipn3ke_representor.c
@@ -2218,9 +2218,6 @@ ipn3ke_rpst_xstats_get
 	struct ipn3ke_rpst_hw_port_stats hw_stats;
 	struct rte_eth_stats stats;
 
-	if (!xstats)
-		return 0;
-
 	if (!ethdev) {
 		IPN3KE_AFU_PMD_ERR("ethernet device to get statistics is NULL");
 		return -EINVAL;
@@ -2258,7 +2255,7 @@ ipn3ke_rpst_xstats_get
 	port_id = atoi(ch);
 
 	count = ipn3ke_rpst_xstats_calc_num();
-	if (n < count)
+	if (xstats == NULL || n < count)
 		return count;
 
 	if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
-- 
2.33.0

[PATCH v2 4/9] net/mvpp2: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently mvpp2 PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: a77b5378cd41 ("net/mrvl: add extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/mvpp2/mrvl_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index f86701d248..9781a0a411 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -1629,7 +1629,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
 	unsigned int i;
 
 	if (!stats)
-		return 0;
+		return RTE_DIM(mrvl_xstats_tbl);
 
 	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
 	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
-- 
2.33.0

[PATCH v2 5/9] net/axgbe: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently axgbe PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 9d1ef6b2e731 ("net/axgbe: add xstats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/axgbe/axgbe_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 951da5cc26..f6a3a52430 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -1014,7 +1014,7 @@ axgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 	unsigned int i;
 
 	if (!stats)
-		return 0;
+		return AXGBE_XSTATS_COUNT;
 
 	axgbe_read_mmc_stats(pdata);
 
-- 
2.33.0

[PATCH v2 6/9] ethdev: fix memory leak when telemetry xstats

[Chengwen Feng]

The 'eth_xstats' should be freed after setup telemetry dictionary. This
patch fixes it.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..b182694b7d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	for (i = 0; i < num_xstats; i++)
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	free(eth_xstats);
 	return 0;
 }
 
-- 
2.33.0

[PATCH v2 7/9] ethdev: support auto-filled flag when telemetry stats

[Chengwen Feng]

This patch supports auto-filled queue xstats when telemetry stats.

Fixes: f30e69b41f94 ("ethdev: add device flag to bypass auto-filled queue xstats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index b182694b7d..01d4370aa7 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5506,6 +5506,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 		struct rte_tel_data *d)
 {
 	struct rte_eth_stats stats;
+	struct rte_eth_dev *dev;
 	int port_id, ret;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
@@ -5514,6 +5515,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	port_id = atoi(params);
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
+	dev = &rte_eth_devices[port_id];
 
 	ret = rte_eth_stats_get(port_id, &stats);
 	if (ret < 0)
@@ -5528,11 +5530,13 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	ADD_DICT_STAT(stats, ierrors);
 	ADD_DICT_STAT(stats, oerrors);
 	ADD_DICT_STAT(stats, rx_nombuf);
-	eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
-	eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
-	eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
-	eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
-	eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
+	if (dev->data->dev_flags & RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS) {
+		eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
+		eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
+		eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
+		eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
+		eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
+	}
 
 	return 0;
 }
-- 
2.33.0

[PATCH v2 8/9] ethdev: fix possible null pointer access

[Chengwen Feng]

The rte_tel_data_alloc() may return NULL, so the caller should add
judgement for it.

Fixes: 083b0b310b19 ("ethdev: add common stats for telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 01d4370aa7..a61ec78473 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5492,6 +5492,8 @@ eth_dev_add_port_queue_stats(struct rte_tel_data *d, uint64_t *q_stats,
 {
 	int q;
 	struct rte_tel_data *q_data = rte_tel_data_alloc();
+	if (q_data == NULL)
+		return;
 	rte_tel_data_start_array(q_data, RTE_TEL_U64_VAL);
 	for (q = 0; q < RTE_ETHDEV_QUEUE_STAT_CNTRS; q++)
 		rte_tel_data_add_array_u64(q_data, q_stats[q]);
-- 
2.33.0

[PATCH v2 9/9] net/cnxk: fix telemetry possible null pointer access

[Chengwen Feng]

The return value of rte_tel_data_alloc() may be null pointer, in this
patch, the null check function is added.

Fixes: 5ea354a1f2cc ("net/cnxk: support telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/cnxk/cnxk_ethdev_telemetry.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/cnxk/cnxk_ethdev_telemetry.c b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
index 83bc65848c..4fd9048643 100644
--- a/drivers/net/cnxk/cnxk_ethdev_telemetry.c
+++ b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
@@ -49,6 +49,8 @@ ethdev_tel_handle_info(const char *cmd __rte_unused,
 	rte_tel_data_add_dict_int(d, "n_ports", n_ports);
 
 	i_data = rte_tel_data_alloc();
+	if (i_data == NULL)
+		return -ENOMEM;
 	rte_tel_data_start_array(i_data, RTE_TEL_U64_VAL);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.33.0

Re: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[fengchengwen]

On 2022/4/25 18:16, Morten Brørup wrote:
>> From: fengchengwen [mailto:fengchengwen@huawei.com]
>> Sent: Sunday, 24 April 2022 05.45
>>
>> The root cause is: when the xstats is NULL and n less than required
>> number of
>> elements is zero, the return value of rte_eth_xstats_get is ambiguous
>> from
>> rte_ethdev.h's declaration.
>>
>> But the implementation of rte_eth_xstats_get return required number of
>> elements
>> when xstats is NULL and n less than required number of elements.
>>
>> There are two modification schemes:
>> a) the value of xstats cannot be NULL, and the value of n must be
>> greater than
>>    or equal to the required number, otherwise, an error code is
>> returned.
>> b) define the behavior as the same as rte_eth_xstats_get_names, which
>> means return
>>    required number of elelement when xstats is NULL and n less than
>> required number
>>    of elements.
>>
>> I prefer the scheme a because rte_eth_xstats_get and
>> rte_eth_xstats_get_name are
>> symbiotic, and it's not necessary to both implement the same logic.
>>
>> Also for scheme a, there is no need to modify the PMD implementation.
>>
>> What about your opinions ?
> 
> This is an excellent proposal.
> 
> And if the documentation for rte_eth_xstats_get() refers to rte_eth_xstats_get_names() for getting the number of elements, it is perfect.

Hi Morten,

   I did a deepin review and found that many application use rte_eth_xstats_get(port_id, NULL, 0),
and also many PMD already support return required number of entries when xstats is NULL. So in
the v2, I use the modified hns3/ipn3ke/mvpp2/axgbe PMD method and explicit return value when xstats
is NULL of rte_eth_xstats-get().

Thanks.

> 
>>
>> On 2022/4/21 14:49, Andrew Rybchenko wrote:
>>> On 4/16/22 04:38, Stephen Hemminger wrote:
>>>> On Sat, 16 Apr 2022 09:07:45 +0800
>>>> Chengwen Feng <fengchengwen@huawei.com> wrote:
>>>>
>>>>> Currently the telemetry xstats uses rte_eth_xstats_get() to
>> retrieve
>>>>> the number of elements. But the value to be returned when the
>> parameter
>>>>> 'xstats' is NULL is not specified, some PMDs (eg.
>> hns3/ipn3ke/mvpp2/
>>>>> axgbe) return zero while others return the required number of
>> elements.
>>>>
>>>> Lets fix the PMD's this impacts other code as well.
>>>
>>> +1
>>>
>>> .
>>
> 

RE: [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs

[Morten Brørup]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 28 April 2022 15.39
> 
> On 2022/4/25 18:16, Morten Brørup wrote:
> >> From: fengchengwen [mailto:fengchengwen@huawei.com]
> >> Sent: Sunday, 24 April 2022 05.45
> >>
> >> The root cause is: when the xstats is NULL and n less than required
> >> number of
> >> elements is zero, the return value of rte_eth_xstats_get is
> ambiguous
> >> from
> >> rte_ethdev.h's declaration.
> >>
> >> But the implementation of rte_eth_xstats_get return required number
> of
> >> elements
> >> when xstats is NULL and n less than required number of elements.
> >>
> >> There are two modification schemes:
> >> a) the value of xstats cannot be NULL, and the value of n must be
> >> greater than
> >>    or equal to the required number, otherwise, an error code is
> >> returned.
> >> b) define the behavior as the same as rte_eth_xstats_get_names,
> which
> >> means return
> >>    required number of elelement when xstats is NULL and n less than
> >> required number
> >>    of elements.
> >>
> >> I prefer the scheme a because rte_eth_xstats_get and
> >> rte_eth_xstats_get_name are
> >> symbiotic, and it's not necessary to both implement the same logic.
> >>
> >> Also for scheme a, there is no need to modify the PMD
> implementation.
> >>
> >> What about your opinions ?
> >
> > This is an excellent proposal.
> >
> > And if the documentation for rte_eth_xstats_get() refers to
> rte_eth_xstats_get_names() for getting the number of elements, it is
> perfect.
> 
> Hi Morten,
> 
>    I did a deepin review and found that many application use
> rte_eth_xstats_get(port_id, NULL, 0),
> and also many PMD already support return required number of entries
> when xstats is NULL. So in
> the v2, I use the modified hns3/ipn3ke/mvpp2/axgbe PMD method and
> explicit return value when xstats
> is NULL of rte_eth_xstats-get().

Instead of modifying the drivers, consider copying this code from rte_eth_xstats_get_names() to rte_eth_xstats_get():

	cnt_expected_entries = eth_dev_get_xstats_count(port_id);
	if (xstats_names == NULL || cnt_expected_entries < 0 ||
			(int)size < cnt_expected_entries)
		return cnt_expected_entries;

Just a suggestion. Either solution is fine with me.

> 
> Thanks.
> 
> >
> >>
> >> On 2022/4/21 14:49, Andrew Rybchenko wrote:
> >>> On 4/16/22 04:38, Stephen Hemminger wrote:
> >>>> On Sat, 16 Apr 2022 09:07:45 +0800
> >>>> Chengwen Feng <fengchengwen@huawei.com> wrote:
> >>>>
> >>>>> Currently the telemetry xstats uses rte_eth_xstats_get() to
> >> retrieve
> >>>>> the number of elements. But the value to be returned when the
> >> parameter
> >>>>> 'xstats' is NULL is not specified, some PMDs (eg.
> >> hns3/ipn3ke/mvpp2/
> >>>>> axgbe) return zero while others return the required number of
> >> elements.
> >>>>
> >>>> Lets fix the PMD's this impacts other code as well.
> >>>
> >>> +1
> >>>
> >>> .
> >>
> >
> 

Re: [PATCH v2 1/9] ethdev: define retval when xstats is null of get xstats

[Andrew Rybchenko]

On 4/28/22 16:15, Chengwen Feng wrote:
> Currently the value returned when xstats is NULL of rte_eth_xstats_get()
> is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/axgbe) return zero
> while others return the required number of elements.
> 
> This patch defines that the return value should be the required number of
> elements when xstats is NULL of rte_eth_xstats_get().
> 
> Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 04cff8ee10..0b18297c95 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3174,7 +3174,7 @@ int rte_eth_xstats_get_names(uint16_t port_id,
>    * @param xstats
>    *   A pointer to a table of structure of type *rte_eth_xstat*
>    *   to be filled with device statistics ids and values.
> - *   This parameter can be set to NULL if n is 0.
> + *   If set to NULL, the function returns the required number of elements.

I'm sorry, but I disagree with the patch. First of all it
removes limitation when xstats may be NULL. Second, I think
that clarification is not required since:
if xstats is NULL, n must be 0 as defined above and return
value description says:
3183  *   - A positive value higher than n: error, the given statistics 
table
3184  *     is too small. The return value corresponds to the size that 
should
3185  *     be given to succeed. The entries in the table are not valid and
3186  *     shall not be used by the caller.


>    * @param n
>    *   The size of the xstats array (number of elements).
>    * @return

[PATCH v3 00/10] bugfix for ethdev telemetry

[Chengwen Feng]

This patch set contains nine bugfix for ethdev telemetry.

Chengwen Feng (10):
  ethdev: define retval when xstats is null of get xstats
  ethdev: optimize xstats-get API's implementation
  net/hns3: adjust retval when xstats is null of get xstats
  net/ipn3ke: adjust retval when xstats is null of get xstats
  net/mvpp2: adjust retval when xstats is null of get xstats
  net/axgbe: adjust retval when xstats is null of get xstats
  ethdev: fix memory leak when telemetry xstats
  ethdev: support auto-filled flag when telemetry stats
  ethdev: fix possible null pointer access
  net/cnxk: fix telemetry possible null pointer access

---
v3:
* refine the descriptor of return value when xstats is null of 
  rte_eth_xstats_get().
* add patch for optimize xstats-get API's implementation.
v2:
* define return value when xstats is null of get xstats.
* fix more bugs when deepin ethdev telemetry.

 drivers/net/axgbe/axgbe_ethdev.c         |  2 +-
 drivers/net/cnxk/cnxk_ethdev_telemetry.c |  2 ++
 drivers/net/hns3/hns3_stats.c            |  7 ++----
 drivers/net/ipn3ke/ipn3ke_representor.c  |  5 +----
 drivers/net/mvpp2/mrvl_ethdev.c          |  2 +-
 lib/ethdev/rte_ethdev.c                  | 28 ++++++++++++------------
 lib/ethdev/rte_ethdev.h                  |  3 ++-
 7 files changed, 23 insertions(+), 26 deletions(-)

-- 
2.33.0

[PATCH v3 01/10] ethdev: define retval when xstats is null of get xstats

[Chengwen Feng]

Currently the value returned when xstats is NULL of rte_eth_xstats_get()
is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/axgbe) return zero
while others return the required number of elements.

This patch defines that the return value should be the required number of
elements when xstats is NULL of rte_eth_xstats_get().

Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 04cff8ee10..1aefc48532 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3174,7 +3174,8 @@ int rte_eth_xstats_get_names(uint16_t port_id,
  * @param xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   This parameter can be set to NULL if n is 0; If set to NULL, the
+ *   function returns the required number of elements.
  * @param n
  *   The size of the xstats array (number of elements).
  * @return
-- 
2.33.0

[PATCH v3 02/10] ethdev: optimize xstats-get API's implementation

[Chengwen Feng]

This patch uses eth_dev_get_xstats_basic_count() to retrieve generic
statistics count.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..ed395a8f54 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2967,21 +2967,14 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 	unsigned int n)
 {
 	struct rte_eth_dev *dev;
-	unsigned int count = 0, i;
+	unsigned int count, i;
 	signed int xcount = 0;
-	uint16_t nb_rxqs, nb_txqs;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
-	/* Return generic statistics */
-	count = RTE_NB_STATS;
-	if (dev->data->dev_flags & RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS)
-		count += (nb_rxqs * RTE_NB_RXQ_STATS) + (nb_txqs * RTE_NB_TXQ_STATS);
+	count = eth_dev_get_xstats_basic_count(dev);
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL) {
-- 
2.33.0

[PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently hns3 PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 8839c5e202f3 ("net/hns3: support device stats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_stats.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
index 806720faff..4165e22f7f 100644
--- a/drivers/net/hns3/hns3_stats.c
+++ b/drivers/net/hns3/hns3_stats.c
@@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
  * @praram xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   If set to NULL, the function returns the required number of elements.
  * @param n
  *   The size of the xstats array (number of elements).
  * @return
@@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	int count;
 	int ret;
 
-	if (xstats == NULL)
-		return 0;
-
 	count = hns3_xstats_calc_num(dev);
-	if ((int)n < count)
+	if (xstats == NULL || (int)n < count)
 		return count;
 
 	count = 0;
-- 
2.33.0

[PATCH v3 04/10] net/ipn3ke: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently ipn3ke PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 5a6d883878db ("net/ipn3ke: implement statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/ipn3ke/ipn3ke_representor.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_representor.c b/drivers/net/ipn3ke/ipn3ke_representor.c
index c9dde1d82e..9646370b5d 100644
--- a/drivers/net/ipn3ke/ipn3ke_representor.c
+++ b/drivers/net/ipn3ke/ipn3ke_representor.c
@@ -2218,9 +2218,6 @@ ipn3ke_rpst_xstats_get
 	struct ipn3ke_rpst_hw_port_stats hw_stats;
 	struct rte_eth_stats stats;
 
-	if (!xstats)
-		return 0;
-
 	if (!ethdev) {
 		IPN3KE_AFU_PMD_ERR("ethernet device to get statistics is NULL");
 		return -EINVAL;
@@ -2258,7 +2255,7 @@ ipn3ke_rpst_xstats_get
 	port_id = atoi(ch);
 
 	count = ipn3ke_rpst_xstats_calc_num();
-	if (n < count)
+	if (xstats == NULL || n < count)
 		return count;
 
 	if (hw->retimer.mac_type == IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
-- 
2.33.0

[PATCH v3 05/10] net/mvpp2: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently mvpp2 PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: a77b5378cd41 ("net/mrvl: add extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/mvpp2/mrvl_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index f86701d248..9781a0a411 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -1629,7 +1629,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
 	unsigned int i;
 
 	if (!stats)
-		return 0;
+		return RTE_DIM(mrvl_xstats_tbl);
 
 	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
 	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
-- 
2.33.0

[PATCH v3 06/10] net/axgbe: adjust retval when xstats is null of get xstats

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently axgbe PMD
returns zero when xstats is NULL.

This patch adjusts that the return value was the required number of
elements when stats is NULL.

Fixes: 9d1ef6b2e731 ("net/axgbe: add xstats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/axgbe/axgbe_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 951da5cc26..f6a3a52430 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -1014,7 +1014,7 @@ axgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 	unsigned int i;
 
 	if (!stats)
-		return 0;
+		return AXGBE_XSTATS_COUNT;
 
 	axgbe_read_mmc_stats(pdata);
 
-- 
2.33.0

[PATCH v3 07/10] ethdev: fix memory leak when telemetry xstats

[Chengwen Feng]

The 'eth_xstats' should be freed after setup telemetry dictionary. This
patch fixes it.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index ed395a8f54..f26a9bac6d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5578,6 +5578,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	for (i = 0; i < num_xstats; i++)
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	free(eth_xstats);
 	return 0;
 }
 
-- 
2.33.0

[PATCH v3 08/10] ethdev: support auto-filled flag when telemetry stats

[Chengwen Feng]

This patch supports auto-filled queue xstats when telemetry stats.

Fixes: f30e69b41f94 ("ethdev: add device flag to bypass auto-filled queue xstats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f26a9bac6d..76037e635b 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5499,6 +5499,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 		struct rte_tel_data *d)
 {
 	struct rte_eth_stats stats;
+	struct rte_eth_dev *dev;
 	int port_id, ret;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
@@ -5507,6 +5508,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	port_id = atoi(params);
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
+	dev = &rte_eth_devices[port_id];
 
 	ret = rte_eth_stats_get(port_id, &stats);
 	if (ret < 0)
@@ -5521,11 +5523,13 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
 	ADD_DICT_STAT(stats, ierrors);
 	ADD_DICT_STAT(stats, oerrors);
 	ADD_DICT_STAT(stats, rx_nombuf);
-	eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
-	eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
-	eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
-	eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
-	eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
+	if (dev->data->dev_flags & RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS) {
+		eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
+		eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
+		eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
+		eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
+		eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
+	}
 
 	return 0;
 }
-- 
2.33.0

[PATCH v3 09/10] ethdev: fix possible null pointer access

[Chengwen Feng]

The rte_tel_data_alloc() may return NULL, so the caller should add
judgement for it.

Fixes: 083b0b310b19 ("ethdev: add common stats for telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 76037e635b..4a76272722 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5485,6 +5485,8 @@ eth_dev_add_port_queue_stats(struct rte_tel_data *d, uint64_t *q_stats,
 {
 	int q;
 	struct rte_tel_data *q_data = rte_tel_data_alloc();
+	if (q_data == NULL)
+		return;
 	rte_tel_data_start_array(q_data, RTE_TEL_U64_VAL);
 	for (q = 0; q < RTE_ETHDEV_QUEUE_STAT_CNTRS; q++)
 		rte_tel_data_add_array_u64(q_data, q_stats[q]);
-- 
2.33.0

[PATCH v3 10/10] net/cnxk: fix telemetry possible null pointer access

[Chengwen Feng]

The return value of rte_tel_data_alloc() may be null pointer, in this
patch, the null check function is added.

Fixes: 5ea354a1f2cc ("net/cnxk: support telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/cnxk/cnxk_ethdev_telemetry.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/cnxk/cnxk_ethdev_telemetry.c b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
index 83bc65848c..4fd9048643 100644
--- a/drivers/net/cnxk/cnxk_ethdev_telemetry.c
+++ b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
@@ -49,6 +49,8 @@ ethdev_tel_handle_info(const char *cmd __rte_unused,
 	rte_tel_data_add_dict_int(d, "n_ports", n_ports);
 
 	i_data = rte_tel_data_alloc();
+	if (i_data == NULL)
+		return -ENOMEM;
 	rte_tel_data_start_array(i_data, RTE_TEL_U64_VAL);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.33.0

Re: [PATCH v2 2/9] net/hns3: adjust retval when xstats is null of get xstats

[Min Hu (Connor)]

Acked-by: Min Hu (Connor) <humin29@huawei.com>

在 2022/4/28 21:15, Chengwen Feng 写道:
> Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
> to retrieve the required number of elements, but currently hns3 PMD
> returns zero when xstats is NULL.
> 
> This patch adjusts that the return value was the required number of
> elements when stats is NULL.
> 
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   drivers/net/hns3/hns3_stats.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 806720faff..4165e22f7f 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>    * @praram xstats
>    *   A pointer to a table of structure of type *rte_eth_xstat*
>    *   to be filled with device statistics ids and values.
> - *   This parameter can be set to NULL if n is 0.
> + *   If set to NULL, the function returns the required number of elements.
>    * @param n
>    *   The size of the xstats array (number of elements).
>    * @return
> @@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>   	int count;
>   	int ret;
>   
> -	if (xstats == NULL)
> -		return 0;
> -
>   	count = hns3_xstats_calc_num(dev);
> -	if ((int)n < count)
> +	if (xstats == NULL || (int)n < count)
>   		return count;
>   
>   	count = 0;
> 

Re: [PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
> to retrieve the required number of elements, but currently hns3 PMD
> returns zero when xstats is NULL.
> 
> This patch adjusts that the return value was the required number of
> elements when stats is NULL.
> 
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   drivers/net/hns3/hns3_stats.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 806720faff..4165e22f7f 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>    * @praram xstats
>    *   A pointer to a table of structure of type *rte_eth_xstat*
>    *   to be filled with device statistics ids and values.
> - *   This parameter can be set to NULL if n is 0.
> + *   If set to NULL, the function returns the required number of elements.
>    * @param n
>    *   The size of the xstats array (number of elements).
>    * @return
> @@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>   	int count;
>   	int ret;
>   
> -	if (xstats == NULL)
> -		return 0;
> -

Above lines are OK. ethdev layer should care about it. See
below.

>   	count = hns3_xstats_calc_num(dev);
> -	if ((int)n < count)
> +	if (xstats == NULL || (int)n < count)

but I dislike the change. xstats may be NULL if and only if n
is 0. If so, the extra check for xstats vs NULL is not required
here.

IMHO ethdev must guarantee it. I.e. rte_eth_xstats_get()
implementation in the first patch should be updated to
return -EINVAL if xstats is NULL, but n is not 0.

The major point for me here is to have just one natural key
parameter to make a decision if number of stats should be
returned or we really need to fill xstats in.
n is the parameter. If space is insufficient to return
xstats, return required space. If n is 0, xstats may be NULL,
since we provide no space for values. If we provide some
space for values (n is greater than 0), xstats must be not
NULL.

One more possible improvement in rte_eth_xstats_get()
implementation is to no provide out of xstats array
pointer if n is smaller or equal to number of basic
stats. Of course, space passed to xstats_get will be
zero anyway and xstats pointer should not be used, but
it is still safer to care about it explicitly. I.e. something like

2984 »···»···xcount = (*dev->dev_ops->xstats_get)(dev,
2985 »···»···»···»···     (n > count) ? xstats + count : NULL,
2986 »···»···»···»···     (n > count) ? n - count : 0);

If we guarantee that xstats is not NULL, if n is positive,
extra check for xstats vs NULL will not be required.

>   		return count;
>   
>   	count = 0;

Re: [PATCH v3 06/10] net/axgbe: adjust retval when xstats is null of get xstats

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
> to retrieve the required number of elements, but currently axgbe PMD
> returns zero when xstats is NULL.
> 
> This patch adjusts that the return value was the required number of
> elements when stats is NULL.
> 
> Fixes: 9d1ef6b2e731 ("net/axgbe: add xstats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   drivers/net/axgbe/axgbe_ethdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index 951da5cc26..f6a3a52430 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -1014,7 +1014,7 @@ axgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
>   	unsigned int i;
>   
>   	if (!stats)

I think the right condition is: n < AXGBE_XSTATS_COUNT
Otherwise the function break contract to return required
number of xstats if space is insufficient.

> -		return 0;
> +		return AXGBE_XSTATS_COUNT;
>   
>   	axgbe_read_mmc_stats(pdata);
>   

Re: [PATCH v3 08/10] ethdev: support auto-filled flag when telemetry stats

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> This patch supports auto-filled queue xstats when telemetry stats.
> 
> Fixes: f30e69b41f94 ("ethdev: add device flag to bypass auto-filled queue xstats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f26a9bac6d..76037e635b 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5499,6 +5499,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
>   		struct rte_tel_data *d)
>   {
>   	struct rte_eth_stats stats;
> +	struct rte_eth_dev *dev;
>   	int port_id, ret;
>   
>   	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> @@ -5507,6 +5508,7 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
>   	port_id = atoi(params);
>   	if (!rte_eth_dev_is_valid_port(port_id))
>   		return -1;
> +	dev = &rte_eth_devices[port_id];
>   
>   	ret = rte_eth_stats_get(port_id, &stats);
>   	if (ret < 0)
> @@ -5521,11 +5523,13 @@ eth_dev_handle_port_stats(const char *cmd __rte_unused,
>   	ADD_DICT_STAT(stats, ierrors);
>   	ADD_DICT_STAT(stats, oerrors);
>   	ADD_DICT_STAT(stats, rx_nombuf);
> -	eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
> -	eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
> -	eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
> -	eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
> -	eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
> +	if (dev->data->dev_flags & RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS) {

I don't understand it. RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS driver
flag means that driver asks ethdev layer to autofill per-queue
*xstats* (generate names and put values) using per-queue basic
statistics.

Drivers do no set the flag if provide per-queue xstats itself
(to avoid duplication) or do not fill in per-queue basic
xstats (to avoid meaningless/misleading zeros in xstats).

So, absence of the flag does not mean that per-queue basic
statistics are not filled in and should be used as the
guard to include corresponding values in telemetry.

> +		eth_dev_add_port_queue_stats(d, stats.q_ipackets, "q_ipackets");
> +		eth_dev_add_port_queue_stats(d, stats.q_opackets, "q_opackets");
> +		eth_dev_add_port_queue_stats(d, stats.q_ibytes, "q_ibytes");
> +		eth_dev_add_port_queue_stats(d, stats.q_obytes, "q_obytes");
> +		eth_dev_add_port_queue_stats(d, stats.q_errors, "q_errors");
> +	}
>   
>   	return 0;
>   }

Re: [PATCH v3 09/10] ethdev: fix possible null pointer access

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> The rte_tel_data_alloc() may return NULL, so the caller should add
> judgement for it.
> 
> Fixes: 083b0b310b19 ("ethdev: add common stats for telemetry")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v3 10/10] net/cnxk: fix telemetry possible null pointer access

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> The return value of rte_tel_data_alloc() may be null pointer, in this
> patch, the null check function is added.
> 
> Fixes: 5ea354a1f2cc ("net/cnxk: support telemetry")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v3 02/10] ethdev: optimize xstats-get API's implementation

[Andrew Rybchenko]

On 5/5/22 11:02, Chengwen Feng wrote:
> This patch uses eth_dev_get_xstats_basic_count() to retrieve generic
> statistics count.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[PATCH v4 0/9] bugfix for ethdev telemetry

[Chengwen Feng]

This patch set contains nine bugfix for ethdev telemetry.

Chengwen Feng (9):
  ethdev: specify return value of xstats-get API
  ethdev: optimize xstats-get API's implementation
  net/hns3: adjust return value of xstats-get ops
  net/ipn3ke: adjust return value of xstats-get ops
  net/mvpp2: adjust return value of xstats-get ops
  net/axgbe: adjust return value of xstats-get ops
  ethdev: fix memory leak when telemetry xstats
  ethdev: fix possible null pointer access
  net/cnxk: fix telemetry possible null pointer access

---
v4:
* refine the return value definitions of rte_eth_xstats_get() and
  the implementation of related drivers is corrected.
* drop auto-filled flag patch.
v3:
* refine the descriptor of return value when xstats is null of
  rte_eth_xstats_get().
* add patch for optimize xstats-get API's implementation.
v2:
* define return value when xstats is null of get xstats.
* fix more bugs when deepin ethdev telemetry.

 drivers/net/axgbe/axgbe_ethdev.c         |  8 ++++----
 drivers/net/cnxk/cnxk_ethdev_telemetry.c |  2 ++
 drivers/net/hns3/hns3_stats.c            |  9 +++++----
 drivers/net/ipn3ke/ipn3ke_representor.c  |  3 ---
 drivers/net/mvpp2/mrvl_ethdev.c          | 11 ++++++-----
 lib/ethdev/rte_ethdev.c                  | 18 ++++++++----------
 lib/ethdev/rte_ethdev.h                  |  6 +++++-
 7 files changed, 30 insertions(+), 27 deletions(-)

-- 
2.33.0

[PATCH v4 1/9] ethdev: specify return value of xstats-get API

[Chengwen Feng]

Currently the value returned when xstats is NULL of rte_eth_xstats_get()
is not specified, some PMDs (eg. hns3/ipn3ke/mvpp2/axgbe) return zero
while others return the required number of elements.

In this patch, special parameter combinations are restricted:
1. specify that xstats is NULL if and only if n is 0.
2. specify that if n is lower than the required number of elements, the
function returns the required number of elements.
3. specify that if n is zero, the xstats must be NULL, the function
returns the required number of elements.

This patch also makes sure that xstats is NULL if n is lower or equal to
the number of basic stats when invoke driver's ops.

Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 4 +++-
 lib/ethdev/rte_ethdev.h | 6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..d9661bd64f 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2973,6 +2973,8 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	if (xstats == NULL && n > 0)
+		return -EINVAL;
 	dev = &rte_eth_devices[port_id];
 
 	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
@@ -2989,7 +2991,7 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 		 * xstats struct.
 		 */
 		xcount = (*dev->dev_ops->xstats_get)(dev,
-				     xstats ? xstats + count : NULL,
+				     (n > count) ? xstats + count : NULL,
 				     (n > count) ? n - count : 0);
 
 		if (xcount < 0)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 04cff8ee10..04225bba4d 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3174,9 +3174,13 @@ int rte_eth_xstats_get_names(uint16_t port_id,
  * @param xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   This parameter can be set to NULL if and only if n is 0.
  * @param n
  *   The size of the xstats array (number of elements).
+ *   If lower than the required number of elements, the function returns
+ *   the required number of elements.
+ *   If equal to zero, the xstats must be NULL, the function returns the
+ *   required number of elements.
  * @return
  *   - A positive value lower or equal to n: success. The return value
  *     is the number of entries filled in the stats table.
-- 
2.33.0

[PATCH v4 2/9] ethdev: optimize xstats-get API's implementation

[Chengwen Feng]

This patch uses eth_dev_get_xstats_basic_count() to retrieve generic
statistics count.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d9661bd64f..78192c26f7 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -2967,9 +2967,8 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 	unsigned int n)
 {
 	struct rte_eth_dev *dev;
-	unsigned int count = 0, i;
+	unsigned int count, i;
 	signed int xcount = 0;
-	uint16_t nb_rxqs, nb_txqs;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -2977,13 +2976,7 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 		return -EINVAL;
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
-	/* Return generic statistics */
-	count = RTE_NB_STATS;
-	if (dev->data->dev_flags & RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS)
-		count += (nb_rxqs * RTE_NB_RXQ_STATS) + (nb_txqs * RTE_NB_TXQ_STATS);
+	count = eth_dev_get_xstats_basic_count(dev);
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL) {
-- 
2.33.0

[PATCH v4 3/9] net/hns3: adjust return value of xstats-get ops

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently hns3 PMD
returns zero when xstats is NULL.

In the previous patch, the framework defines that the required number
of entries should be returned when xstats is NULL and n is zero. And
xstats is NULL if and only if n is zero.

Based on the preceding constraints, this patch removes the logic of
"return zero when xstats is NULL".

Fixes: 8839c5e202f3 ("net/hns3: support device stats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_stats.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
index 806720faff..ac1f0d9126 100644
--- a/drivers/net/hns3/hns3_stats.c
+++ b/drivers/net/hns3/hns3_stats.c
@@ -1020,9 +1020,13 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
  * @praram xstats
  *   A pointer to a table of structure of type *rte_eth_xstat*
  *   to be filled with device statistics ids and values.
- *   This parameter can be set to NULL if n is 0.
+ *   This parameter can be set to NULL if and only if n is 0.
  * @param n
  *   The size of the xstats array (number of elements).
+ *   If lower than the required number of elements, the function returns the
+ *   required number of elements.
+ *   If equal to zero, the xstats parameter must be NULL, the function returns
+ *   the required number of elements.
  * @return
  *   0 on fail, count(The size of the statistics elements) on success.
  */
@@ -1041,9 +1045,6 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	int count;
 	int ret;
 
-	if (xstats == NULL)
-		return 0;
-
 	count = hns3_xstats_calc_num(dev);
 	if ((int)n < count)
 		return count;
-- 
2.33.0

[PATCH v4 4/9] net/ipn3ke: adjust return value of xstats-get ops

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently ipn3ke PMD
returns zero when xstats is NULL.

In the previous patch, the framework defines that the required number
of entries should be returned when xstats is NULL and n is zero. And
xstats is NULL if and only if n is zero.

Based on the preceding constraints, this patch removes the logic of
"return zero when xstats is NULL".

Fixes: 5a6d883878db ("net/ipn3ke: implement statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/ipn3ke/ipn3ke_representor.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ipn3ke/ipn3ke_representor.c b/drivers/net/ipn3ke/ipn3ke_representor.c
index c9dde1d82e..abbecfdf2e 100644
--- a/drivers/net/ipn3ke/ipn3ke_representor.c
+++ b/drivers/net/ipn3ke/ipn3ke_representor.c
@@ -2218,9 +2218,6 @@ ipn3ke_rpst_xstats_get
 	struct ipn3ke_rpst_hw_port_stats hw_stats;
 	struct rte_eth_stats stats;
 
-	if (!xstats)
-		return 0;
-
 	if (!ethdev) {
 		IPN3KE_AFU_PMD_ERR("ethernet device to get statistics is NULL");
 		return -EINVAL;
-- 
2.33.0

[PATCH v4 5/9] net/mvpp2: adjust return value of xstats-get ops

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently mvpp2 PMD
returns zero when xstats is NULL.

In the previous patch, the framework defines that the required number
of entries should be returned when n is lower than the required number
of entries, and makes sure xstats must not be NULL when n is non-zero.

This patch removes the logic of "return zero when xstats is NULL", and
adds the logic of "return the required number of entries when n is
lower than the required number of entries".

Fixes: a77b5378cd41 ("net/mrvl: add extended statistics")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/mvpp2/mrvl_ethdev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index f86701d248..735efb6cfc 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -1626,13 +1626,14 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
 {
 	struct mrvl_priv *priv = dev->data->dev_private;
 	struct pp2_ppio_statistics ppio_stats;
-	unsigned int i;
+	unsigned int i, count;
 
-	if (!stats)
-		return 0;
+	count = RTE_DIM(mrvl_xstats_tbl);
+	if (n < count)
+		return count;
 
 	pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
-	for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
+	for (i = 0; i < count; i++) {
 		uint64_t val;
 
 		if (mrvl_xstats_tbl[i].size == sizeof(uint32_t))
@@ -1648,7 +1649,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
 		stats[i].value = val;
 	}
 
-	return n;
+	return count;
 }
 
 /**
-- 
2.33.0

[PATCH v4 6/9] net/axgbe: adjust return value of xstats-get ops

[Chengwen Feng]

Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
to retrieve the required number of elements, but currently axgbe PMD
returns zero when xstats is NULL.

In the previous patch, the framework defines that the required number
of entries should be returned when n is lower than the required number
of entries, and makes sure xstats must not be NULL when n is non-zero.

This patch removes the logic of "return zero when xstats is NULL", and
adds the logic of "return the required number of entries when n is
lower than the required number of entries".

Fixes: 9d1ef6b2e731 ("net/axgbe: add xstats")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/axgbe/axgbe_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 951da5cc26..e6822fa711 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -1013,18 +1013,18 @@ axgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 	struct axgbe_port *pdata = dev->data->dev_private;
 	unsigned int i;
 
-	if (!stats)
-		return 0;
+	if (n < AXGBE_XSTATS_COUNT)
+		return AXGBE_XSTATS_COUNT;
 
 	axgbe_read_mmc_stats(pdata);
 
-	for (i = 0; i < n && i < AXGBE_XSTATS_COUNT; i++) {
+	for (i = 0; i < AXGBE_XSTATS_COUNT; i++) {
 		stats[i].id = i;
 		stats[i].value = *(u64 *)((uint8_t *)&pdata->mmc_stats +
 				axgbe_xstats_strings[i].offset);
 	}
 
-	return i;
+	return AXGBE_XSTATS_COUNT;
 }
 
 static int
-- 
2.33.0

[PATCH v4 7/9] ethdev: fix memory leak when telemetry xstats

[Chengwen Feng]

The 'eth_xstats' should be freed after setup telemetry dictionary. This
patch fixes it.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 78192c26f7..1ae686a7aa 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5580,6 +5580,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	for (i = 0; i < num_xstats; i++)
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	free(eth_xstats);
 	return 0;
 }
 
-- 
2.33.0

[PATCH v4 8/9] ethdev: fix possible null pointer access

[Chengwen Feng]

The rte_tel_data_alloc() may return NULL, so the caller should add
judgement for it.

Fixes: 083b0b310b19 ("ethdev: add common stats for telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/ethdev/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 1ae686a7aa..60c4361256 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5487,6 +5487,8 @@ eth_dev_add_port_queue_stats(struct rte_tel_data *d, uint64_t *q_stats,
 {
 	int q;
 	struct rte_tel_data *q_data = rte_tel_data_alloc();
+	if (q_data == NULL)
+		return;
 	rte_tel_data_start_array(q_data, RTE_TEL_U64_VAL);
 	for (q = 0; q < RTE_ETHDEV_QUEUE_STAT_CNTRS; q++)
 		rte_tel_data_add_array_u64(q_data, q_stats[q]);
-- 
2.33.0

[PATCH v4 9/9] net/cnxk: fix telemetry possible null pointer access

[Chengwen Feng]

The return value of rte_tel_data_alloc() may be null pointer, in this
patch, the null check function is added.

Fixes: 5ea354a1f2cc ("net/cnxk: support telemetry")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/cnxk/cnxk_ethdev_telemetry.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/cnxk/cnxk_ethdev_telemetry.c b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
index 83bc65848c..4fd9048643 100644
--- a/drivers/net/cnxk/cnxk_ethdev_telemetry.c
+++ b/drivers/net/cnxk/cnxk_ethdev_telemetry.c
@@ -49,6 +49,8 @@ ethdev_tel_handle_info(const char *cmd __rte_unused,
 	rte_tel_data_add_dict_int(d, "n_ports", n_ports);
 
 	i_data = rte_tel_data_alloc();
+	if (i_data == NULL)
+		return -ENOMEM;
 	rte_tel_data_start_array(i_data, RTE_TEL_U64_VAL);
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.33.0

RE: [PATCH v4 0/9] bugfix for ethdev telemetry

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Friday, 13 May 2022 04.54
> 
> This patch set contains nine bugfix for ethdev telemetry.
> 
> Chengwen Feng (9):
>   ethdev: specify return value of xstats-get API
>   ethdev: optimize xstats-get API's implementation
>   net/hns3: adjust return value of xstats-get ops
>   net/ipn3ke: adjust return value of xstats-get ops
>   net/mvpp2: adjust return value of xstats-get ops
>   net/axgbe: adjust return value of xstats-get ops
>   ethdev: fix memory leak when telemetry xstats
>   ethdev: fix possible null pointer access
>   net/cnxk: fix telemetry possible null pointer access
> 
> ---

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v4 0/9] bugfix for ethdev telemetry

[Andrew Rybchenko]

On 5/16/22 11:43, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
>> Sent: Friday, 13 May 2022 04.54
>>
>> This patch set contains nine bugfix for ethdev telemetry.
>>
>> Chengwen Feng (9):
>>    ethdev: specify return value of xstats-get API
>>    ethdev: optimize xstats-get API's implementation
>>    net/hns3: adjust return value of xstats-get ops
>>    net/ipn3ke: adjust return value of xstats-get ops
>>    net/mvpp2: adjust return value of xstats-get ops
>>    net/axgbe: adjust return value of xstats-get ops
>>    ethdev: fix memory leak when telemetry xstats
>>    ethdev: fix possible null pointer access
>>    net/cnxk: fix telemetry possible null pointer access
>>
>> ---
> 
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

Series-reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

I did some fixes in patches summary and description,
please, double-check in next-net.

Applied, thanks.