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 60090CD5BC7 for ; Thu, 5 Sep 2024 13:56:00 +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=j72SRMwZbgesZJhHX6WjWoZslyaQOKCacKBuJblmEuE=; b=woY/AQyUc0c0X/mNCEYaAmlqFm 8Fk8RXgtQgwGIzJLNPVtk7dlpeE+0k++pHscYej+I0vxu9+ltiySBcfQsNqgHJeozWtdKo1gycb6Q BupAqM1gJV6If1kXZTOjmMRP2KQr4qkOsAMPasK775mPc72/ad0AurauvBtp7vKeFu+UQG4NwG0Pi lylPKc5bz/gVO9zIy/1Utd289kpDk71PL59uEDSXFMRBVRXSbmgKg50vAn6iW2S8CCbXuzAqd4HUh A4PNCLCkO1fhu61z9I3pDgDhiIKEAGRnw+p/icmREY1byjp8pcjcFRzj0elK8SWxy9gGKM1pMqtPh EKtmXvLw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1smCxf-00000008bfF-2acE; Thu, 05 Sep 2024 13:55:47 +0000 Received: from mail-lf1-x12a.google.com ([2a00:1450:4864:20::12a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1smCwh-00000008bUZ-2S7u for linux-arm-kernel@lists.infradead.org; Thu, 05 Sep 2024 13:54:48 +0000 Received: by mail-lf1-x12a.google.com with SMTP id 2adb3069b0e04-5356a0a56f4so134213e87.3 for ; Thu, 05 Sep 2024 06:54:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1725544485; x=1726149285; 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=j72SRMwZbgesZJhHX6WjWoZslyaQOKCacKBuJblmEuE=; b=E/c4M1ve5EJjTMTjyrgbISS9OGGvc3B+B/0JBH88/GttyTqR34QfLzflTPn6MbazBL Fpb6inTaqFn3Sb/hTbW5QrXmZGpfEBBAwoQovTUOdTEXSK/u04Jpr3r1lF90jJICWerj ux7eF1AQU3zl4UnQZiNBLu+1HKaMwGl5Y4wv09bE8W/eND/yLJDf31eCjxmG8yJVeyfz yiPGWnSvmlxzmQacw/sAdbPh8F0nyFNhCY4xaXM1kaJdjyULrW8N/G6nU+XKbs3316Cp Gkpl0PMt5CVq6LXMO/zpfZ/y+5oPvEXJItPtZvuoAiIpKa1HEc5As5Lqkb1xU4pUWTtH 6czw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725544485; x=1726149285; 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=j72SRMwZbgesZJhHX6WjWoZslyaQOKCacKBuJblmEuE=; b=b0HM9kd0QM01y9B9Ow0uMvhSz/EJmfE3xnxbWCW1RnAArAkLf3szZVHE0Cwt4tinb1 yuqQUsErUqH7fUls3tB7l4DWHNr86cDDYW1ajP7eQwTsYcW494pZiuctIOF31UfSivnR vNN1m83j/xt/IOlIo/t4GYpiJNGdFKkquBNES9TOOG+jnFmXq77Dc0DhGJE2pdH1Iq9P 9WEs5GfBlZ7VD+uZafAkfGPbMEM55004PRpoQWVlTkxZWwYjMkrliJPruezLGlHoB7zf Gv8NxvT56K7eyWkXU31/gTv24+3ALbqFVeWZj4LSdSBiw0l+jiqum8BUBxBOCS1ujC9+ HcAg== X-Forwarded-Encrypted: i=1; AJvYcCWghZbu9lGOBF2M3qVPCnr6x50aPxmgDusEkm3EfpsBQE4ZnRkdzgKkIXGh+0t6tJRK0m5uTBSwt7o1NCiT5dJe@lists.infradead.org X-Gm-Message-State: AOJu0Yz/ZYANMdvz8JwPi2YposwV4v4vbYd6SuMu+D0g+BdyAmL92b+C e0M+yzcB5q1osfNtMdFCv967o64x8bKF66CR2VAeI2IXp5jiLTR0 X-Google-Smtp-Source: AGHT+IEGHceBUm9ge3LWUoJ0fD0Z2UOrlf5AZ1gk7JTSUBOffG9cHqN+jDgXYhMUztM4Iwbd35KQOw== X-Received: by 2002:ac2:5695:0:b0:536:54df:c000 with SMTP id 2adb3069b0e04-53654dfc031mr481122e87.8.1725544484197; Thu, 05 Sep 2024 06:54:44 -0700 (PDT) Received: from skbuf ([188.25.134.29]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8a7c662bcdsm33568166b.34.2024.09.05.06.54.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Sep 2024 06:54:43 -0700 (PDT) Date: Thu, 5 Sep 2024 16:54:40 +0300 From: Vladimir Oltean To: Furong Xu <0x1207@gmail.com> Cc: Alexander Lobakin , Serge Semin , "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, rmk+kernel@armlinux.org.uk, linux@armlinux.org.uk, xfr@outlook.com, Vladimir Oltean Subject: Re: [PATCH net-next v8 3/7] net: stmmac: refactor FPE verification process Message-ID: <20240905135440.hcgbva7fzic2x4ps@skbuf> References: <0b72fd0463b662796fd3eaa996211f1a5d0a4341.1725518135.git.0x1207@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0b72fd0463b662796fd3eaa996211f1a5d0a4341.1725518135.git.0x1207@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240905_065447_653752_EDCF7432 X-CRM114-Status: GOOD ( 32.24 ) 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 Thu, Sep 05, 2024 at 03:02:24PM +0800, Furong Xu wrote: > +void stmmac_fpe_apply(struct stmmac_priv *priv) > +{ > + struct ethtool_mm_state *state = &priv->fpe_cfg.state; > + 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 (!state->verify_enabled) { > + stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, > + priv->plat->tx_queues_to_use, > + priv->plat->rx_queues_to_use, > + state->tx_enabled, > + state->pmac_enabled); > + } else { > + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_INITIAL; > + fpe_cfg->verify_retries = STMMAC_FPE_MM_MAX_VERIFY_RETRIES; > + > + if (netif_device_present(priv->dev) && netif_running(priv->dev)) > + stmmac_fpe_verify_timer_arm(fpe_cfg); > + } > } In the cover letter, you say: 2. check netif_running() to guarantee synchronization rules between mod_timer() and timer_delete_sync() [ by the way, it would be nice if you could list the changes in individual patches as well ] but I guess this helps with something other than what you say it helps with. netif_running() essentially checks that __dev_open() has been called, aka "ip link set dev eth0 up". And I don't see the ethtool_ops :: begin() implemented by the driver any longer, so I think you've done this in order to accept stmmac_set_mm() calls even before the netdev has been brought operationally up. Okay. As for netif_device_present(), I don't know, maybe the intention was to suppress stmmac_set_mm() calls made after stmmac_suspend(). But ethnl_ops_begin() has its own netif_device_present() call, so I'm not sure why it is needed - they should already be suppressed. But in v7, I was thinking about the concurrency issues here: static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, struct netlink_ext_ack *extack) { /* Wait for the verification that's currently in progress to finish */ del_timer_sync(&fpe_cfg->verify_timer); <- Concurrent code can run here: stmmac_fpe_link_state_handle(), called from phylink_resolve() workqueue context, rtnl_lock() not held. spin_lock_irqsave(&fpe_cfg->lock, flags); stmmac_fpe_apply(priv); spin_unlock_irqrestore(&fpe_cfg->lock, flags); } 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; timer_delete_sync(&fpe_cfg->verify_timer); <- Concurrent code can run here: stmmac_set_mm() spin_lock_irqsave(&fpe_cfg->lock, flags); if (is_up && fpe_cfg->state.pmac_enabled) { /* VERIFY process requires pmac enabled when NIC comes up */ stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, priv->plat->tx_queues_to_use, priv->plat->rx_queues_to_use, false, true); /* 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); } spin_unlock_irqrestore(&fpe_cfg->lock, flags); } [ oh btw, you forgot to replace the del_timer_sync() instance from stmmac_set_mm() to timer_delete_sync() ] Because the timer can be restarted right after the timer_delete_sync() call, this is a half-baked implementation. I think at the end of the day, we need to ask ourselves: what is the timer_delete_sync() call even supposed to accomplish? What if the verify timer is allowed to run concurrently with us changing the settings? Well, for example, if it runs concurrently with stmmac_fpe_link_state_handle(is_down==false), it will not learn that the link is down, it will send an MPACKET_VERIFY, get no response, and fail. So, not very bad. And the other way around: stmmac_set_mm() stops the verify timer, but the link comes up, the timer is armed with the old settings, it does whatever (succeeds, fails), and only afterwards does stmmac_set_mm() manage to grab &fpe_cfg->lock, change the settings to the new ones, and re-trigger the verify timer once again, if needed. So bottom line, I think timer_delete_sync() is to avoid some useless work, but otherwise, it is not critical to have it. The choice is between removing the timer_delete_sync() calls from these 2 functions altogether, or implementing an actually effective mechanism to stop the timer for a while. I _think_ that the simplest way to stop it is to hold one more lock for the verify_timer when we call timer_delete_sync() and stmmac_fpe_verify_timer_arm(), lock which _is_ IRQ-safe, unlike &fpe_cfg->lock. static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, struct netlink_ext_ack *extack) { spin_lock(&fpe_cfg->verify_timer_lock); timer_delete_sync(&fpe_cfg->verify_timer); spin_lock_irqsave(&fpe_cfg->lock, flags); stmmac_fpe_apply(priv); spin_unlock_irqrestore(&fpe_cfg->lock, flags); spin_unlock(&fpe_cfg->verify_timer_lock); } static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) { spin_lock(&fpe_cfg->verify_timer_lock); timer_delete_sync(&fpe_cfg->verify_timer); spin_lock_irqsave(&fpe_cfg->lock, flags); if (is_up && fpe_cfg->state.pmac_enabled) { /* VERIFY process requires pmac enabled when NIC comes up */ stmmac_fpe_configure(priv, priv->ioaddr, fpe_cfg, priv->plat->tx_queues_to_use, priv->plat->rx_queues_to_use, false, true); /* 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); } spin_unlock_irqrestore(&fpe_cfg->lock, flags); spin_unlock(&fpe_cfg->verify_timer_lock); } Looking at the __timer_delete_sync() implementation, I don't think verify_timer_lock needs to be sleepable and hence a mutex (except on PREEMPT_RT where spinlocks are sleepable no matter what you do). But I think the implementation would be simpler without timer_delete_sync() in these 2 functions, and this overengineered mechanism. I would expect a comment in stmmac_release() here: if (priv->dma_cap.fpesel) timer_delete_sync(&priv->fpe_cfg.verify_timer); that timer restarts are not possible, because we have rtnl_lock() held and a concurrent stmmac_set_mm() cannot run now, and the earlier phylink_stop() has also ensured stmmac_fpe_link_state_handle() cannot run any longer. Similarly, I would like to see an explanation in the form of a comment for why timer restarts are not possible after the same pattern in stmmac_suspend(). The explanation is different there, I think.