[PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Vishal Badole]

Ethtool has advanced with additional configurable options, but the
current driver does not support tx-usecs configuration.

Add support to configure and retrieve 'tx-usecs' using ethtool, which
specifies the wait time before servicing an interrupt for Tx coalescing.

Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

---
v1 -> v2:
    * Replace netdev_err() with extack interface for user error reporting.
---
 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 19 +++++++++++++++++--
 drivers/net/ethernet/amd/xgbe/xgbe.h         |  1 +
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index 12395428ffe1..19cb1e2b7d92 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -450,6 +450,7 @@ static int xgbe_get_coalesce(struct net_device *netdev,
 	ec->rx_coalesce_usecs = pdata->rx_usecs;
 	ec->rx_max_coalesced_frames = pdata->rx_frames;
 
+	ec->tx_coalesce_usecs = pdata->tx_usecs;
 	ec->tx_max_coalesced_frames = pdata->tx_frames;
 
 	return 0;
@@ -463,7 +464,7 @@ static int xgbe_set_coalesce(struct net_device *netdev,
 	struct xgbe_prv_data *pdata = netdev_priv(netdev);
 	struct xgbe_hw_if *hw_if = &pdata->hw_if;
 	unsigned int rx_frames, rx_riwt, rx_usecs;
-	unsigned int tx_frames;
+	unsigned int tx_frames, tx_usecs;
 
 	rx_riwt = hw_if->usec_to_riwt(pdata, ec->rx_coalesce_usecs);
 	rx_usecs = ec->rx_coalesce_usecs;
@@ -485,9 +486,22 @@ static int xgbe_set_coalesce(struct net_device *netdev,
 		return -EINVAL;
 	}
 
+	tx_usecs = ec->tx_coalesce_usecs;
 	tx_frames = ec->tx_max_coalesced_frames;
 
+	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
+	if (!tx_usecs && !tx_frames) {
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "tx_usecs and tx_frames must not be 0 together");
+		return -EINVAL;
+	}
+
 	/* Check the bounds of values for Tx */
+	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
+		NL_SET_ERR_MSG_FMT_MOD(extack, "tx-usecs is limited to %d usec",
+				       XGMAC_MAX_COAL_TX_TICK);
+		return -EINVAL;
+	}
 	if (tx_frames > pdata->tx_desc_count) {
 		netdev_err(netdev, "tx-frames is limited to %d frames\n",
 			   pdata->tx_desc_count);
@@ -499,6 +513,7 @@ static int xgbe_set_coalesce(struct net_device *netdev,
 	pdata->rx_frames = rx_frames;
 	hw_if->config_rx_coalesce(pdata);
 
+	pdata->tx_usecs = tx_usecs;
 	pdata->tx_frames = tx_frames;
 	hw_if->config_tx_coalesce(pdata);
 
@@ -830,7 +845,7 @@ static int xgbe_set_channels(struct net_device *netdev,
 }
 
 static const struct ethtool_ops xgbe_ethtool_ops = {
-	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS |
+	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_drvinfo = xgbe_get_drvinfo,
 	.get_msglevel = xgbe_get_msglevel,
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
index 42fa4f84ff01..e330ae9ea685 100755
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -272,6 +272,7 @@
 /* Default coalescing parameters */
 #define XGMAC_INIT_DMA_TX_USECS		1000
 #define XGMAC_INIT_DMA_TX_FRAMES	25
+#define XGMAC_MAX_COAL_TX_TICK		100000
 
 #define XGMAC_MAX_DMA_RIWT		0xff
 #define XGMAC_INIT_DMA_RX_USECS		30
-- 
2.34.1

Re: [PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Hariprasad Kelam]

On 2025-08-12 at 10:20:35, Vishal Badole (Vishal.Badole@amd.com) wrote:
> Ethtool has advanced with additional configurable options, but the
> current driver does not support tx-usecs configuration.
> 
> Add support to configure and retrieve 'tx-usecs' using ethtool, which
> specifies the wait time before servicing an interrupt for Tx coalescing.
> 
> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> ---
> v1 -> v2:
>     * Replace netdev_err() with extack interface for user error reporting.
> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 19 +++++++++++++++++--
>  drivers/net/ethernet/amd/xgbe/xgbe.h         |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> index 12395428ffe1..19cb1e2b7d92 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> @@ -450,6 +450,7 @@ static int xgbe_get_coalesce(struct net_device *netdev,
>  	ec->rx_coalesce_usecs = pdata->rx_usecs;
>  	ec->rx_max_coalesced_frames = pdata->rx_frames;
>  
> +	ec->tx_coalesce_usecs = pdata->tx_usecs;
>  	ec->tx_max_coalesced_frames = pdata->tx_frames;
>  
>  	return 0;
> @@ -463,7 +464,7 @@ static int xgbe_set_coalesce(struct net_device *netdev,
>  	struct xgbe_prv_data *pdata = netdev_priv(netdev);
>  	struct xgbe_hw_if *hw_if = &pdata->hw_if;
>  	unsigned int rx_frames, rx_riwt, rx_usecs;
> -	unsigned int tx_frames;
> +	unsigned int tx_frames, tx_usecs;
>  
>  	rx_riwt = hw_if->usec_to_riwt(pdata, ec->rx_coalesce_usecs);
>  	rx_usecs = ec->rx_coalesce_usecs;
> @@ -485,9 +486,22 @@ static int xgbe_set_coalesce(struct net_device *netdev,
>  		return -EINVAL;
>  	}
>  
> +	tx_usecs = ec->tx_coalesce_usecs;
>  	tx_frames = ec->tx_max_coalesced_frames;
>  
> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
> +	if (!tx_usecs && !tx_frames) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "tx_usecs and tx_frames must not be 0 together");
> +		return -EINVAL;
> +	}
> +
>  	/* Check the bounds of values for Tx */
> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "tx-usecs is limited to %d usec",
> +				       XGMAC_MAX_COAL_TX_TICK);
> +		return -EINVAL;
> +	}
>  	if (tx_frames > pdata->tx_desc_count) {
>  		netdev_err(netdev, "tx-frames is limited to %d frames\n",
>  			   pdata->tx_desc_count);
> @@ -499,6 +513,7 @@ static int xgbe_set_coalesce(struct net_device *netdev,
>  	pdata->rx_frames = rx_frames;
>  	hw_if->config_rx_coalesce(pdata);
>  
> +	pdata->tx_usecs = tx_usecs;
>  	pdata->tx_frames = tx_frames;
>  	hw_if->config_tx_coalesce(pdata);
>  
> @@ -830,7 +845,7 @@ static int xgbe_set_channels(struct net_device *netdev,
>  }
>  
>  static const struct ethtool_ops xgbe_ethtool_ops = {
> -	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS |
> +	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_MAX_FRAMES,
>  	.get_drvinfo = xgbe_get_drvinfo,
>  	.get_msglevel = xgbe_get_msglevel,
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index 42fa4f84ff01..e330ae9ea685 100755
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -272,6 +272,7 @@
>  /* Default coalescing parameters */
>  #define XGMAC_INIT_DMA_TX_USECS		1000
>  #define XGMAC_INIT_DMA_TX_FRAMES	25
> +#define XGMAC_MAX_COAL_TX_TICK		100000
>  
>  #define XGMAC_MAX_DMA_RIWT		0xff
>  #define XGMAC_INIT_DMA_RX_USECS		30
> -- 
> 2.34.1
> 
Reviewed-by: Hariprasad Kelam <hkelam@marvell.com> 

Re: [PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Joe Damato]

On Tue, Aug 12, 2025 at 10:20:35AM +0530, Vishal Badole wrote:
> Ethtool has advanced with additional configurable options, but the
> current driver does not support tx-usecs configuration.
> 
> Add support to configure and retrieve 'tx-usecs' using ethtool, which
> specifies the wait time before servicing an interrupt for Tx coalescing.
> 
> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> ---
> v1 -> v2:
>     * Replace netdev_err() with extack interface for user error reporting.

In the future, it's a good idea to link to the previous revisions on
https://lore.kernel.org/netdev/ so that other reviewers can more easily look
back at the previous thread.

> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 19 +++++++++++++++++--
>  drivers/net/ethernet/amd/xgbe/xgbe.h         |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> index 12395428ffe1..19cb1e2b7d92 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c

[...]

> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
> +	if (!tx_usecs && !tx_frames) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "tx_usecs and tx_frames must not be 0 together");
> +		return -EINVAL;
> +	}
> +
>  	/* Check the bounds of values for Tx */
> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "tx-usecs is limited to %d usec",
> +				       XGMAC_MAX_COAL_TX_TICK);
> +		return -EINVAL;
> +	}
>  	if (tx_frames > pdata->tx_desc_count) {
>  		netdev_err(netdev, "tx-frames is limited to %d frames\n",
>  			   pdata->tx_desc_count);

Looks like there might be a future cleanup patch to use the extack interface
for user error reporting in the pre-existing code.

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Jakub Kicinski]

