| 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
| ||
|
Message-ID: <06e29d07-e492-4093-88d5-af91c9060a99@amd.com>
Date: Fri, 25 Jul 2025 14:29:16 +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/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@....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.
>
> 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
>>>
>>
>
Powered by blists - more mailing lists