Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races

[Russell King - ARM Linux <linux@armlinux.org.uk>]

On Fri, Dec 08, 2017 at 05:17:14PM +0100, Andrew Lunn wrote:
> Hi Russell
> 
> > There is an open question whether there should be generic helpers for
> > this.  Generic helpers would mean:
> > 
> > - Additional couple of function pointers in phy_driver to read/write the
> >   paging register.  This has the restriction that there must only be one
> >   paging register.
> 
> I must be missing something. I don't see why there is this
> restriction. Don't we just need
> 
> int phy_get_page(phydev);
> int phy_set_page(phydev, page);

The restriction occurs because a PHY may have several different
registers, and knowing which of the registers need touching becomes an
issue.  We wouldn't want these accessors to needlessly access several
registers each and every time we requested an access to the page
register.

There's also the issue of whether an "int" or whatever type we choose to
pass the "page" around is enough bits.  I haven't surveyed all the PHY
drivers yet to know the answer to that.

> We might even be able to do without phy_get_page(), if we assume page
> 0 should be selected by default, and all paged reads/write return to
> page 0 afterwards.

That would break the marvell driver, where it polls the copper page (0)
and fiber page (1) for link, and leaves the page set appropriately
depending on which negotiated the link.  Not nice, but that's how the
driver is written to work.

> > - The helpers become more expensive, and because they're in a separate
> >   compilation unit, the compiler will be unable to optimise them by
> >   inlining the static functions.
> 
> The mdio bus is slow. Often there is a completion function triggered
> by an interrupt etc. Worst case it is bit-banging. I suspect the gains
> from inlining are just noise in the bigger picture.

Yep.

> > - The helpers would be re-usable, saving replications of that code, and
> >   making it more likely for phy authors to safely access the PHY.
> 
> This is the key point for me. This has likely to of been broken for
> years. If it is obviously broken, driver writers are more likely to
> get it right. Here it is not obvious, so we should take it out of the
> driver writers hands and do it in the core.

See the patch below that partially does this on top of this series.

 drivers/net/phy/marvell.c  |  70 +++++++++++++++++++--------
 drivers/net/phy/phy-core.c | 118 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h        |   8 +++
 3 files changed, 176 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 5548ac8fa239..183a60a06099 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -537,9 +537,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 	else
 		mscr = 0;
 
-	return marvell_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
-				    MII_88E1121_PHY_MSCR_REG,
-				    MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
+	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
+				MII_88E1121_PHY_MSCR_REG,
+				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
 }
 
 static int m88e1121_config_aneg(struct phy_device *phydev)
@@ -567,9 +567,9 @@ static int m88e1318_config_aneg(struct phy_device *phydev)
 {
 	int err;
 
-	err = marvell_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
-				   MII_88E1318S_PHY_MSCR1_REG,
-				   0, MII_88E1318S_PHY_MSCR1_PAD_ODD);
+	err = phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
+			       MII_88E1318S_PHY_MSCR1_REG,
+			       0, MII_88E1318S_PHY_MSCR1_PAD_ODD);
 	if (err < 0)
 		return err;
 
@@ -894,9 +894,9 @@ static int m88e1121_config_init(struct phy_device *phydev)
 	int err;
 
 	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
-	err = marvell_write_paged(phydev, MII_MARVELL_LED_PAGE,
-				  MII_88E1121_PHY_LED_CTRL,
-				  MII_88E1121_PHY_LED_DEF);
+	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+			      MII_88E1121_PHY_LED_CTRL,
+			      MII_88E1121_PHY_LED_DEF);
 	if (err < 0)
 		return err;
 
@@ -1544,7 +1544,7 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
 	int val;
 	u64 ret;
 
-	val = marvell_read_paged(phydev, stat.page, stat.reg);
+	val = phy_read_paged(phydev, stat.page, stat.reg);
 	if (val < 0) {
 		ret = UINT64_MAX;
 	} else {
@@ -1684,8 +1684,8 @@ static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 
 	*temp = 0;
 
-	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
-				 MII_88E1510_TEMP_SENSOR);
+	ret = phy_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+			     MII_88E1510_TEMP_SENSOR);
 	if (ret < 0)
 		return ret;
 
@@ -1700,8 +1700,8 @@ static int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 
 	*temp = 0;
 
-	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
-				 MII_88E1121_MISC_TEST);
+	ret = phy_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+			     MII_88E1121_MISC_TEST);
 	if (ret < 0)
 		return ret;
 
@@ -1718,10 +1718,10 @@ static int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 	temp = temp / 1000;
 	temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
 
-	return marvell_modify_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
-				    MII_88E1121_MISC_TEST,
-				    ~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK,
-				    temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT);
+	return phy_modify_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				MII_88E1121_MISC_TEST,
+				~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK,
+				temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT);
 }
 
 static int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
@@ -1730,8 +1730,8 @@ static int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 
 	*alarm = false;
 
-	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
-				 MII_88E1121_MISC_TEST);
+	ret = phy_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+			     MII_88E1121_MISC_TEST);
 	if (ret < 0)
 		return ret;
 
