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

["Badole, Vishal" <vishal.badole@amd.com>]

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