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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2375C433DB for ; Wed, 10 Feb 2021 02:23:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A7A1564DE7 for ; Wed, 10 Feb 2021 02:23:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235432AbhBJCX2 (ORCPT ); Tue, 9 Feb 2021 21:23:28 -0500 Received: from vps0.lunn.ch ([185.16.172.187]:59100 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234459AbhBJBwY (ORCPT ); Tue, 9 Feb 2021 20:52:24 -0500 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1l9efG-005Dku-19; Wed, 10 Feb 2021 02:51:34 +0100 Date: Wed, 10 Feb 2021 02:51:34 +0100 From: Andrew Lunn To: Michael Walle Cc: bcm-kernel-feedback-list@broadcom.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Florian Fainelli , Heiner Kallweit , Russell King , "David S . Miller" , Jakub Kicinski Subject: Re: [PATCH net-next] net: phy: introduce phydev->port Message-ID: References: <20210209163852.17037-1-michael@walle.cc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210209163852.17037-1-michael@walle.cc> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > @@ -1552,6 +1552,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page) > phydev->asym_pause = 0; > phydev->speed = SPEED_UNKNOWN; > phydev->duplex = DUPLEX_UNKNOWN; > + phydev->port = fiber ? PORT_FIBRE : PORT_TP; > > if (phydev->autoneg == AUTONEG_ENABLE) > err = marvell_read_status_page_an(phydev, fiber, status); ... > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, > if (phydev->interface == PHY_INTERFACE_MODE_MOCA) > cmd->base.port = PORT_BNC; > else > - cmd->base.port = PORT_MII; > + cmd->base.port = phydev->port; > cmd->base.transceiver = phy_is_internal(phydev) ? > XCVR_INTERNAL : XCVR_EXTERNAL; > cmd->base.phy_address = phydev->mdio.addr; This is a general comment, not a problem specific to this patch. There is some interesting race conditions here. The marvell driver first checks the fibre page and gets the status of the fiber port. As you can see from the hunk above, it clears out pause, duplex, speed, sets port to PORT_FIBRE, and then reads the PHY registers to set these values. If link is not detected on the fibre, it swaps page and does it all again, but for the copper port. So once per second, phydev->port is going to flip flop PORT_FIBER->PORT_TP, if copper has link. Now, the read_status() call into the driver should be performed while holding the phydev->lock. So to the PHY state machine, this flip/flop does not matter, it is atomic with respect to the lock. But phy_ethtool_ksettings_get() is not talking the lock. It could see speed, duplex, and port while they have _UNKNOWN values, or port is part way through a flip flop. I think we need to take the lock here. phy_ethtool_ksettings_set() should also probably take the lock. Andrew