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

[RESEND PATCH 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>
---
 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..362f8623433a 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) {
+		netdev_err(netdev,
+			   "tx_usecs and tx_frames must not be 0 together\n");
+		return -EINVAL;
+	}
+
 	/* Check the bounds of values for Tx */
+	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
+		netdev_err(netdev, "tx-usecs is limited to %d usec\n",
+			   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: [RESEND PATCH net-next] amd-xgbe: Configure and retrieve 'tx-usecs' for Tx coalescing

[Vadim Fedorenko]

On 19.07.2025 08:26, 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>
> ---
>   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..362f8623433a 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) {
> +		netdev_err(netdev,
> +			   "tx_usecs and tx_frames must not be 0 together\n");
> +		return -EINVAL;
> +	}
> +
>   	/* Check the bounds of values for Tx */
> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
> +		netdev_err(netdev, "tx-usecs is limited to %d usec\n",
> +			   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);
>

I'm not quite sure, but it looks like it never works. config_tx_coalesce()
callback equals to xgbe_config_tx_coalesce() which is implemented as:

static int xgbe_config_tx_coalesce(struct xgbe_prv_data *pdata)
{
         return 0;
}

How is it expected to change anything from HW side?

> @@ -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

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

[Badole, Vishal]

On 7/19/2025 8:46 PM, Vadim Fedorenko wrote:
> On 19.07.2025 08:26, 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>
>> ---
>>   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..362f8623433a 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) {
>> +        netdev_err(netdev,
>> +               "tx_usecs and tx_frames must not be 0 together\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       /* Check the bounds of values for Tx */
>> +    if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
>> +        netdev_err(netdev, "tx-usecs is limited to %d usec\n",
>> +               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);
>>
> 
> I'm not quite sure, but it looks like it never works. config_tx_coalesce()
> callback equals to xgbe_config_tx_coalesce() which is implemented as:
> 
> static int xgbe_config_tx_coalesce(struct xgbe_prv_data *pdata)
> {
>          return 0;
> }
> 
> How is it expected to change anything from HW side?
> 

The code analysis reveals that pdata, a pointer to xgbe_prv_data, is 
obtained via netdev_priv(netdev). The tx_usecs member of the 
xgbe_prv_data structure is then updated with the user-specified value 
through this pdata pointer. This updated tx_usecs value propagates 
throughout the codebase wherever TX coalescing functionality is referenced.

We have validated this behavior through log analysis and transmission 
timestamps, confirming the parameter updates are taking effect.

Since this is a legacy driver implementation where 
xgbe_config_tx_coalesce() currently lacks actual hardware configuration 
logic for TX coalescing parameters, we plan to modernize the xgbe driver 
and eliminate redundant code segments in future releases.

>> @@ -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
> 

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

[Vadim Fedorenko]

On 20.07.2025 19:28, Badole, Vishal wrote:
> 
> 
> On 7/19/2025 8:46 PM, Vadim Fedorenko wrote:
>> On 19.07.2025 08:26, 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>
>>> ---
>>>   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..362f8623433a 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) {
>>> +        netdev_err(netdev,
>>> +               "tx_usecs and tx_frames must not be 0 together\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       /* Check the bounds of values for Tx */
>>> +    if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
>>> +        netdev_err(netdev, "tx-usecs is limited to %d usec\n",
>>> +               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);
>>>
>>
>> I'm not quite sure, but it looks like it never works. config_tx_coalesce()
>> callback equals to xgbe_config_tx_coalesce() which is implemented as:
>>
>> static int xgbe_config_tx_coalesce(struct xgbe_prv_data *pdata)
>> {
>>          return 0;
>> }
>>
>> How is it expected to change anything from HW side?
>>
> 
> The code analysis reveals that pdata, a pointer to xgbe_prv_data, is obtained 
> via netdev_priv(netdev). The tx_usecs member of the xgbe_prv_data structure is 
> then updated with the user-specified value through this pdata pointer. This 
> updated tx_usecs value propagates throughout the codebase wherever TX coalescing 
> functionality is referenced.
> 
> We have validated this behavior through log analysis and transmission 
> timestamps, confirming the parameter updates are taking effect.
> 
> Since this is a legacy driver implementation where xgbe_config_tx_coalesce() 
> currently lacks actual hardware configuration logic for TX coalescing 
> parameters, we plan to modernize the xgbe driver and eliminate redundant code 
> segments in future releases.

Effectively, when the user asks for the coalescing configuration, the driver 
reports values which are not really HW-configured values. At the same time
driver reports correct configuration even though the configuration is not
actually supported by the driver and it doesn't configure HW. This sounds odd.

Why didn't you start with the actual implementation instead of doing this
useless copying of values?


> 
>>> @@ -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
>>
> 

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

[Badole, Vishal]

On 7/21/2025 3:05 AM, Vadim Fedorenko wrote:
> On 20.07.2025 19:28, Badole, Vishal wrote:
>>
>>
>> On 7/19/2025 8:46 PM, Vadim Fedorenko wrote:
>>> On 19.07.2025 08:26, 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>
>>>> ---
>>>>   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..362f8623433a 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) {
>>>> +        netdev_err(netdev,
>>>> +               "tx_usecs and tx_frames must not be 0 together\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>>       /* Check the bounds of values for Tx */
>>>> +    if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
>>>> +        netdev_err(netdev, "tx-usecs is limited to %d usec\n",
>>>> +               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);
>>>>
>>>
>>> I'm not quite sure, but it looks like it never works. 
>>> config_tx_coalesce()
>>> callback equals to xgbe_config_tx_coalesce() which is implemented as:
>>>
>>> static int xgbe_config_tx_coalesce(struct xgbe_prv_data *pdata)
>>> {
>>>          return 0;
>>> }
>>>
>>> How is it expected to change anything from HW side?
>>>
>>
>> The code analysis reveals that pdata, a pointer to xgbe_prv_data, is 
>> obtained via netdev_priv(netdev). The tx_usecs member of the 
>> xgbe_prv_data structure is then updated with the user-specified value 
>> through this pdata pointer. This updated tx_usecs value propagates 
>> throughout the codebase wherever TX coalescing functionality is 
>> referenced.
>>
>> We have validated this behavior through log analysis and transmission 
>> timestamps, confirming the parameter updates are taking effect.
>>
>> Since this is a legacy driver implementation where 
>> xgbe_config_tx_coalesce() currently lacks actual hardware 
>> configuration logic for TX coalescing parameters, we plan to modernize 
>> the xgbe driver and eliminate redundant code segments in future releases.
> 
> Effectively, when the user asks for the coalescing configuration, the 
> driver reports values which are not really HW-configured values. At the 
> same time
> driver reports correct configuration even though the configuration is not
> actually supported by the driver and it doesn't configure HW. This 
> sounds odd.
> 
> Why didn't you start with the actual implementation instead of doing this
> useless copying of values?
> 
> 
Since the XGMAC controller does not provide hardware-level register 
support for tx_usecs-based TX interrupt coalescing, the driver employs 
an advanced timer-driven software implementation to achieve interrupt 
batching and improve performance. The tx_usecs parameter is accessible 
through the pdata structure pointer, allowing dynamic updates that 
automatically influence the TX coalescing timer mechanism across the 
driver implementation.
>>
>>>> @@ -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
>>>
>>
> 

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

[Jakub Kicinski]

On Sat, 19 Jul 2025 12:56:08 +0530 Vishal Badole wrote:
> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
> +	if (!tx_usecs && !tx_frames) {
> +		netdev_err(netdev,
> +			   "tx_usecs and tx_frames must not be 0 together\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Check the bounds of values for Tx */
> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
> +		netdev_err(netdev, "tx-usecs is limited to %d usec\n",
> +			   XGMAC_MAX_COAL_TX_TICK);
> +		return -EINVAL;
> +	}

Please use extack to report the error to the user rather than system
logs.
-- 
pw-bot: cr

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

[Badole, Vishal]

On 7/26/2025 6:37 AM, Jakub Kicinski wrote:
> On Sat, 19 Jul 2025 12:56:08 +0530 Vishal Badole wrote:
>> +	/* Check if both tx_usecs and tx_frames are set to 0 simultaneously */
>> +	if (!tx_usecs && !tx_frames) {
>> +		netdev_err(netdev,
>> +			   "tx_usecs and tx_frames must not be 0 together\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	/* Check the bounds of values for Tx */
>> +	if (tx_usecs > XGMAC_MAX_COAL_TX_TICK) {
>> +		netdev_err(netdev, "tx-usecs is limited to %d usec\n",
>> +			   XGMAC_MAX_COAL_TX_TICK);
>> +		return -EINVAL;
>> +	}
> 
> Please use extack to report the error to the user rather than system
> logs.

Thank you for your observations. Since this driver is quite old, we have 
used netdev_err() to report errors to maintain consistency. In the 
future, we plan to upgrade the driver to use netlink interfaces with 
extack parameters for returning error information to user-space.

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

[Jakub Kicinski]

On Sat, 19 Jul 2025 12:56:08 +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.

If you're reviving this patch in PW I strongly advise you to stop.