[RFC 0/5] make PMD xstat_get alike

[RFC 0/5] make PMD xstat_get alike

[Stephen Hemminger]

The recent bug report about the txgbe driver motivated looking
at inconsistencies in drivers handling of xstats_get callback.
All drivers should treat call the same.

This is RFC since I don't have access to any of this hardware to test.

Stephen Hemminger (5):
  net/txgbe: fix query handling in xstats_get
  net/ngbe: fix query handling in xstats_get
  net/atlantic: fix handling of xstats_get
  net/octeontx: fix handling of xstats_get
  net/sfc: fix handling of xstats_get queries

 drivers/net/atlantic/atl_ethdev.c      |  2 +-
 drivers/net/ngbe/ngbe_ethdev.c         | 11 ++++-------
 drivers/net/octeontx/octeontx_ethdev.c |  3 +++
 drivers/net/sfc/sfc_ethdev.c           |  7 ++++---
 drivers/net/txgbe/txgbe_ethdev.c       | 12 +++---------
 5 files changed, 15 insertions(+), 20 deletions(-)

-- 
2.45.2

[RFC 1/5] net/txgbe: fix query handling in xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Bugzilla ID: 1560
Fixes: 91fe49c87d76 ("net/txgbe: support device xstats")
Cc: jiawenwu@trustnetic.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/txgbe/txgbe_ethdev.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 2834468764..ee829d9120 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -2547,19 +2547,13 @@ txgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
 	unsigned int i, count;
 
-	txgbe_read_stats_registers(hw, hw_stats);
-
-	/* If this is a reset xstats is NULL, and we have cleared the
-	 * registers by reading them.
-	 */
 	count = txgbe_xstats_calc_num(dev);
-	if (xstats == NULL)
+	if (limit < count)
 		return count;
 
-	limit = min(limit, txgbe_xstats_calc_num(dev));
-
+	txgbe_read_stats_registers(hw, hw_stats);
 	/* Extended stats from txgbe_hw_stats */
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < count; i++) {
 		uint32_t offset = 0;
 
 		if (txgbe_get_offset_by_id(i, &offset)) {
-- 
2.45.2

[RFC 2/5] net/ngbe: fix query handling in xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Bugzilla ID: 1560
Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
Cc: jiawenwu@trustnetic.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ngbe/ngbe_ethdev.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 6c45ffaad3..ee65829aa0 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -1693,19 +1693,16 @@ ngbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 	struct ngbe_hw_stats *hw_stats = NGBE_DEV_STATS(dev);
 	unsigned int i, count;
 
-	ngbe_read_stats_registers(hw, hw_stats);
-
-	/* If this is a reset xstats is NULL, and we have cleared the
-	 * registers by reading them.
-	 */
 	count = ngbe_xstats_calc_num(dev);
-	if (xstats == NULL)
+	if (limit < count)
 		return count;
 
+	ngbe_read_stats_registers(hw, hw_stats);
+
 	limit = min(limit, ngbe_xstats_calc_num(dev));
 
 	/* Extended stats from ngbe_hw_stats */
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < count; i++) {
 		uint32_t offset = 0;
 
 		if (ngbe_get_offset_by_id(i, &offset)) {
-- 
2.45.2

[RFC 3/5] net/atlantic: fix handling of xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: fbe059e87209 ("net/atlantic: implement device statistics")
Cc: pavel.belous@aquantia.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/atlantic/atl_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/atlantic/atl_ethdev.c b/drivers/net/atlantic/atl_ethdev.c
index 9cde935834..eac6f5a7b9 100644
--- a/drivers/net/atlantic/atl_ethdev.c
+++ b/drivers/net/atlantic/atl_ethdev.c
@@ -1029,7 +1029,7 @@ atl_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 	unsigned int i;
 	unsigned int count = atl_dev_xstats_get_count(dev);
 
-	if (!stats)
+	if (n < count)
 		return count;
 
 	if (hw->aq_fw_ops->send_macsec_req != NULL) {
-- 
2.45.2

[RFC 4/5] net/octeontx: fix handling of xstats_get

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: 5538990924f2 ("net/octeontx: add basic stats support")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/octeontx/octeontx_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 3b8d717797..51093cc815 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -1016,6 +1016,9 @@ octeontx_dev_xstats_get(struct rte_eth_dev *dev,
 	struct octeontx_nic *nic = octeontx_pmd_priv(dev);
 
 	PMD_INIT_FUNC_TRACE();
+	if (n < NUM_BGX_XSTAT)
+		return NUM_BGX_XSTAT;
+
 	return octeontx_port_xstats(nic, xstats, n);
 }
 
-- 
2.45.2

[RFC 5/5] net/sfc: fix handling of xstats_get queries

[Stephen Hemminger]

The xstats_get function in this driver did not act the same
as other drivers when queried. The correct check is to look
at the requested number of stats and compare it to the available
stats and if the request is too small, return the correct size.

Fixes: fdd7719eb3c1 ("net/sfc: add xstats for Rx/Tx doorbells")
Cc: ivan.ilchenko@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ethdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 3480a51642..f0710dab5c 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -833,11 +833,12 @@ sfc_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 {
 	struct sfc_adapter *sa = sfc_adapter_by_eth_dev(dev);
 	unsigned int nb_written = 0;
-	unsigned int nb_supported = 0;
+	unsigned int nb_supported;
 	int rc;
 
-	if (unlikely(xstats == NULL))
-		return sfc_xstats_get_nb_supported(sa);
+	nb_supported = sfc_xstats_get_nb_supported(sa);
+	if (xstats_count < nb_supported)
+		return nb_supported;
 
 	rc = sfc_port_get_mac_stats(sa, xstats, xstats_count, &nb_written);
 	if (rc < 0)
-- 
2.45.2

RE: [RFC 1/5] net/txgbe: fix query handling in xstats_get

[Jiawen Wu]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, October 9, 2024 12:00 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; jiawenwu@trustnetic.com
> Subject: [RFC 1/5] net/txgbe: fix query handling in xstats_get
> 
> The xstats_get function in this driver did not act the same
> as other drivers when queried. The correct check is to look
> at the requested number of stats and compare it to the available
> stats and if the request is too small, return the correct size.
> 
> Bugzilla ID: 1560
> Fixes: 91fe49c87d76 ("net/txgbe: support device xstats")
> Cc: jiawenwu@trustnetic.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/txgbe/txgbe_ethdev.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index 2834468764..ee829d9120 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -2547,19 +2547,13 @@ txgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>  	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
>  	unsigned int i, count;
> 
> -	txgbe_read_stats_registers(hw, hw_stats);
> -
> -	/* If this is a reset xstats is NULL, and we have cleared the
> -	 * registers by reading them.
> -	 */
>  	count = txgbe_xstats_calc_num(dev);
> -	if (xstats == NULL)
> +	if (limit < count)
>  		return count;
> 
> -	limit = min(limit, txgbe_xstats_calc_num(dev));
> -
> +	txgbe_read_stats_registers(hw, hw_stats);
>  	/* Extended stats from txgbe_hw_stats */
> -	for (i = 0; i < limit; i++) {
> +	for (i = 0; i < count; i++) {
>  		uint32_t offset = 0;
> 
>  		if (txgbe_get_offset_by_id(i, &offset)) {
> --
> 2.45.2
> 
> 

Thanks!
Reviewed-by: Jiawen Wu <jiawenwu@trustnetic.com>