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 512D5C3DA4A for ; Tue, 20 Aug 2024 12:36:34 +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=/VHGZt1R2aqOcWKdCxk8PVxhDicABM+R1yiEYakU1s0=; b=nsUXid+pFyWUMlPHnDk47W+Gdw Z9lRU1o7an9ZNtG2Oi6OCg3gvR+wRfDfTcGHR9YwqJT+9MyhepyOw8Sc+Wq3fvnVDkK1L6R2SQ6mx nu2RQ1HHjQOdamw2SPpLa2+ah993bk1UkOwetEcsEgCOCnSHxF9YdqOC575q/tSGXflGpn0e5Pt3u PrrJpGYaxhn6ccwwQ8kRmf59GgbDvk647sDPKDd1RBtdbqb/VLxN42hrxt2Ds9eyo5WEqztZC+Pvr /DE9UWCFXL6HUMWK9G73EBYZcjLyhiVO4KYNq8Xadw25I7NCW0v0ea4lSQtePYrtmrvSxHsyGVch9 oChLhoqA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgO5z-00000005E0s-1avP; Tue, 20 Aug 2024 12:36:19 +0000 Received: from mail-ed1-x52c.google.com ([2a00:1450:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgO4l-00000005Dg6-1X2t for linux-arm-kernel@lists.infradead.org; Tue, 20 Aug 2024 12:35:06 +0000 Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-5bed83487aeso4133254a12.2 for ; Tue, 20 Aug 2024 05:35:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724157301; x=1724762101; 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=/VHGZt1R2aqOcWKdCxk8PVxhDicABM+R1yiEYakU1s0=; b=kxHc0W0B6P9AE9nTosFCv6XEjHmH5EMY6iaNZJhsBIpkqB6R6KU/EIRVzW0qcesBYH PUsCKtmyb/3l0lEzPAmT+HsZzBrMmx77LuZa3llUQvb+xAr7RrKUfd/RwvFaUZNdyJyD 16Gqm0tJFIK5Ycf50qmNMcvdxKfI/vjmr1zrAqrnpMhAl5MtHtiCEz50btWFnwo0iI2s BAF+ZF2nzzgNLH1P2cKGNtb2ZzTOGp+GOaMr31S2FKZmbMifl5C40BnXl9xv4q2DSadw 553UCMBf6FhKM3vTmLgeVWwR6+R+rViDDoIYUmqc8savXoKvXidMAZIXG2MQjq/+z3hr nhyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724157301; x=1724762101; 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=/VHGZt1R2aqOcWKdCxk8PVxhDicABM+R1yiEYakU1s0=; b=MzAzFneG/B1N2n9xRhU/17NSSuuhtl5Iv6CKLLKH1RJyjE72NsqcMnShzfrLQrkjnX wtTsPSp3o1VJFhdyFW98wWig3qEVfHRdyInzDXOjBNdnIDO6Gc2QNBCKY7vHxuiJSy97 AqkMB6BUWdNdFxEAN0MDtFAJtqGmlcExR5A3Vcv4SOqiLBNJrfaHzmbzfN6kvAqw+lmG Nk0vEWkXpOOaxSVZxLVXNhMNMWkjRI+Yca+L/dkBVCE0ib2QUpdHeuYJ/y0FTHprlyMK Jw87zSOG+2nE78++Owftvc6p59s9EDlkrZvh1XPEpmlXd2/fmAgc2LL5OQDnDUIHoCwm Vl/w== X-Forwarded-Encrypted: i=1; AJvYcCW8ExCyjOC/o6Df1Fgd0QalVuC5vrWYr8KSbuzwloWnUhJFw0HjaKDGl+3jg2s1brjG/7V/6FMa4fYNDhEzofAP@lists.infradead.org X-Gm-Message-State: AOJu0YziFcCMzC5oYM4GTrumIugbgrARnaXpSSsXg3oRhSCtTHQkSo6L wszFF2dLSG5hF8NymZi1i/8jmLOmcefMmRNun8K+W+tv8oaG6lXB X-Google-Smtp-Source: AGHT+IF6uYBW9R6jzN702jDQ9saKget2CSs108Txn3oN6qQ6VWBBn4r2ghfXbAlhLU9xpNVAIt0uWw== X-Received: by 2002:a05:6402:11c9:b0:5be:ffe1:6539 with SMTP id 4fb4d7f45d1cf-5beffe1666fmr3978232a12.24.1724157300170; Tue, 20 Aug 2024 05:35:00 -0700 (PDT) Received: from skbuf ([188.25.134.29]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5bec82e5eacsm5909592a12.19.2024.08.20.05.34.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Aug 2024 05:34:59 -0700 (PDT) Date: Tue, 20 Aug 2024 15:34:56 +0300 From: Vladimir Oltean To: Furong Xu <0x1207@gmail.com> Cc: Serge Semin , 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 Subject: Re: [PATCH net-next v4 3/7] net: stmmac: refactor FPE verification process Message-ID: <20240820123456.qbt4emjdjg5pouym@skbuf> References: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="n5bc2hehdh25qze3" Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240820_053503_503350_E5D194EE X-CRM114-Status: GOOD ( 43.65 ) 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 --n5bc2hehdh25qze3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Aug 20, 2024 at 05:38:31PM +0800, Furong Xu wrote: > Drop driver defined stmmac_fpe_state, and switch to common > ethtool_mm_verify_status for local TX verification status. > > Local side and remote side verification processes are completely > independent. There is no reason at all to keep a local state and > a remote state. > > Add a spinlock to avoid races among ISR, workqueue, link update > and register configuration. > > Signed-off-by: Furong Xu <0x1207@gmail.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 21 +-- > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 172 ++++++++++-------- > .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 6 - > 3 files changed, 102 insertions(+), 97 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index 458d6b16ce21..407b59f2783f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -146,14 +146,6 @@ struct stmmac_channel { > u32 index; > }; > > -/* FPE link state */ > -enum stmmac_fpe_state { > - FPE_STATE_OFF = 0, > - FPE_STATE_CAPABLE = 1, > - FPE_STATE_ENTERING_ON = 2, > - FPE_STATE_ON = 3, > -}; > - > /* FPE link-partner hand-shaking mPacket type */ > enum stmmac_mpacket_type { > MPACKET_VERIFY = 0, > @@ -166,11 +158,16 @@ enum stmmac_fpe_task_state_t { > }; > > struct stmmac_fpe_cfg { > - bool enable; /* FPE enable */ > - bool hs_enable; /* FPE handshake enable */ > - enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */ > - enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */ > + /* Serialize access to MAC Merge state between ethtool requests > + * and link state updates. > + */ > + spinlock_t lock; > + > u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */ > + u32 verify_time; /* see ethtool_mm_state */ > + bool pmac_enabled; /* see ethtool_mm_state */ > + bool verify_enabled; /* see ethtool_mm_state */ > + enum ethtool_mm_verify_status status; > }; > > struct stmmac_tc_entry { > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 3072ad33b105..6ae95f20b24f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -969,17 +969,21 @@ static void stmmac_mac_config(struct phylink_config *config, unsigned int mode, > static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) > { > struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; > - 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; > + unsigned long flags; > + > + spin_lock_irqsave(&priv->fpe_cfg.lock, flags); > + > + if (!fpe_cfg->pmac_enabled) > + goto __unlock_out; > > - if (is_up && *hs_enable) { > + if (is_up && fpe_cfg->verify_enabled) > stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, > MPACKET_VERIFY); > - } else { > - *lo_state = FPE_STATE_OFF; > - *lp_state = FPE_STATE_OFF; > - } > + else > + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; The fpe_task may be scheduled here. When you unlock, it may run and overwrite the fpe_cfg->status you've just set. Although I don't actually recommend setting ETHTOOL_MM_VERIFY_STATUS_DISABLED unless cfg->verify_enabled=false. > + > +__unlock_out: > + spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); > } > > static void stmmac_mac_link_down(struct phylink_config *config, > @@ -4091,11 +4095,25 @@ static int stmmac_release(struct net_device *dev) > > stmmac_release_ptp(priv); > > - pm_runtime_put(priv->device); > - > - if (priv->dma_cap.fpesel) > + if (priv->dma_cap.fpesel) { > stmmac_fpe_stop_wq(priv); > > + /* stmmac_ethtool_ops.begin() guarantees that all ethtool > + * requests to fail with EBUSY when !netif_running() > + * > + * Prepare some params here, then fpe_cfg can keep consistent > + * with the register states after a SW reset by __stmmac_open(). > + */ > + priv->fpe_cfg.pmac_enabled = false; > + priv->fpe_cfg.verify_enabled = false; > + priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; > + > + /* Reset MAC_FPE_CTRL_STS reg cache */ > + priv->fpe_cfg.fpe_csr = 0; > + } With this block of code, you're saying that you're deliberately okay for the ethtool-mm state to be lost after a stmmac_release() call. Mind you, some of the call sites of this are: - stmmac_change_mtu() - stmmac_reinit_queues() - stmmac_reinit_ringparam() I disagree that it's okay to lose the state configured by user space. Instead, you should reprogram the saved state once lost. Note that because stmmac_release() calls phylink_stop(), I think that restoring the state in stmmac_fpe_link_state_handle() is enough. Because there will always be a link drop. > + > + pm_runtime_put(priv->device); > + > return 0; > } > > @@ -5979,44 +5997,34 @@ static int stmmac_set_features(struct net_device *netdev, > static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status) > { > struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; > - 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; > > - if (status == FPE_EVENT_UNKNOWN || !*hs_enable) > - return; > + spin_lock(&priv->fpe_cfg.lock); > > - /* If LP has sent verify mPacket, LP is FPE capable */ > - if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER) { > - if (*lp_state < FPE_STATE_CAPABLE) > - *lp_state = FPE_STATE_CAPABLE; > + if (!fpe_cfg->pmac_enabled || status == FPE_EVENT_UNKNOWN) > + goto __unlock_out; > > - /* If user has requested FPE enable, quickly response */ > - if (*hs_enable) > - stmmac_fpe_send_mpacket(priv, priv->ioaddr, > - fpe_cfg, > - MPACKET_RESPONSE); > - } > + /* LP has sent verify mPacket */ > + if ((status & FPE_EVENT_RVER) == FPE_EVENT_RVER) > + stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, > + MPACKET_RESPONSE); > > - /* If Local has sent verify mPacket, Local is FPE capable */ > - if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER) { > - if (*lo_state < FPE_STATE_CAPABLE) > - *lo_state = FPE_STATE_CAPABLE; > - } > + /* Local has sent verify mPacket */ > + if ((status & FPE_EVENT_TVER) == FPE_EVENT_TVER && > + fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED) > + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; > > - /* If LP has sent response mPacket, LP is entering FPE ON */ > + /* LP has sent response mPacket */ > if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP) > - *lp_state = FPE_STATE_ENTERING_ON; > - > - /* If Local has sent response mPacket, Local is entering FPE ON */ > - if ((status & FPE_EVENT_TRSP) == FPE_EVENT_TRSP) > - *lo_state = FPE_STATE_ENTERING_ON; > + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; > > if (!test_bit(__FPE_REMOVING, &priv->fpe_task_state) && > !test_and_set_bit(__FPE_TASK_SCHED, &priv->fpe_task_state) && > priv->fpe_wq) { > queue_work(priv->fpe_wq, &priv->fpe_task); > } > + > +__unlock_out: > + spin_unlock(&priv->fpe_cfg.lock); > } > > static void stmmac_common_interrupt(struct stmmac_priv *priv) > @@ -7372,50 +7380,57 @@ int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size) > return ret; > } > > -#define SEND_VERIFY_MPAKCET_FMT "Send Verify mPacket lo_state=%d lp_state=%d\n" > -static void stmmac_fpe_lp_task(struct work_struct *work) > +static void stmmac_fpe_verify_task(struct work_struct *work) > { > struct stmmac_priv *priv = container_of(work, struct stmmac_priv, > fpe_task); > struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; > - 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) { > - /* Bail out immediately if FPE handshake is OFF */ > - if (*lo_state == FPE_STATE_OFF || !*hs_enable) > + int verify_limit = 3; /* defined by 802.3 */ > + unsigned long flags; > + u32 sleep_ms; > + > + spin_lock(&priv->fpe_cfg.lock); > + sleep_ms = fpe_cfg->verify_time; > + spin_unlock(&priv->fpe_cfg.lock); > + > + while (1) { > + /* The initial VERIFY was triggered by linkup event or > + * stmmac_set_mm(), sleep then check MM_VERIFY_STATUS. > + */ > + msleep(sleep_ms); Thanks for the added comment. But why don't you just use queue_delayed_work() instead of queue_work() and sleeping at the very beginning? With this, you really don't need to drop the lock and read fpe_cfg->verify_time twice. But I think what is needed here is better suited for a timer, especially because of the required coordination with the IRQ. See the end and the attachment for more details. > + > + if (!netif_running(priv->dev)) > break; > > - 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); > + spin_lock_irqsave(&priv->fpe_cfg.lock, flags); > > - netdev_info(priv->dev, "configured FPE\n"); > + if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_DISABLED || > + fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED || > + !fpe_cfg->pmac_enabled || !fpe_cfg->verify_enabled) { > + spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); > + break; > + } > > - *lo_state = FPE_STATE_ON; > - *lp_state = FPE_STATE_ON; > - netdev_info(priv->dev, "!!! BOTH FPE stations ON\n"); > + if (verify_limit == 0) { > + fpe_cfg->verify_enabled = false; I don't understand why turn off verify_enabled after failure? Only the user should be able to modify this. > + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED; > + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, > + priv->plat->tx_queues_to_use, > + priv->plat->rx_queues_to_use, > + false); I don't understand why turn off tx_enabled after failure, rather than not turning it on at all until success? This really has me thinking. This hardware does not have the explicit notion of the verification state - it is purely a driver construct. So I wonder if the EFPE bit in MAC_FPE_CTRL_STS isn't actually what corresponds to "tx_active" rather than "tx_enabled"? (definitions at https://docs.kernel.org/networking/ethtool-netlink.html) And "tx_enabled" would just correspond to a state variable in the driver, which does nothing until verification is actually complete. There is a test in manual_failed_verification() which checks the correctness of the tx_enabled/tx_active behavior. If tx_enabled=true but verification fails (and also _up until_ that point), the MM layer is supposed to send packets through the eMAC (because tx_active=false). But for your driver, that test is inconclusive, because you don't report ethtool stats broken down by eMAC/pMAC, just aggregate. So we don't know unless we take a closer look manually at the driver in that state. > + spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); > break; > } > > - if ((*lo_state == FPE_STATE_CAPABLE || > - *lo_state == FPE_STATE_ENTERING_ON) && > - *lp_state != FPE_STATE_ON) { > - netdev_info(priv->dev, SEND_VERIFY_MPAKCET_FMT, > - *lo_state, *lp_state); > - stmmac_fpe_send_mpacket(priv, priv->ioaddr, > - fpe_cfg, > + if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING) > + stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, > MPACKET_VERIFY); > - } > - /* Sleep then retry */ > - msleep(500); > + > + sleep_ms = fpe_cfg->verify_time; > + > + spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); > + > + verify_limit--; > } I took the liberty of rewriting the fpe_task to a timer, and delete the workqueue. Here is a completely untested patch, which at least is less complex, has less code and is easier to understand. What do you think? --n5bc2hehdh25qze3 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-net-stmmac-replace-FPE-workqueue-with-timer.patch" >From 6ce277245128638160385d948583a3e6d2561a94 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 20 Aug 2024 14:50:32 +0300 Subject: [PATCH] net: stmmac: replace FPE workqueue with timer What remains in the fpe_task after decoupling RX from TX appears overengineered to use a workqueue. A timer which retransmits Verify mPackets until the verify_limit expires, or enables transmission on success, seems enough. In the INITIAL state, the timer sends MPACKET_VERIFY. Eventually the stmmac_fpe_event_status() IRQ fires and advances the state to VERIFYING, then rearms the timer after verify_time ms. If a subsequent IRQ comes in and modifies the state to SUCCEEDED after getting MPACKET_RESPONSE, the timer sees this. It must enable the EFPE bit now. Otherwise, it decrements the verify_limit counter and tries again. Eventually it moves the status to FAILED, from which the IRQ cannot move it anywhere else, except for another stmmac_fpe_apply() call. Signed-off-by: Vladimir Oltean --- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 16 +- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 35 +-- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 212 +++++++----------- 3 files changed, 100 insertions(+), 163 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index 407b59f2783f..dd15f71e1663 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -152,21 +152,18 @@ enum stmmac_mpacket_type { MPACKET_RESPONSE = 1, }; -enum stmmac_fpe_task_state_t { - __FPE_REMOVING, - __FPE_TASK_SCHED, -}; - struct stmmac_fpe_cfg { /* Serialize access to MAC Merge state between ethtool requests * and link state updates. */ spinlock_t lock; - + struct timer_list verify_timer; u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */ u32 verify_time; /* see ethtool_mm_state */ bool pmac_enabled; /* see ethtool_mm_state */ bool verify_enabled; /* see ethtool_mm_state */ + bool tx_enabled; + int verify_limit; enum ethtool_mm_verify_status status; }; @@ -364,10 +361,6 @@ struct stmmac_priv { struct work_struct service_task; /* Frame Preemption feature (FPE) */ - unsigned long fpe_task_state; - struct workqueue_struct *fpe_wq; - struct work_struct fpe_task; - char wq_name[IFNAMSIZ + 4]; struct stmmac_fpe_cfg fpe_cfg; /* TC Handling */ @@ -422,7 +415,8 @@ bool stmmac_eee_init(struct stmmac_priv *priv); int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt); int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size); int stmmac_bus_clks_config(struct stmmac_priv *priv, bool enabled); -void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable); +void stmmac_fpe_apply(struct stmmac_priv *priv); +void stmmac_fpe_verify_timer_arm(struct stmmac_fpe_cfg *fpe_cfg); static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv) { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index a8cdcacecc26..3eb5344e2412 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -1293,14 +1293,7 @@ static int stmmac_get_mm(struct net_device *ndev, * variable has a range between 1 and 128 ms inclusive. Limit to that. */ state->max_verify_time = 128; - - /* Cannot read MAC_FPE_CTRL_STS register here, or FPE interrupt events - * can be lost. - * - * See commit 37e4b8df27bc ("net: stmmac: fix FPE events losing") - */ - state->tx_enabled = !!(priv->fpe_cfg.fpe_csr == EFPE); - + state->tx_enabled = priv->fpe_cfg.tx_enabled; /* 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 || @@ -1326,34 +1319,28 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, if (!priv->dma_cap.fpesel) return -EOPNOTSUPP; - /* Wait for the fpe_task that's currently in progress to finish */ - flush_workqueue(priv->fpe_wq); - err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size, &add_frag_size, extack); if (err) return err; - spin_lock_irqsave(&priv->fpe_cfg.lock, flags); + /* Wait for the verification that's currently in progress to finish */ + del_timer_sync(&fpe_cfg->verify_timer); + + spin_lock_irqsave(&fpe_cfg->lock, flags); fpe_cfg->pmac_enabled = cfg->pmac_enabled; + fpe_cfg->tx_enabled = cfg->tx_enabled; fpe_cfg->verify_time = cfg->verify_time; fpe_cfg->verify_enabled = cfg->verify_enabled; - - stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, - priv->plat->tx_queues_to_use, - priv->plat->rx_queues_to_use, - cfg->tx_enabled, cfg->pmac_enabled); + fpe_cfg->verify_limit = 3; /* IEEE 802.3 constant */ + if (!cfg->verify_enabled) + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; stmmac_fpe_set_add_frag_size(priv, priv->ioaddr, add_frag_size); + stmmac_fpe_apply(priv); - if (cfg->verify_enabled) - stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, - MPACKET_VERIFY); - else - fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; - - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); + spin_unlock_irqrestore(&fpe_cfg->lock, flags); return 0; } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index a5d01162fcc5..fa74504f3ad5 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -971,19 +971,22 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; unsigned long flags; - spin_lock_irqsave(&priv->fpe_cfg.lock, flags); + del_timer_sync(&fpe_cfg->verify_timer); - if (!fpe_cfg->pmac_enabled) - goto __unlock_out; + spin_lock_irqsave(&fpe_cfg->lock, flags); - if (is_up && fpe_cfg->verify_enabled) - stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, - MPACKET_VERIFY); - else - fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; + if (is_up) { + /* New link => maybe new partner => new verification process */ + stmmac_fpe_apply(priv); + } else { + /* No link => turn off EFPE */ + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, + priv->plat->tx_queues_to_use, + priv->plat->rx_queues_to_use, + false, false); + } -__unlock_out: - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); + spin_unlock_irqrestore(&fpe_cfg->lock, flags); } static void stmmac_mac_link_down(struct phylink_config *config, @@ -3362,27 +3365,6 @@ static void stmmac_safety_feat_configuration(struct stmmac_priv *priv) } } -static int stmmac_fpe_start_wq(struct stmmac_priv *priv) -{ - char *name; - - clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state); - clear_bit(__FPE_REMOVING, &priv->fpe_task_state); - - name = priv->wq_name; - sprintf(name, "%s-fpe", priv->dev->name); - - priv->fpe_wq = create_singlethread_workqueue(name); - if (!priv->fpe_wq) { - netdev_err(priv->dev, "%s: Failed to create workqueue\n", name); - - return -ENOMEM; - } - netdev_dbg(priv->dev, "FPE workqueue start"); - - return 0; -} - /** * stmmac_hw_setup - setup mac in a usable state. * @dev : pointer to the device structure. @@ -3537,22 +3519,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) stmmac_set_hw_vlan_mode(priv, priv->hw); - if (priv->dma_cap.fpesel) { - /* A SW reset just happened in stmmac_init_dma_engine(), - * we should restore fpe_cfg to HW, or FPE will stop working - * from suspend/resume. - */ - spin_lock(&priv->fpe_cfg.lock); - stmmac_fpe_configure(priv, priv->ioaddr, - &priv->fpe_cfg, - priv->plat->tx_queues_to_use, - priv->plat->rx_queues_to_use, - false, priv->fpe_cfg.pmac_enabled); - spin_unlock(&priv->fpe_cfg.lock); - - stmmac_fpe_start_wq(priv); - } - return 0; } @@ -4049,18 +4015,6 @@ static int stmmac_open(struct net_device *dev) return ret; } -static void stmmac_fpe_stop_wq(struct stmmac_priv *priv) -{ - set_bit(__FPE_REMOVING, &priv->fpe_task_state); - - if (priv->fpe_wq) { - destroy_workqueue(priv->fpe_wq); - priv->fpe_wq = NULL; - } - - netdev_dbg(priv->dev, "FPE workqueue stop"); -} - /** * stmmac_release - close entry point of the driver * @dev : device pointer. @@ -4108,22 +4062,8 @@ static int stmmac_release(struct net_device *dev) stmmac_release_ptp(priv); - if (priv->dma_cap.fpesel) { - stmmac_fpe_stop_wq(priv); - - /* stmmac_ethtool_ops.begin() guarantees that all ethtool - * requests to fail with EBUSY when !netif_running() - * - * Prepare some params here, then fpe_cfg can keep consistent - * with the register states after a SW reset by __stmmac_open(). - */ - priv->fpe_cfg.pmac_enabled = false; - priv->fpe_cfg.verify_enabled = false; - priv->fpe_cfg.status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; - - /* Reset MAC_FPE_CTRL_STS reg cache */ - priv->fpe_cfg.fpe_csr = 0; - } + if (priv->dma_cap.fpesel) + del_timer_sync(&priv->fpe_cfg.verify_timer); pm_runtime_put(priv->device); @@ -6030,11 +5970,7 @@ static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status) if ((status & FPE_EVENT_RRSP) == FPE_EVENT_RRSP) fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; - if (!test_bit(__FPE_REMOVING, &priv->fpe_task_state) && - !test_and_set_bit(__FPE_TASK_SCHED, &priv->fpe_task_state) && - priv->fpe_wq) { - queue_work(priv->fpe_wq, &priv->fpe_task); - } + stmmac_fpe_verify_timer_arm(fpe_cfg); __unlock_out: spin_unlock(&priv->fpe_cfg.lock); @@ -7395,60 +7331,82 @@ int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size) return ret; } -static void stmmac_fpe_verify_task(struct work_struct *work) +/** + * stmmac_fpe_verify_timer - Timer for MAC Merge verification + * @t: timer_list struct containing private info + * + * Verify the MAC Merge capability in the local TX direction, by + * transmitting Verify mPackets up to 3 times. Wait until link + * partner responds with a Response mPacket, otherwise fail. + */ +static void stmmac_fpe_verify_timer(struct timer_list *t) { - struct stmmac_priv *priv = container_of(work, struct stmmac_priv, - fpe_task); - struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; - int verify_limit = 3; /* defined by 802.3 */ - unsigned long flags; - u32 sleep_ms; + struct stmmac_fpe_cfg *fpe_cfg = from_timer(fpe_cfg, t, verify_timer); + struct stmmac_priv *priv = container_of(fpe_cfg, struct stmmac_priv, + fpe_cfg); + bool rearm = false; - spin_lock(&priv->fpe_cfg.lock); - sleep_ms = fpe_cfg->verify_time; - spin_unlock(&priv->fpe_cfg.lock); + spin_lock(&fpe_cfg->lock); - while (1) { - /* The initial VERIFY was triggered by linkup event or - * stmmac_set_mm(), sleep then check MM_VERIFY_STATUS. - */ - msleep(sleep_ms); - - if (!netif_running(priv->dev)) - break; - - spin_lock_irqsave(&priv->fpe_cfg.lock, flags); - - if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_DISABLED || - fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED || - !fpe_cfg->pmac_enabled || !fpe_cfg->verify_enabled) { - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); - break; - } - - if (verify_limit == 0) { - fpe_cfg->verify_enabled = false; + switch (fpe_cfg->status) { + case ETHTOOL_MM_VERIFY_STATUS_INITIAL: + case ETHTOOL_MM_VERIFY_STATUS_VERIFYING: + stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, + MPACKET_VERIFY); + if (fpe_cfg->verify_limit != 0) { + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; + rearm = true; + } else { fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_FAILED; - stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, - priv->plat->tx_queues_to_use, - priv->plat->rx_queues_to_use, - false, fpe_cfg->pmac_enabled); - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); - break; } + fpe_cfg->verify_limit--; + break; + case ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED: + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, + priv->plat->tx_queues_to_use, + priv->plat->rx_queues_to_use, + true, true); + break; + default: + break; + } - if (fpe_cfg->status == ETHTOOL_MM_VERIFY_STATUS_VERIFYING) - stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg, - MPACKET_VERIFY); - - sleep_ms = fpe_cfg->verify_time; + if (rearm) { + mod_timer(&fpe_cfg->verify_timer, + msecs_to_jiffies(fpe_cfg->verify_time)); + } - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags); + spin_unlock(&fpe_cfg->lock); +} - verify_limit--; +void stmmac_fpe_verify_timer_arm(struct stmmac_fpe_cfg *fpe_cfg) +{ + if (fpe_cfg->pmac_enabled && fpe_cfg->tx_enabled && + fpe_cfg->verify_enabled && + fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_FAILED && + fpe_cfg->status != ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED) { + mod_timer(&fpe_cfg->verify_timer, + msecs_to_jiffies(fpe_cfg->verify_time)); } +} - clear_bit(__FPE_TASK_SCHED, &priv->fpe_task_state); +void stmmac_fpe_apply(struct stmmac_priv *priv) +{ + struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg; + + /* If verification is disabled, configure FPE right away. + * Otherwise let the timer code do it. + */ + if (!fpe_cfg->verify_enabled) { + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, + priv->plat->tx_queues_to_use, + priv->plat->rx_queues_to_use, + fpe_cfg->tx_enabled, + fpe_cfg->pmac_enabled); + } else { + fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL; + stmmac_fpe_verify_timer_arm(fpe_cfg); + } } static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) @@ -7565,9 +7523,6 @@ int stmmac_dvr_probe(struct device *device, INIT_WORK(&priv->service_task, stmmac_service_task); - /* Initialize FPE verify workqueue */ - INIT_WORK(&priv->fpe_task, stmmac_fpe_verify_task); - /* Override with kernel parameters if supplied XXX CRS XXX * this needs to have multiple instances */ @@ -7733,6 +7688,7 @@ int stmmac_dvr_probe(struct device *device, mutex_init(&priv->lock); spin_lock_init(&priv->fpe_cfg.lock); + timer_setup(&priv->fpe_cfg.verify_timer, stmmac_fpe_verify_timer, 0); priv->fpe_cfg.pmac_enabled = false; priv->fpe_cfg.verify_time = 128; /* ethtool_mm_state.max_verify_time */ priv->fpe_cfg.verify_enabled = false; @@ -7912,7 +7868,7 @@ int stmmac_suspend(struct device *dev) rtnl_unlock(); if (priv->dma_cap.fpesel) - stmmac_fpe_stop_wq(priv); + del_timer_sync(&priv->fpe_cfg.verify_timer); priv->speed = SPEED_UNKNOWN; return 0; -- 2.34.1 --n5bc2hehdh25qze3--