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 C0C4FCF649D for ; Mon, 30 Sep 2024 10:19:08 +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=9OlRk2qJffviTG3xiJHPw655zAjPrxKfaoMhCDVoxhU=; b=n1Q5lBRNK7TSB4gtZhe9J7cWhk Al8QxA21ZBfpkKKyvUOjolhucbQhi9R4E0hd0P5HjUMeVJIfdum59URUT6lbOlfs6mDkYSWhVAb/Z LYXv4b+mzrkX8tb8R19pQStcvAEXhf4tnXck+hh0teJcpd69ynq8XeE+e8jCV4N1aEUm8yvBC+0uF EruLWaQN/Lvfd6Mf8pW6Ig5D/Vrv1WWeDC6BZnd0WDCTw13oZzhR4qB7EfVz81VO4p5+3jpo94Zwx tEgTimKmED22X4+PC8BhLtFwVWV4s+wTrvbqRfUcuDy4ecAuL6yQFhnP20aEQYc2y4J5kgTbEhuC5 OUWP986Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1svDUV-0000000GgPH-3p2o; Mon, 30 Sep 2024 10:18:55 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1svDSN-0000000Gfx2-3GHK for linux-arm-kernel@lists.infradead.org; Mon, 30 Sep 2024 10:16:45 +0000 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=9OlRk2qJffviTG3xiJHPw655zAjPrxKfaoMhCDVoxhU=; b=rJ+/s2ZZ+r1qhJgR5sXdq8swui jsh1nMH1fR4V/oWf9FZXMwwI/Hk5nIBQyd3S1HfKYvC29P1K1iye7WHFjWYQVVMAEwD8R4YP2joJC AyP0nT9kdNN8pIzpO5LyUgufmVf9HADZ+gBuCTWQu0jMZeQfOqu511rej5iysuuHHt0b5ueYYx6hw pKvOhV3HWIGc7INL0rpdBLz61nVMLGXFJiSEVwwGQ+RbufWUgkJ0XAdLfbtlEdPdBwvBBK9LgLDhi QtYbKhUxGZYBcDE1uygEP/jJQ1R10aZEl3CMA37vcsPOGp0zPhS1UE5wBwL3uRo0DcvA3NpaEiuUe NDKjZSTw==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:44266) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1svDQ7-00041t-03; Mon, 30 Sep 2024 11:14:22 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.96) (envelope-from ) id 1svDPz-0003uN-0e; Mon, 30 Sep 2024 11:14:15 +0100 Date: Mon, 30 Sep 2024 11:14:15 +0100 From: "Russell King (Oracle)" To: Serge Semin Cc: Andrew Lunn , Heiner Kallweit , Alexandre Torgue , "David S. Miller" , Eric Dumazet , Florian Fainelli , Jakub Kicinski , Jiawen Wu , Jose Abreu , Jose Abreu , linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Maxime Coquelin , Mengyuan Lou , netdev@vger.kernel.org, Paolo Abeni , Vladimir Oltean Subject: Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config() Message-ID: 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-20240930_031643_846404_66488F5A X-CRM114-Status: GOOD ( 22.66 ) 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 Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote: > Hi Russell > > On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote: > > +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface) > > +{ > > + struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs); > > + const struct dw_xpcs_compat *compat; > > + int ret; > > + > > + if (!xpcs->need_reset) > > + return; > > + > > > + compat = xpcs_find_compat(xpcs->desc, interface); > > + if (!compat) { > > + dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n", > > + phy_modes(interface)); > > + return; > > + } > > Please note, it's better to preserve the xpcs_find_compat() call even > if the need_reset flag is false, since it makes sure that the > PHY-interface is actually supported by the PCS. Sorry, but I strongly disagree. xpcs_validate() will already have dealt with that, so we can be sure at this point that the interface is always valid. The NULL check is really only there because it'll stop the "you've forgotten to check for NULL on this function that can return NULL" brigade endlessly submitting patches to add something there - just like xpcs_get_state() and xpcs_do_config(). > > + bool need_reset; > > If you still prefer the PCS-reset being done in the pre_config() > function, then what about just directly checking the PMA id in there? > > if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) > return 0; > > return xpcs_soft_reset(xpcs); I think you've missed what "need_reset" is doing as you seem to think it's just to make it conditional on the PMA ID. That's only part of the story. In the existing code, the reset only happens _once_ when the create happens, not every time the PCS is configured. I am preserving this behaviour, because I do _NOT_ wish to incorporate multiple functional changes into one patch - and certainly in a cleanup series keep the number of functional changes to a minimum. So, all in all, I don't see the need to change anything in my patch. Thanks for the feedback anyway. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!