| 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: <20250304222122.67bjes6jzbiveqfk@skbuf>
Date: Wed, 5 Mar 2025 00:21:22 +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>,
Andrew Halaney <ahalaney@...hat.com>,
Choong Yong Liang <yong.liang.choong@...ux.intel.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 v7 1/9] net: ethtool: mm: extract stmmac
verification logic into common library
On Mon, Mar 03, 2025 at 05:26:50AM -0500, Faizal Rahim wrote:
> +/**
> + * ethtool_mmsv_link_state_handle() - Inform MAC Merge Software Verification
> + * of link state changes
> + * @mmsv: MAC Merge Software Verification state
> + * @up: True if device carrier is up and able to pass verification packets
> + *
> + * Calling context is expected to be from a task, interrupts enabled.
> + */
> +void ethtool_mmsv_link_state_handle(struct ethtool_mmsv *mmsv, bool up)
> +{
> + unsigned long flags;
> +
> + ethtool_mmsv_stop(mmsv);
> +
> + spin_lock_irqsave(&mmsv->lock, flags);
> +
> + if (up && mmsv->pmac_enabled) {
> + /* VERIFY process requires pMAC enabled when NIC comes up */
> + ethtool_mmsv_configure_pmac(mmsv, true);
> +
> + /* New link => maybe new partner => new verification process */
> + ethtool_mmsv_apply(mmsv);
> + } else {
> + /* Reset the reported verification state while the link is down */
> + if (mmsv->verify_enabled)
> + mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
As requested in v5, please make this a separate patch. It represents a
functional change for stmmac, and I don't want somebody who bisects it
to find a code movement commit and search for a needle through a haystack.
> +
> + /* No link or pMAC not enabled */
> + ethtool_mmsv_configure_pmac(mmsv, false);
> + ethtool_mmsv_configure_tx(mmsv, false);
> + }
> +
> + spin_unlock_irqrestore(&mmsv->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(ethtool_mmsv_link_state_handle);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 918a32f8fda8..25533d6a3175 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -1210,37 +1210,17 @@ static int stmmac_get_mm(struct net_device *ndev,
> struct ethtool_mm_state *state)
> {
> struct stmmac_priv *priv = netdev_priv(ndev);
> - unsigned long flags;
> u32 frag_size;
>
> if (!stmmac_fpe_supported(priv))
> return -EOPNOTSUPP;
>
> - spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
> + ethtool_mmsv_get_mm(&priv->fpe_cfg.mmsv, state);
>
> - state->max_verify_time = STMMAC_FPE_MM_MAX_VERIFY_TIME_MS;
> - state->verify_enabled = priv->fpe_cfg.verify_enabled;
> - state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
> - state->verify_time = priv->fpe_cfg.verify_time;
> - state->tx_enabled = priv->fpe_cfg.tx_enabled;
> - state->verify_status = priv->fpe_cfg.status;
> state->rx_min_frag_size = ETH_ZLEN;
> -
> - /* FPE active if common tx_enabled and
> - * (verification success or disabled(forced))
> - */
> - if (state->tx_enabled &&
> - (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
> - state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED))
> - state->tx_active = true;
> - else
> - state->tx_active = false;
> -
> frag_size = stmmac_fpe_get_add_frag_size(priv);
> state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(frag_size);
>
> - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
> -
> return 0;
> }
>
> @@ -1248,8 +1228,6 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> struct netlink_ext_ack *extack)
> {
> struct stmmac_priv *priv = netdev_priv(ndev);
> - struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> - unsigned long flags;
> u32 frag_size;
> int err;
>
> @@ -1258,23 +1236,8 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> if (err)
> return err;
>
> - /* Wait for the verification that's currently in progress to finish */
> - timer_shutdown_sync(&fpe_cfg->verify_timer);
> -
> - spin_lock_irqsave(&fpe_cfg->lock, flags);
> -
> - fpe_cfg->verify_enabled = cfg->verify_enabled;
> - fpe_cfg->pmac_enabled = cfg->pmac_enabled;
> - fpe_cfg->verify_time = cfg->verify_time;
> - fpe_cfg->tx_enabled = cfg->tx_enabled;
> -
> - if (!cfg->verify_enabled)
> - fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> -
> stmmac_fpe_set_add_frag_size(priv, frag_size);
This is another change in behavior which unfortunately I am noticing
only now: stmmac_fpe_set_add_frag_size() and stmmac_fpe_get_add_frag_size()
used to execute under &fpe_cfg->lock, and now run outside of what
eventually became the mmsv->lock (but still under rtnl_lock() protection).
I am not seeing a need for the additional fragment size to be covered
by the mmsv->lock (the mmsv has no business with this parameter), and
rtnl_lock() should be sufficient to serialize stmmac_get_mm() with
stmmac_set_mm(). So I guess I am ok with the code structure, but again,
please move stmmac_fpe_set_add_frag_size() and stmmac_fpe_get_add_frag_size()
outside of the spin_lock_irqsave(&fpe_cfg->lock) context (essentially to
their current positions) as a preliminary separate patch. Again, it is
best for a "git blame" caused by a behavior change here to not point to
a commit that moves code from one file to another.
> - stmmac_fpe_apply(priv);
> -
> - spin_unlock_irqrestore(&fpe_cfg->lock, flags);
> + ethtool_mmsv_set_mm(&priv->fpe_cfg.mmsv, cfg);
>
> return 0;
> }
Powered by blists - more mailing lists