On Tue, 12 Aug 2025 10:20:35 +0530 Vishal Badole wrote:
> Ethtool has advanced with additional configurable options, but the
> current driver does not support tx-usecs configuration.

Not sure what you mean by this, perhaps:

  current driver does not even support tx-usecs configuration.

? tx-usecs is a very old tunable.

> Add support to configure and retrieve 'tx-usecs' using ethtool, which
> specifies the wait time before servicing an interrupt for Tx coalescing.
> 

> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
> +	if (!tx_usecs && !tx_frames) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "tx_usecs and tx_frames must not be 0 together");
> +		return -EINVAL;
> +	}
> +
>  	/* Check the bounds of values for Tx */
> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "tx-usecs is limited to %d usec",
> +				       XGMAC_MAX_COAL_TX_TICK);
> +		return -EINVAL;
> +	}

Normal configuration granularity for this parameter is in 10s of usecs.
You seem to be using a timer, so I think you should either round the
value up / down to what the jiffy resolution will give you or
reject configuration that's not expressible in jiffies (not a multiple
of jiffies_to_usecs(1)). Otherwise users may waste time turning this
knob by 100usec which will have zero effect.
-- 
pw-bot: cr

Re: [PATCH v2 net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Badole, Vishal]

On 8/14/2025 5:30 AM, Jakub Kicinski wrote:
> On Tue, 12 Aug 2025 10:20:35 +0530 Vishal Badole wrote:
>> Ethtool has advanced with additional configurable options, but the
>> current driver does not support tx-usecs configuration.
> 
> Not sure what you mean by this, perhaps:
> 
>    current driver does not even support tx-usecs configuration.
> 
> ? tx-usecs is a very old tunable.
> 
>> Add support to configure and retrieve 'tx-usecs' using ethtool, which
>> specifies the wait time before servicing an interrupt for Tx coalescing.
>>
Thanks for pointing that out. My intent was to highlight that while 
ethtool has gained more advanced and configurable options over time, the 
driver in its current form does not have support for tx-usecs 
configuration using Ethool.
I’ll reword the sentence to make this clearer in next patch version.>
>> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
>> +	if (!tx_usecs && !tx_frames) {
>> +		NL_SET_ERR_MSG_FMT_MOD(extack,
>> +				       "tx_usecs and tx_frames must not be 0 together");
>> +		return -EINVAL;
>> +	}
>> +
>>   	/* Check the bounds of values for Tx */
>> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
>> +		NL_SET_ERR_MSG_FMT_MOD(extack, "tx-usecs is limited to %d usec",
>> +				       XGMAC_MAX_COAL_TX_TICK);
>> +		return -EINVAL;
>> +	}
> 
> Normal configuration granularity for this parameter is in 10s of usecs.
> You seem to be using a timer, so I think you should either round the
> value up / down to what the jiffy resolution will give you or
> reject configuration that's not expressible in jiffies (not a multiple
> of jiffies_to_usecs(1)). Otherwise users may waste time turning this
> knob by 100usec which will have zero effect.

Good point — the hardware uses a timer, so sub-jiffy resolution won’t 
have any real effect. I’ll update the code in next patch version to 
round the tx-usecs value to the nearest multiple.