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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 325B6C636D4 for ; Fri, 17 Feb 2023 12:32:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229672AbjBQMcv (ORCPT ); Fri, 17 Feb 2023 07:32:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38502 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229724AbjBQMct (ORCPT ); Fri, 17 Feb 2023 07:32:49 -0500 Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:32c8:5054:ff:fe00:142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6F19C2681 for ; Fri, 17 Feb 2023 04:32:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender: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-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=8b4TRasz9WS/99qQeBZUZqQjt7rW1bdnmmxCS6SHFaY=; b=zGp6GtiLyYK8mtNYyw9KDuIaLT ufnyfOi+0/B1A2fM0EbhWRgG7mW/wjhHAfby9AvuXdU4yiXdKnznykJEQMavJBCS0LCGv0TK9Bj6k Z0bISy9eFMjdQ9JBW5LYzHdnp9rSLfgqJKSBg7Svs85k8nXGNgUnPtCJZmcIFm0I74vz2BhD4qGxc 1o6cZtMpk9dshuJJIcBZK/AAsuQH/u6xyj2xOgnuQ0XFkz6Livs/dkY/UZsT5m3QlgJff7r/lKjrz U/BE3VO50/cOZImSuGX2XQpOTFgLK8GOp7tNxq1NsCccJIge5DIAx2YNOBiD+vEkwEKHkJbasSXA0 NMVQo8tA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:36490) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1pSzuq-0000vS-PX; Fri, 17 Feb 2023 12:32:40 +0000 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1pSzuk-0006nJ-Q7; Fri, 17 Feb 2023 12:32:34 +0000 Date: Fri, 17 Feb 2023 12:32:34 +0000 From: "Russell King (Oracle)" To: Oleksij Rempel Cc: Andrew Lunn , netdev , Florian Fainelli , Vladimir Oltean , Sean Wang , Landen Chao , DENG Qingfang , Matthias Brugger , AngeloGioacchino Del Regno , Doug Berger , Broadcom internal kernel review list , Wei Fang , Shenwei Wang , Clark Wang , NXP Linux Team , UNGLinuxDriver@microchip.com, Byungho An , Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , Maxime Coquelin , Heiner Kallweit , Woojung Huh , Oleksij Rempel Subject: Re: [PATCH RFC 08/18] net: FEC: Fixup EEE Message-ID: References: <20230217034230.1249661-1-andrew@lunn.ch> <20230217034230.1249661-9-andrew@lunn.ch> <20230217081943.GA9065@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230217081943.GA9065@pengutronix.de> Sender: Russell King (Oracle) Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Feb 17, 2023 at 09:19:43AM +0100, Oleksij Rempel wrote: > On Fri, Feb 17, 2023 at 04:42:20AM +0100, Andrew Lunn wrote: > > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) > > return -ENETDOWN; > > > > p->tx_lpi_timer = edata->tx_lpi_timer; > > - > > - if (!edata->eee_enabled || !edata->tx_lpi_enabled || > > - !edata->tx_lpi_timer) > > - ret = fec_enet_eee_mode_set(ndev, false); > > - else > > - ret = fec_enet_eee_mode_set(ndev, true); > > - > > - if (ret) > > - return ret; > > + p->tx_lpi_enabled = edata->tx_lpi_enabled; > > Hm.. this change have effect only after link restart. Should we do > something like this? > > if (phydev->link) > fec_enet_eee_mode_set(ndev, phydev->eee_active); > > or, execute phy_ethtool_set_eee() first and some how detect if link > changed? Or restart link by phylib on every change? Restarting the link on every "change" (even when nothing has changed) is a dirty hack, and can be very annoying, leading to multiple link restarts e.g. at boot time which can turn into several seconds of boot delay depending on how much is configured. I would suggest as a general principle, we should be keeping link renegotiations to a minimum - and phylib already tries to do that in several places. Also note that reading phydev->eee_active without holding the phydev mutex can be racy - and I would also ask what would prevent two calls to fec_enet_eee_mode_set() running concurrently, once by the adjust_link callback and once via the set_eee method. This applies to reading phydev->link as well, so it may be best to hold the phydev mutex around that entire if() clause. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!