@@ -1977,6 +1977,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1995,6 +1997,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2013,6 +2017,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2031,6 +2037,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2050,6 +2058,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2071,6 +2081,8 @@ static struct phy_driver marvell_drivers[] = {
 		.set_wol = &m88e1318_set_wol,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2089,6 +2101,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2107,6 +2121,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2125,6 +2141,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2143,6 +2161,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2164,6 +2184,8 @@ static struct phy_driver marvell_drivers[] = {
 		.set_wol = &m88e1318_set_wol,
 		.resume = &marvell_resume,
 		.suspend = &marvell_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2184,6 +2206,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2203,6 +2227,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2223,6 +2249,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -2242,6 +2270,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.read_page = marvell_read_page,
+		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 83d32644cb4d..66363b425c2a 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -280,3 +280,121 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 	return ret;
 }
 EXPORT_SYMBOL(phy_write_mmd);
+
+static int __phy_read_page(struct phy_device *phydev)
+{
+	return phydev->drv->read_page(phydev);
+}
+
+static int __phy_write_page(struct phy_device *phydev, int page)
+{
+	return phydev->drv->write_page(phydev, page);
+}
+
+static int __phy_select_page(struct phy_device *phydev, int page)
+{
+	int ret, oldpage;
+
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+	oldpage = ret = __phy_read_page(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = __phy_write_page(phydev, page);
+	if (ret < 0)
+		return ret;
+
+	return oldpage;
+}
+
+static int __phy_restore_page(struct phy_device *phydev, int page, int rc)
+{
+	int ret;
+
+	if (page < 0) {
+		/* Propagate the phy page selection error code */
+		ret = page;
+	} else {
+		ret = __phy_write_page(phydev, page);
+
+		/* Propagate the operation return code if the page write
+		 * was successful.
+		 */
+		if (rc < 0 && ret >= 0)
+			ret = rc;
+	}
+
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
+
+	return ret;
+}
+
+/**
+ * phy_read_paged() - Convenience function for reading a paged register
+ * @phydev: a pointer to a &struct phy_device
+ * @page: the page for the phy
+ * @regnum: register number
+ *
+ * Same rules as for phy_read();
+ */
+int phy_read_paged(struct phy_device *phydev, int page, int regnum)
+{
+	int ret, oldpage;
+
+	oldpage = __phy_select_page(phydev, page);
+	if (oldpage >= 0)
+		ret = __phy_read(phydev, regnum);
+
+	return __phy_restore_page(phydev, oldpage, ret);
+}
+EXPORT_SYMBOL(phy_read_paged);
+
+/**
+ * phy_write_paged() - Convenience function for writing a paged register
+ * @phydev: a pointer to a &struct phy_device
+ * @page: the page for the phy
+ * @regnum: register number
+ * @val: value to write
+ *
+ * Same rules as for phy_write();
+ */
+int phy_write_paged(struct phy_device *phydev, int page, int regnum, int val)
+{
+	int ret, oldpage;
+
+	oldpage = __phy_select_page(phydev, page);
+	if (oldpage >= 0)
+		ret = __phy_write(phydev, regnum, val);
+
+	return __phy_restore_page(phydev, oldpage, ret);
+}
+EXPORT_SYMBOL(phy_write_paged);
+
+/**
+ * phy_modify_paged() - Convenience function for modifying a paged register
+ * @phydev: a pointer to a &struct phy_device
+ * @page: the page for the phy
+ * @regnum: register number
+ * @mask: bit mask of bits to preserve
+ * @set: bit mask of bits to set
+ *
+ * Same rules as for phy_read() and phy_write();
+ */
+int phy_modify_paged(struct phy_device *phydev, int page, int regnum,
+		     int mask, int set)
+{
+	int ret, res, oldpage;
+
+	oldpage = __phy_select_page(phydev, page);
+	if (oldpage >= 0) {
+		ret = __phy_read(phydev, regnum);
+		if (ret >= 0) {
+			res = __phy_write(phydev, regnum, (ret & mask) | set);
+			if (res < 0)
+				ret = res;
+		}
+	}
+
+	return __phy_restore_page(phydev, oldpage, ret);
+}
+EXPORT_SYMBOL(phy_modify_paged);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1db6e8eec85b..fa6f17d2a2de 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -634,6 +634,9 @@ struct phy_driver {
 	int (*write_mmd)(struct phy_device *dev, int devnum, u16 regnum,
 			 u16 val);
 
+	int (*read_page)(struct phy_device *dev);
+	int (*write_page)(struct phy_device *dev, int page);
+
 	/* Get the size and type of the eeprom contained within a plug-in
 	 * module */
 	int (*module_info)(struct phy_device *dev,
@@ -840,6 +843,11 @@ static inline bool phy_is_pseudo_fixed_link(struct phy_device *phydev)
  */
 int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val);
 
+int phy_read_paged(struct phy_device *phydev, int page, int regnum);
+int phy_write_paged(struct phy_device *phydev, int page, int regnum, int val);
+int phy_modify_paged(struct phy_device *phydev, int page, int regnum,
+		     int mask, int set);
+
 bool phy_check_valid(int speed, int duplex, u32 features);
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up