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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e2ee64c4-4923-4691-bcfd-df9222f2c30b@amd.com>
Date: Sun, 20 Jul 2025 23:58:49 +0530
From: "Badole, Vishal" <vishal.badole@....com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Shyam-sundar.S-k@....com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH net-next] amd-xgbe: Configure and retrieve
 'tx-usecs' for Tx coalescing



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@....com>
>> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@....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
>