From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8EA0CC3DA4A for ; Thu, 1 Aug 2024 20:05:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=svzaLRly6/piQEhmTsRu32eA5qubsY2cEZkb7yILYP4=; b=EWBI6Ce5FdguuXW6h8ZyzVo6UQ dEF2MDh8cv4alllXDz5acmkAxmf+J6FezRotrgdlVXg6lC0bMj2AKtWPGeCZ2OkIngHjA006e5eiH CJyqQS5xXL2/LNunI3mBiCDQC78KGNs+N2IVTy/re2IKWlbjzwJqi/DcTym+Ab503LVAUx7Me388k i1F2PUwsYAH149TAEDBnyalR+QDQB6uUhf4lDHH0uMMCVHJ4eErvGch5fuLMGiR+YtBnuEIupIXci RWmQySg7I5ySLsgEf809BTKGaUkxyHiEesBogNBu4lga5gFGg4ZgMcmD2pDxEBXxoZ+Mi+ouqdJaL GxWdQVUg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZc2i-00000006c7h-39Hu; Thu, 01 Aug 2024 20:04:56 +0000 Received: from mail-lj1-x22a.google.com ([2a00:1450:4864:20::22a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZc2D-00000006c23-25f9 for linux-arm-kernel@lists.infradead.org; Thu, 01 Aug 2024 20:04:27 +0000 Received: by mail-lj1-x22a.google.com with SMTP id 38308e7fff4ca-2ef2c56da6cso83665981fa.1 for ; Thu, 01 Aug 2024 13:04:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722542663; x=1723147463; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=svzaLRly6/piQEhmTsRu32eA5qubsY2cEZkb7yILYP4=; b=j7mTFsUlktuRF7CbCLyUELeAnb4SUF6fi6tD589FdQq0LnO6vjoGObPC+s/1El2w3K sCvKRcah8hIUu9q4C/4lEQI8WusmlxinxVIEeCZIdoXdvLIz2QQdcHCV9gp5L8gH6vJb KOUrjx2dBajnS7v4ZAL/qpxu2Z2D6l25JESpv2wkbLMJwN9jjOP0VCsTThCmUEK2E9vu gVLe7FtBJMm34H42MW+XTqS6xuN5ZPy50Lz7P7pJ0bMk0YGTYknmw5uLrxucitvfxj8k Ne6pfw3XmhhsjtA+q8CDclWbjGIoZeB6Yl0isXkaUMypX9377/y4IGZlT2/U5mjfoTLJ nTgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722542663; x=1723147463; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=svzaLRly6/piQEhmTsRu32eA5qubsY2cEZkb7yILYP4=; b=B8Jqu1ckyYpiLMgIj5yG0BQU3S1BmVKN/KDjEoRJEtEhqgB4uH3GUE4aF9oC4Ar6IV WRZ562XdTIOjQJLtVZba15jGKeOU7j7qLeT179KuxzGvZ2jJfN19BS+afgkg9/4htuPA svS2sNCypVQOI966S/Qf0wp1+St8uCmAG+M3uI8U5LV9wpd8Eo4sUjw3QMEADA+EzI3g kyIoWTi7dSNmHwZMXyT6w6GLT6CJoVGxbeG2hhpIC053p4GaeoYr5+Mdm4AfqrtFxlkB 1zvI156lYLNZCcUtNdMsQQMokhT/xQabz9lSKvWFlRzmIs4Aj656JEx/0Le60DrW+mKZ Z2Nw== X-Forwarded-Encrypted: i=1; AJvYcCVwgWHjWVY0B4VZwQFO2sk+xee4xJdDAvGCr4wEBQLmZvbaxAXqUm4wIthxxQcO16HCRoDkVpvH0uNyd9L+NXGe720H1Epa/VI0aP0g6KNvuSpLniA= X-Gm-Message-State: AOJu0Yx5p/aGTpkKFnRSO1E4qZmM03UR8XsgbUPd8uEsCtOLrV3yAdHV 3nHKd3VerW5D2iEMB3DCK4uJPPIj4XH+s9Ur5z3pUrjyNpiRacSw X-Google-Smtp-Source: AGHT+IF1rgY/ErrP3mP5eIig5nR3oDZkb6fvwAxelgQkhXiPc8SfFBiS5eCL9jtGfDknIKnyJ+0fFQ== X-Received: by 2002:a2e:b602:0:b0:2ef:2df7:3d9a with SMTP id 38308e7fff4ca-2f15aa84e17mr8653871fa.9.1722542662737; Thu, 01 Aug 2024 13:04:22 -0700 (PDT) Received: from skbuf ([188.25.135.70]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5b83bf3b99dsm153406a12.91.2024.08.01.13.04.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Aug 2024 13:04:21 -0700 (PDT) Date: Thu, 1 Aug 2024 23:04:19 +0300 From: Vladimir Oltean To: Furong Xu <0x1207@gmail.com> Cc: Andrew Lunn , "David S. Miller" , Alexandre Torgue , Jose Abreu , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , Joao Pinto , netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, xfr@outlook.com, rock.xu@nio.com Subject: Re: [PATCH net-next v1 1/5] net: stmmac: configure FPE via ethtool-mm Message-ID: <20240801200419.g3b264sjcc3njvwg@skbuf> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240801_130425_608389_037E3FC9 X-CRM114-Status: GOOD ( 37.37 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jul 31, 2024 at 06:43:12PM +0800, Furong Xu wrote: > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index b23b920eedb1..5228493bc68c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -345,6 +345,9 @@ struct stmmac_priv { > struct work_struct fpe_task; > char wq_name[IFNAMSIZ + 4]; > > + /* Serialize access to MAC Merge state between ethtool requests */ > + struct mutex mm_lock; The thing is, ethtool requests are already serialized by the rtnl_mutex. The purpose of a driver-level locking scheme is to serialize the ethtool requests with things like interrupts, work queues, etc, to avoid corrupting driver state. Surprisingly, even if the code runs indeed very much unlocked, I don't see too many races with severe consequences. There are some exceptions though. Like for example, stmmac_fpe_handshake(enable=false) races on priv->plat->fpe_cfg->lo_fpe_state and priv->plat->fpe_cfg->lp_fpe_state with stmmac_fpe_lp_task(). static void stmmac_fpe_lp_task(struct work_struct *work) { enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state; enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state; /* Bail out immediately if FPE handshake is OFF */ if (*lo_state == FPE_STATE_OFF || !*hs_enable) break; if (*lo_state == FPE_STATE_ENTERING_ON && *lp_state == FPE_STATE_ENTERING_ON) { netdev_dbg(priv->dev, "configured FPE\n"); Another CPU runs here: stmmac_set_mm() -> stmmac_fpe_handshake(enable=false) -> priv->plat->fpe_cfg->lo_fpe_state = FPE_STATE_OFF; -> priv->plat->fpe_cfg->lp_fpe_state = FPE_STATE_OFF; *lo_state = FPE_STATE_ON; *lp_state = FPE_STATE_ON; netdev_dbg(priv->dev, "!!! BOTH FPE stations ON\n"); break; } ... } Simply put, due to lack of locking, stmmac_set_mm() can try to stop the FPE verification task but fail, since it can lose the race. I would expect a way for stmmac_fpe_handshake() to be able to stop further FPE interrupts from taking place (or at least from being processed), and then flush_workqueue(&priv->fpe_task) to make sure that all pending stmmac_fpe_lp_task()s have finished. > + > /* TC Handling */ > unsigned int tc_entries_max; > unsigned int tc_off_max; > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index 7008219fd88d..ca85e8694285 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -19,6 +19,7 @@ > #include "stmmac.h" > #include "dwmac_dma.h" > #include "dwxgmac2.h" > +#include "dwmac5.h" > > #define REG_SPACE_SIZE 0x1060 > #define GMAC4_REG_SPACE_SIZE 0x116C > @@ -1270,6 +1271,113 @@ static int stmmac_set_tunable(struct net_device *dev, > return ret; > } > > +static int stmmac_get_mm(struct net_device *ndev, > + struct ethtool_mm_state *state) > +{ > + struct stmmac_priv *priv = netdev_priv(ndev); > + enum stmmac_fpe_state lo_state = priv->plat->fpe_cfg->lo_fpe_state; I expect this to have to serialize with the writers of lo_fpe_state on some sort of lock. May have to be a spinlock (making the priv->mm_lock mutex inadequate in its current form), since stmmac_interrupt() is not threaded and thus runs in atomic context. > + u32 add_frag_size; > + > + if (!priv->dma_cap.fpesel) > + return -EOPNOTSUPP; > + > + mutex_lock(&priv->mm_lock); > + > + /* If FPE is supported by hardware, preemptible MAC is always enabled */ > + state->pmac_enabled = true; Documentation/networking/ethtool-netlink.rst: "set if RX of preemptible and SMD-V frames is enabled". You can use the pmac_enabled knob as a hook to call stmmac_fpe_start_wq() and stmmac_fpe_stop_wq(), as well as whether to process FPE interrupt status bits. The idea being that as long as the pMAC is enabled, you are obliged to respond to mPackets from the link partner (to allow his side* of the verification process to run). Otherwise you are not - you should behave like a FPE-incapable NIC. In fact, this is what the manual_failed_verification() portion of the kselftest attempts to test. *This is actually a misconception of the current driver implementation. The two verification processes are completely independent, since each refers to its TX side only. One device can have ethtool --set-mm verify-enabled on, and the other off. There is no reason at all to keep a local device state + a link partner state, and to make any decision in the driver based on the LP state. Like here: stmmac_fpe_lp_task(): if (*lo_state == FPE_STATE_ENTERING_ON && *lp_state == FPE_STATE_ENTERING_ON) { // lp_state does not matter netdev_dbg(priv->dev, "configured FPE\n"); *lo_state = FPE_STATE_ON; *lp_state = FPE_STATE_ON; netdev_dbg(priv->dev, "!!! BOTH FPE stations ON\n"); break; } if ((*lo_state == FPE_STATE_CAPABLE || *lo_state == FPE_STATE_ENTERING_ON) && *lp_state != FPE_STATE_ON) { // lp_state does not matter netdev_dbg(priv->dev, SEND_VERIFY_MPAKCET_FMT, *lo_state, *lp_state); stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, MPACKET_VERIFY); } To my superficial reading of the driver, the distinction between FPE_STATE_ENTERING_ON and FPE_STATE_ON exists solely to wait for the LP to finish its verification as well. I think you noticed that is a non-goal as well, because your stmmac_get_mm() implementation reports ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED when we are in either of the 2 states: FPE_STATE_ENTERING_ON or FPE_STATE_ON. I hope you don't mind me if I say that since waiting both sides to finish verification is a non-goal, I disagree with your choice of papering over the existence of the 2 FPE driver states. I would recommend consolidating them into a single one. Another mistake relating to this (TX verification processes are not necessarily symmetric) is: stmmac_set_mm() -> stmmac_fpe_handshake(priv, cfg->verify_enabled) -> priv->plat->fpe_cfg->hs_enable = enable stmmac_fpe_event_status(): if (status == FPE_EVENT_UNKNOWN || !*hs_enable) return; if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER) { /* If LP has sent Verify mPacket, send back a Responds mPacket */ if (*hs_enable) stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, MPACKET_RESPONSE); } We should always respond to the link partner's Verify mPacket as long as pmac_enabled=true, and not just if hs_enable=true. > + > + state->verify_time = priv->plat->fpe_cfg->verify_time; > + > + /* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime > + * variable has a range between 1 and 128 ms inclusive. Limit to that. > + */ > + state->max_verify_time = 128; > + > + if (lo_state == FPE_STATE_CAPABLE) > + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; > + else if (lo_state == FPE_STATE_ENTERING_ON || lo_state == FPE_STATE_ON) > + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; > + else if (lo_state == FPE_STATE_OFF) > + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; > + else > + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN; > + > + /* Cannot read MAC_FPE_CTRL_STS register here, or FPE interrupt events > + * can lose. s/lose/be lost/ > + * > + * See commit 37e4b8df27bc ("net: stmmac: fix FPE events losing") > + */ > + state->tx_enabled = !!(priv->plat->fpe_cfg->fpe_csr == EFPE); > + > + /* FPE active if common tx_enabled and verification success or disabled (forced) */ > + state->tx_active = state->tx_enabled && > + (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED || > + state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED); > + > + state->verify_enabled = priv->plat->fpe_cfg->hs_enable; > + > + add_frag_size = stmmac_fpe_get_add_frag_size(priv, priv->ioaddr); > + state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(add_frag_size); > + > + state->rx_min_frag_size = ETH_ZLEN; > + > + mutex_unlock(&priv->mm_lock); > + > + return 0; > +} > + > +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); > + u32 add_frag_size; > + int err; > + > + if (!priv->dma_cap.fpesel) > + return -EOPNOTSUPP; > + > + err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size, > + &add_frag_size, extack); > + if (err) > + return err; > + > + mutex_lock(&priv->mm_lock); > + > + priv->plat->fpe_cfg->verify_time = cfg->verify_time; > + > + stmmac_fpe_configure(priv, priv->ioaddr, priv->plat->fpe_cfg, > + priv->plat->tx_queues_to_use, > + priv->plat->rx_queues_to_use, > + cfg->tx_enabled); > + > + stmmac_fpe_set_add_frag_size(priv, priv->ioaddr, add_frag_size); > + > + stmmac_fpe_handshake(priv, cfg->verify_enabled); > + > + mutex_unlock(&priv->mm_lock); > + > + return 0; > +} > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 12689774d755..9b1cf81c50ea 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -7384,7 +7384,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work) > enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state; > enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state; > bool *hs_enable = &fpe_cfg->hs_enable; > - bool *enable = &fpe_cfg->enable; > int retries = 20; > > while (retries-- > 0) { > @@ -7394,11 +7393,6 @@ static void stmmac_fpe_lp_task(struct work_struct *work) > > if (*lo_state == FPE_STATE_ENTERING_ON && > *lp_state == FPE_STATE_ENTERING_ON) { > - stmmac_fpe_configure(priv, priv->ioaddr, > - fpe_cfg, > - priv->plat->tx_queues_to_use, > - priv->plat->rx_queues_to_use, > - *enable); > > netdev_info(priv->dev, "configured FPE\n"); > > @@ -7418,7 +7412,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work) > MPACKET_VERIFY); > } > /* Sleep then retry */ > - msleep(500); > + msleep(fpe_cfg->verify_time); FWIW, if you want to follow the standard, I guess you should modify "retries" to 3 as well - this is the constant that 802.3 uses for verifyLimit. It helps make the verification process fail more predictably (within verifyTime * 3 ms). > } > > clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state); > @@ -7720,6 +7714,7 @@ int stmmac_dvr_probe(struct device *device, > stmmac_napi_add(ndev); > > mutex_init(&priv->lock); > + mutex_init(&priv->mm_lock); > > /* If a specific clk_csr value is passed from the platform > * this means that the CSR Clock Range selection cannot be