| 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: <20250306004301.evw34gqoyll36mso@skbuf>
Date: Thu, 6 Mar 2025 02:43:01 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Simon Horman <horms@...nel.org>,
Russell King <linux@...linux.org.uk>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Furong Xu <0x1207@...il.com>,
Russell King <rmk+kernel@...linux.org.uk>,
Serge Semin <fancer.lancer@...il.com>,
Xiaolei Wang <xiaolei.wang@...driver.com>,
Suraj Jaiswal <quic_jsuraj@...cinc.com>,
Kory Maincent <kory.maincent@...tlin.com>,
Gal Pressman <gal@...dia.com>,
Jesper Nilsson <jesper.nilsson@...s.com>,
Choong Yong Liang <yong.liang.choong@...ux.intel.com>,
Chwee-Lin Choong <chwee.lin.choong@...el.com>,
Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
Vinicius Costa Gomes <vinicius.gomes@...el.com>,
intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, bpf@...r.kernel.org
Subject: Re: [PATCH iwl-next v8 08/11] igc: add support to set
tx-min-frag-size
On Wed, Mar 05, 2025 at 08:00:23AM -0500, Faizal Rahim wrote:
> Add support to set tx-min-frag-size via set_mm callback in igc.
> Increase the max limit of tx-ming-frag-size in ethtool from 252 to 256
> since i225/6 value range is 64, 128, 192 and 256.
>
> Co-developed-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 1 +
> drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 5 +++
> drivers/net/ethernet/intel/igc/igc_tsn.c | 37 ++++++++++++++++++--
> drivers/net/ethernet/intel/igc/igc_tsn.h | 2 +-
> net/ethtool/mm.c | 2 +-
> 6 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index d9ecb7cf80c9..4dfd133b4d6f 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -42,6 +42,7 @@ void igc_ethtool_set_ops(struct net_device *);
>
> struct igc_fpe_t {
> struct ethtool_mmsv mmsv;
> + u32 tx_min_frag_size;
> };
>
> enum igc_mac_filter_type {
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 22db1de02964..038ee89f1e08 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -551,6 +551,7 @@
> #define IGC_TQAVCTRL_PREEMPT_ENA 0x00000002
> #define IGC_TQAVCTRL_ENHANCED_QAV 0x00000008
> #define IGC_TQAVCTRL_FUTSCDDIS 0x00000080
> +#define IGC_TQAVCTRL_MIN_FRAG_MASK 0x0000C000
>
> #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT 0x00000001
> #define IGC_TXQCTL_STRICT_CYCLE 0x00000002
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index b64d5c6c1d20..529654ccd83f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1789,6 +1789,11 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
> struct igc_adapter *adapter = netdev_priv(netdev);
> struct igc_fpe_t *fpe = &adapter->fpe;
>
> + fpe->tx_min_frag_size = igc_fpe_get_supported_frag_size(cmd->tx_min_frag_size);
> + if (fpe->tx_min_frag_size != cmd->tx_min_frag_size)
> + NL_SET_ERR_MSG_MOD(extack,
> + "tx-min-frag-size value set is unsupported. Rounded up to supported value (64, 128, 192, 256)");
> +
> if (fpe->mmsv.pmac_enabled != cmd->pmac_enabled) {
> if (cmd->pmac_enabled)
> static_branch_inc(&igc_fpe_enabled);
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index 0a2c747fde2d..2ec5909bf8b0 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
> @@ -6,6 +6,12 @@
> #include "igc_hw.h"
> #include "igc_tsn.h"
>
> +#define MIN_MULTPLIER_TX_MIN_FRAG 0
> +#define MAX_MULTPLIER_TX_MIN_FRAG 3
> +/* Frag size is based on the Section 8.12.2 of the SW User Manual */
> +#define TX_MIN_FRAG_SIZE 64
> +#define TX_MAX_FRAG_SIZE (TX_MIN_FRAG_SIZE * (MAX_MULTPLIER_TX_MIN_FRAG + 1))
> +
> DEFINE_STATIC_KEY_FALSE(igc_fpe_enabled);
>
> static int igc_fpe_init_smd_frame(struct igc_ring *ring,
> @@ -128,6 +134,7 @@ static const struct ethtool_mmsv_ops igc_mmsv_ops = {
>
> void igc_fpe_init(struct igc_adapter *adapter)
> {
> + adapter->fpe.tx_min_frag_size = TX_MIN_FRAG_SIZE;
> ethtool_mmsv_init(&adapter->fpe.mmsv, adapter->netdev, &igc_mmsv_ops);
> }
>
> @@ -278,7 +285,7 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
> tqavctrl = rd32(IGC_TQAVCTRL);
> tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
> IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS |
> - IGC_TQAVCTRL_PREEMPT_ENA);
> + IGC_TQAVCTRL_PREEMPT_ENA | IGC_TQAVCTRL_MIN_FRAG_MASK);
>
> wr32(IGC_TQAVCTRL, tqavctrl);
>
> @@ -324,12 +331,34 @@ static void igc_tsn_set_retx_qbvfullthreshold(struct igc_adapter *adapter)
> wr32(IGC_RETX_CTL, retxctl);
> }
>
> +static u8 igc_fpe_get_frag_size_mult(const struct igc_fpe_t *fpe)
> +{
> + u8 mult = (fpe->tx_min_frag_size / TX_MIN_FRAG_SIZE) - 1;
> +
> + return clamp_t(u8, mult, MIN_MULTPLIER_TX_MIN_FRAG,
> + MAX_MULTPLIER_TX_MIN_FRAG);
> +}
> +
> +u32 igc_fpe_get_supported_frag_size(u32 frag_size)
> +{
> + const u32 supported_sizes[] = {64, 128, 192, 256};
> +
> + /* Find the smallest supported size that is >= frag_size */
> + for (int i = 0; i < ARRAY_SIZE(supported_sizes); i++) {
> + if (frag_size <= supported_sizes[i])
> + return supported_sizes[i];
> + }
> +
> + return TX_MAX_FRAG_SIZE; /* Should not happen, value > 256 is blocked by ethtool */
Try to place comments on separate lines from code.
> +}
> +
> static int igc_tsn_enable_offload(struct igc_adapter *adapter)
> {
> struct igc_hw *hw = &adapter->hw;
> u32 tqavctrl, baset_l, baset_h;
> u32 sec, nsec, cycle, rxpbs;
> ktime_t base_time, systim;
> + u32 frag_size_mult;
> int i;
>
> wr32(IGC_TSAUXC, 0);
> @@ -501,13 +530,15 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
> }
>
> tqavctrl = rd32(IGC_TQAVCTRL) & ~(IGC_TQAVCTRL_FUTSCDDIS |
> - IGC_TQAVCTRL_PREEMPT_ENA);
> -
> + IGC_TQAVCTRL_PREEMPT_ENA | IGC_TQAVCTRL_MIN_FRAG_MASK);
> tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
>
> if (adapter->fpe.mmsv.pmac_enabled)
> tqavctrl |= IGC_TQAVCTRL_PREEMPT_ENA;
>
> + frag_size_mult = igc_fpe_get_frag_size_mult(&adapter->fpe);
> + tqavctrl |= FIELD_PREP(IGC_TQAVCTRL_MIN_FRAG_MASK, frag_size_mult);
> +
> adapter->qbv_count++;
>
> cycle = adapter->cycle_time;
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h
> index a2534228cc0e..975f4e38836e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_tsn.h
> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h
> @@ -14,7 +14,7 @@ enum igc_txd_popts_type {
> DECLARE_STATIC_KEY_FALSE(igc_fpe_enabled);
>
> void igc_fpe_init(struct igc_adapter *adapter);
> -u32 igc_fpe_get_supported_frag_size(u32 user_frag_size);
> +u32 igc_fpe_get_supported_frag_size(u32 frag_size);
The "-" piece shouldn't exist. You are renaming a function argument for
a function declaration that shouldn't have existed in the code prior to
the introduction of its definition. Please delete it from the original
patch that added it.
> int igc_tsn_offload_apply(struct igc_adapter *adapter);
> int igc_tsn_reset(struct igc_adapter *adapter);
> void igc_tsn_adjust_txtime_offset(struct igc_adapter *adapter);
> diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
> index ad9b40034003..4c395cd949ab 100644
> --- a/net/ethtool/mm.c
> +++ b/net/ethtool/mm.c
> @@ -153,7 +153,7 @@ const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1] = {
> [ETHTOOL_A_MM_VERIFY_TIME] = NLA_POLICY_RANGE(NLA_U32, 1, 128),
> [ETHTOOL_A_MM_TX_ENABLED] = NLA_POLICY_MAX(NLA_U8, 1),
> [ETHTOOL_A_MM_PMAC_ENABLED] = NLA_POLICY_MAX(NLA_U8, 1),
> - [ETHTOOL_A_MM_TX_MIN_FRAG_SIZE] = NLA_POLICY_RANGE(NLA_U32, 60, 252),
> + [ETHTOOL_A_MM_TX_MIN_FRAG_SIZE] = NLA_POLICY_RANGE(NLA_U32, 60, 256),
Please make this a separate patch with a reasonably convincing
justification for any reader, and also state why it is a change that
will not introduce regressions to the other drivers. It shows that
you've done the due dilligence of checking that they all use
ethtool_mm_frag_size_min_to_add(), which errors out on non-standard
values.
To be clear, extending the policy from 252 to 256 is just to suppress
the netlink warning which states that the driver rounds up the minimum
fragment size, correct? Because even if you pass 252 (the current
netlink maximum), the driver will still use 256.
> };
>
> static void mm_state_to_cfg(const struct ethtool_mm_state *state,
> --
> 2.34.1
>
Powered by blists - more mailing lists