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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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