From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH net-next v2 5/7] net: phy: marvell: fix paged access races Date: Tue, 2 Jan 2018 17:00:47 +0100 Message-ID: <20180102160047.GD20986@lunn.ch> References: <20180102105218.GB21998@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Fainelli , netdev@vger.kernel.org To: Russell King Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:50633 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751024AbeABQAt (ORCPT ); Tue, 2 Jan 2018 11:00:49 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jan 02, 2018 at 10:58:48AM +0000, Russell King wrote: > For paged accesses to be truely safe, we need to hold the bus lock to > prevent anyone else gaining access to the registers while we modify > them. > > The phydev->lock mutex does not do this: userspace via the MII ioctl > can still sneak in and read or write any register while we are on a > different page, and the suspend/resume methods can be called by a > thread different to the thread polling the phy status. > > Races have been observed with mvneta on SolidRun Clearfog with phylink, > particularly between the phylib worker reading the PHYs status, and > the thread resuming mvneta, calling phy_start() which then calls > through to m88e1121_config_aneg_rgmii_delays(), which tries to > read-modify-write the MSCR register: > > CPU0 CPU1 > marvell_read_status_page() > marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE) > ... > m88e1121_config_aneg_rgmii_delays() > set_page(MII_MARVELL_MSCR_PAGE) > phy_read(phydev, MII_88E1121_PHY_MSCR_REG) > marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE); > ... > phy_write(phydev, MII_88E1121_PHY_MSCR_REG) > > The result of this is we end up writing the copper page register 21, > which causes the copper PHY to be disabled, and the link partner sees > the link immediately go down. > > Solve this by taking the bus lock instead of the PHY lock, thereby > preventing other accesses to the PHY while we are accessing other PHY > pages. > > Signed-off-by: Russell King Reviewed-by: Andrew Lunn Andrew