Skip to contentSkip to conversation

Fixes for Marvell MII paged register access races

RFC PATCH In review Russell King - ARM Linux 4 participants 16 messages Dec 8, 2017 – Dec 10, 2017

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Russell King - ARM Linux

author reviewer
#0 lore raw

cover Fixes for Marvell MII paged register access races

Hi,

While doing final testing of the mvneta changes for phylink, a very easy to trigger race condition was found with the Marvell PHY driver which manifested itself as the link going down when a hibernate cycle terminates.

The issue turned out to be a race between two threads accessing the PHY - one trying to do a status read and the other configuring the PHY.

The result is the configuration thread tries to read-modify-write a paged register in a non-copper page, but the status read thread switches the PHY back to the copper page half-way through.

Various solutions involving phy->lock were considered, but found to create more lock dependency issues than were nice to deal with.

The solution proposed here uses the mdiobus lock to ensure that accesses to paged registers become atomic with respect to all other bus accesses, including those from userspace.

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.

- 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 helpers would be re-usable, saving replications of that code, and making it more likely for phy authors to safely access the PHY.

Another potential question is whether using the mdiobus lock (which excludes all other MII bus access) is best - while it has the advantage of also ensuring atomicity with userspace accesses, it means that no one else can access an independent PHY on the same bus while a paged access is on-going. It feels like a big hammer, but I'm not convinced that we will see a lot of contention on it.

Comments?

 drivers/net/phy/marvell.c  | 365 +++++++++++++++++++++------------------------
 drivers/net/phy/mdio_bus.c |  65 ++++++--
 drivers/net/phy/phy-core.c |  11 +-
 include/linux/mdio.h       |   3 +
 include/linux/phy.h        |  26 ++++
 5 files changed, 256 insertions(+), 214 deletions(-)
Signature · 3 lines
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
#1 lore raw

RFC PATCH 1/4 net: mdiobus: add unlocked accessors

Add unlocked versions of the bus accessors, which allows access to the bus with all the tracing. These accessors validate that the bus mutex is held, which is a basic requirement for all mii bus accesses.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/mdio_bus.c | 65 +++++++++++++++++++++++++++++++++++++---------
 include/linux/mdio.h       |  3 +++
 2 files changed, 56 insertions(+), 12 deletions(-)
drivers/net/phy/mdio_bus.c +53 -12
index 2df7b62c1a36..20fd8128b03c 100644
@@ -493,6 +493,55 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 EXPORT_SYMBOL(mdiobus_scan);
 
 /**
+ * __mdiobus_read - Unlocked version of the mdiobus_read function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to read
+ *
+ * Read a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
+{
+	int retval;
+
+	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
+
+	retval = bus->read(bus, addr, regnum);
+
+	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
+
+	return retval;
+}
+EXPORT_SYMBOL(__mdiobus_read);
+
+/**
+ * __mdiobus_write - Unlocked version of the mdiobus_write function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * Write a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_write(struct mii_bus *bus, int addr, int regnum, int val)
+{
+	int err;
+
+	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
+
+	err = bus->write(bus, addr, regnum, val);
+
+	trace_mdio_access(bus, 0, addr, regnum, val, err);
+
+	return err;
+}
+EXPORT_SYMBOL(__mdiobus_write);
+
+/**
  * mdiobus_read_nested - Nested version of the mdiobus_read function
  * @bus: the mii_bus struct
  * @addr: the phy address

@@ -512,11 +561,9 @@ int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum)
 	BUG_ON(in_interrupt());
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
-	retval = bus->read(bus, addr, regnum);
+	retval = __mdiobus_read(bus, addr, regnum);
 	mutex_unlock(&bus->mdio_lock);
 
-	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
-
 	return retval;
 }
 EXPORT_SYMBOL(mdiobus_read_nested);

@@ -538,11 +585,9 @@ int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 	BUG_ON(in_interrupt());
 
 	mutex_lock(&bus->mdio_lock);
-	retval = bus->read(bus, addr, regnum);
+	retval = __mdiobus_read(bus, addr, regnum);
 	mutex_unlock(&bus->mdio_lock);
 
-	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
-
 	return retval;
 }
 EXPORT_SYMBOL(mdiobus_read);

@@ -568,11 +613,9 @@ int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 	BUG_ON(in_interrupt());
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
-	err = bus->write(bus, addr, regnum, val);
+	err = __mdiobus_write(bus, addr, regnum, val);
 	mutex_unlock(&bus->mdio_lock);
 
-	trace_mdio_access(bus, 0, addr, regnum, val, err);
-
 	return err;
 }
 EXPORT_SYMBOL(mdiobus_write_nested);

@@ -595,11 +638,9 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 	BUG_ON(in_interrupt());
 
 	mutex_lock(&bus->mdio_lock);
-	err = bus->write(bus, addr, regnum, val);
+	err = __mdiobus_write(bus, addr, regnum, val);
 	mutex_unlock(&bus->mdio_lock);
 
-	trace_mdio_access(bus, 0, addr, regnum, val, err);
-
 	return err;
 }
 EXPORT_SYMBOL(mdiobus_write);
include/linux/mdio.h +3 -0
index ca08ab16ecdc..4be30adc033b 100644
@@ -257,6 +257,9 @@ static inline u16 ethtool_adv_to_mmd_eee_adv_t(u32 adv)
 	return reg;
 }
 
+int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
+int __mdiobus_write(struct mii_bus *bus, int addr, int regnum, int val);
+
 int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
 int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum);
 int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
Signature · 1 line
2.7.4
Reviewed-by on v1 1/4
#2 lore raw

Re: [PATCH RFC 1/4] net: mdiobus: add unlocked accessors

Quoted 5 lines · Russell King

Add unlocked versions of the bus accessors, which allows access to the bus with all the tracing. These accessors validate that the bus mutex is held, which is a basic requirement for all mii bus accesses.

Signed-off-by Russell King <[email protected]>
Reviewed-by Florian Fainelli <[email protected]>
Signature · 1 line
Florian
#3 lore raw

RFC PATCH 2/4 net: phy: use unlocked accessors for indirect MMD accesses

In reply to Russell King - ARM Linux

Use unlocked accessors for indirect MMD accesses to clause 22 PHYs. This permits tracing of these accesses.

Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/phy-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/net/phy/phy-core.c +6 -5
index 21f75ae244b3..83d32644cb4d 100644
@@ -193,13 +193,14 @@ static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 			     u16 regnum)
 {
 	/* Write the desired MMD Devad */
-	bus->write(bus, phy_addr, MII_MMD_CTRL, devad);
+	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
 
 	/* Write the desired MMD register address */
-	bus->write(bus, phy_addr, MII_MMD_DATA, regnum);
+	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
 
 	/* Select the Function : DATA with no post increment */
-	bus->write(bus, phy_addr, MII_MMD_CTRL, devad | MII_MMD_CTRL_NOINCR);
+	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
+			devad | MII_MMD_CTRL_NOINCR);
 }
 
 /**

@@ -232,7 +233,7 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 		mmd_phy_indirect(bus, phy_addr, devad, regnum);
 
 		/* Read the content of the MMD's selected register */
-		val = bus->read(bus, phy_addr, MII_MMD_DATA);
+		val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
 		mutex_unlock(&bus->mdio_lock);
 	}
 	return val;

@@ -271,7 +272,7 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 		mmd_phy_indirect(bus, phy_addr, devad, regnum);
 
 		/* Write the data into MMD's selected register */
-		bus->write(bus, phy_addr, MII_MMD_DATA, val);
+		__mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
 		mutex_unlock(&bus->mdio_lock);
 
 		ret = 0;
Signature · 1 line
2.7.4
#5 lore raw

RFC PATCH 3/4 net: phy: add unlocked accessors

In reply to Russell King - ARM Linux

Add unlocked versions of the bus accessors, which allows access to the bus with all the tracing. These accessors validate that the bus mutex is held, which is a basic requirement for all mii bus accesses.

Signed-off-by Russell King <[email protected]>
---
 include/linux/phy.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
include/linux/phy.h +26 -0
index 71d777fe6c3d..964803bd7324 100644
@@ -716,6 +716,18 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
 }
 
 /**
+ * __phy_read - convenience function for reading a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to read
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_read(struct phy_device *phydev, u32 regnum)
+{
+	return __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr, regnum);
+}
+
+/**
  * phy_write - Convenience function for writing a given PHY register
  * @phydev: the phy_device struct
  * @regnum: register number to write

@@ -731,6 +743,20 @@ static inline int phy_write(struct phy_device *phydev, u32 regnum, u16 val)
 }
 
 /**
+ * __phy_write - Convenience function for writing a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_write(struct phy_device *phydev, u32 regnum, u16 val)
+{
+	return __mdiobus_write(phydev->mdio.bus, phydev->mdio.addr, regnum,
+			       val);
+}
+
+/**
  * phy_interrupt_is_valid - Convenience function for testing a given PHY irq
  * @phydev: the phy_device struct
  *
Signature · 1 line
2.7.4
Reviewed-by on v1 3/4
#6 lore raw

Re: [PATCH RFC 3/4] net: phy: add unlocked accessors

Quoted 5 lines · Russell King

Add unlocked versions of the bus accessors, which allows access to the bus with all the tracing. These accessors validate that the bus mutex is held, which is a basic requirement for all mii bus accesses.

Signed-off-by Russell King <[email protected]>
Reviewed-by Florian Fainelli <[email protected]>
Quoted 9 lines
---
 include/linux/phy.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
include/linux/phy.h +0 -0
index 71d777fe6c3d..964803bd7324 100644

include/linux/phy.h static inline int phy_read(struct phy_device *phydev, u32 regnum) · line 725

Quoted 10 lines
include/linux/phy.h +7 -0
@@ -716,6 +716,18 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
 }
 
 /**
+ * __phy_read - convenience function for reading a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to read
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_read(struct phy_device *phydev, u32 regnum)

Do you know if we could have sparse validate that the caller of these functions holds the mutex? I remember reading somewhere that sparse does not do that yet, but can't get my hands on it.

Signature · 1 line
Florian
author reviewer
#7 lore raw

Re: [PATCH RFC 3/4] net: phy: add unlocked accessors

Quoted 22 lines · Florian Fainelli
Quoted 5 lines · Russell King

Add unlocked versions of the bus accessors, which allows access to the bus with all the tracing. These accessors validate that the bus mutex is held, which is a basic requirement for all mii bus accesses.

Signed-off-by Russell King <[email protected]>
Reviewed-by Florian Fainelli <[email protected]>
Quoted 11 lines
---
 include/linux/phy.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
include/linux/phy.h +0 -0
index 71d777fe6c3d..964803bd7324 100644
@@ -716,6 +716,18 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
 }
Quoted 12 lines

include/linux/phy.h static inline int phy_read(struct phy_device *phydev, u32 regnum) · line 725

Quoted 8 lines
include/linux/phy.h +7 -0
 /**
+ * __phy_read - convenience function for reading a given PHY register
+ * @phydev: the phy_device struct
+ * @regnum: register number to read
+ *
+ * The caller must have taken the MDIO bus lock.
+ */
+static inline int __phy_read(struct phy_device *phydev, u32 regnum)

Do you know if we could have sparse validate that the caller of these functions holds the mutex? I remember reading somewhere that sparse does not do that yet, but can't get my hands on it.

Hi Florian,

I tried adding __acquires() and __releases() annotations, but sparse complained about unbalanced lock counts when I did, because sparse has no knowledge about mutexes being taken or released.

Unfortunately, it seems that these annotations are undocumented in the kernel, but from what I can see, it turns out that they only apply to spinlocks.

Signature · 3 lines
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
#8 lore raw

RFC PATCH 4/4 net: phy: marvell: fix paged access races

In reply to Russell King - ARM Linux

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 <[email protected]>
---
 drivers/net/phy/marvell.c | 365 +++++++++++++++++++++-------------------------
 1 file changed, 168 insertions(+), 197 deletions(-)
drivers/net/phy/marvell.c +168 -197
index 4d02b27df044..87b76db456cf 100644
@@ -177,27 +177,96 @@ struct marvell_priv {
 	struct device *hwmon_dev;
 };
 
-static int marvell_get_page(struct phy_device *phydev)
+static int marvell_read_page(struct phy_device *phydev)
 {
-	return phy_read(phydev, MII_MARVELL_PHY_PAGE);
+	return __phy_read(phydev, MII_MARVELL_PHY_PAGE);
 }
 
-static int marvell_set_page(struct phy_device *phydev, int page)
+static int marvell_write_page(struct phy_device *phydev, int page)
 {
-	return phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
+	return __phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
 }
 
-static int marvell_get_set_page(struct phy_device *phydev, int page)
+static int marvell_save_page(struct phy_device *phydev)
 {
-	int oldpage = marvell_get_page(phydev);
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+	return marvell_read_page(phydev);
+}
 
-	if (oldpage < 0)
-		return oldpage;
+static int marvell_select_page(struct phy_device *phydev, int page)
+{
+	int ret, oldpage;
 
-	if (page != oldpage)
-		return marvell_set_page(phydev, page);
+	oldpage = ret = marvell_save_page(phydev);
+	if (ret < 0)
+		return ret;
 
-	return 0;
+	if (oldpage != page)
+		ret = marvell_write_page(phydev, page);
+
+	return ret < 0 ? ret : oldpage;
+}
+
+static int marvell_restore_page(struct phy_device *phydev, int oldpage, int ret)
+{
+	int r;
+
+	if (oldpage >= 0) {
+		r = marvell_write_page(phydev, oldpage);
+		if (ret == 0 && r < 0)
+			ret = r;
+	} else {
+		ret = oldpage;
+	}
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
+
+	return ret;
+}
+
+static int marvell_read_paged(struct phy_device *phydev, int page, int regnum)
+{
+	int ret = 0, oldpage;
+
+	oldpage = marvell_select_page(phydev, page);
+	if (oldpage >= 0)
+		ret = __phy_read(phydev, regnum);
+
+	return marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_write_paged(struct phy_device *phydev, int page, int regnum,
+			       int val)
+{
+	int ret = 0, oldpage;
+
+	oldpage = marvell_select_page(phydev, page);
+	if (oldpage >= 0)
+		ret = __phy_write(phydev, regnum, val);
+
+	return marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_modify_paged(struct phy_device *phydev, int page, int regnum,
+				int mask, int set)
+{
+	int ret = 0, res, oldpage;
+
+	oldpage = marvell_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 marvell_restore_page(phydev, oldpage, ret);
+}
+
+static int marvell_set_page(struct phy_device *phydev, int page)
+{
+	return phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
 }
 
 static int marvell_ack_interrupt(struct phy_device *phydev)

@@ -399,7 +468,7 @@ static int m88e1111_config_aneg(struct phy_device *phydev)
 static int marvell_of_reg_init(struct phy_device *phydev)
 {
 	const __be32 *paddr;
-	int len, i, saved_page, current_page, ret;
+	int len, i, saved_page, current_page, ret = 0;
 
 	if (!phydev->mdio.dev.of_node)
 		return 0;

@@ -409,12 +478,11 @@ static int marvell_of_reg_init(struct phy_device *phydev)
 	if (!paddr || len < (4 * sizeof(*paddr)))
 		return 0;
 
-	saved_page = marvell_get_page(phydev);
+	saved_page = marvell_save_page(phydev);
 	if (saved_page < 0)
-		return saved_page;
+		goto err;
 	current_page = saved_page;
 
-	ret = 0;
 	len /= sizeof(*paddr);
 	for (i = 0; i < len - 3; i += 4) {
 		u16 page = be32_to_cpup(paddr + i);

@@ -425,14 +493,14 @@ static int marvell_of_reg_init(struct phy_device *phydev)
 
 		if (page != current_page) {
 			current_page = page;
-			ret = marvell_set_page(phydev, page);
+			ret = marvell_write_page(phydev, page);
 			if (ret < 0)
 				goto err;
 		}
 
 		val = 0;
 		if (mask) {
-			val = phy_read(phydev, reg);
+			val = __phy_read(phydev, reg);
 			if (val < 0) {
 				ret = val;
 				goto err;

@@ -441,17 +509,12 @@ static int marvell_of_reg_init(struct phy_device *phydev)
 		}
 		val |= val_bits;
 
-		ret = phy_write(phydev, reg, val);
+		ret = __phy_write(phydev, reg, val);
 		if (ret < 0)
 			goto err;
 	}
 err:
-	if (current_page != saved_page) {
-		i = marvell_set_page(phydev, saved_page);
-		if (ret == 0)
-			ret = i;
-	}
-	return ret;
+	return marvell_restore_page(phydev, saved_page, ret);
 }
 #else
 static int marvell_of_reg_init(struct phy_device *phydev)

@@ -462,34 +525,21 @@ static int marvell_of_reg_init(struct phy_device *phydev)
 
 static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 {
-	int err, oldpage, mscr;
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
-	if (oldpage < 0)
-		return oldpage;
-
-	mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG);
-	if (mscr < 0) {
-		err = mscr;
-		goto out;
-	}
-
-	mscr &= MII_88E1121_PHY_MSCR_DELAY_MASK;
+	int mscr;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
-		mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |
-			 MII_88E1121_PHY_MSCR_TX_DELAY);
+		mscr = MII_88E1121_PHY_MSCR_RX_DELAY |
+		       MII_88E1121_PHY_MSCR_TX_DELAY;
 	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
-		mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
+		mscr = MII_88E1121_PHY_MSCR_RX_DELAY;
 	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
-		mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
-
-	err = phy_write(phydev, MII_88E1121_PHY_MSCR_REG, mscr);
-
-out:
-	marvell_set_page(phydev, oldpage);
+		mscr = MII_88E1121_PHY_MSCR_TX_DELAY;
+	else
+		mscr = 0;
 
-	return err;
+	return marvell_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)

@@ -515,20 +565,11 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 
 static int m88e1318_config_aneg(struct phy_device *phydev)
 {
-	int err, oldpage, mscr;
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MSCR_PAGE);
-	if (oldpage < 0)
-		return oldpage;
-
-	mscr = phy_read(phydev, MII_88E1318S_PHY_MSCR1_REG);
-	mscr |= MII_88E1318S_PHY_MSCR1_PAD_ODD;
-
-	err = phy_write(phydev, MII_88E1318S_PHY_MSCR1_REG, mscr);
-	if (err < 0)
-		return err;
+	int err;
 
-	err = marvell_set_page(phydev, oldpage);
+	err = marvell_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
+				   MII_88E1318S_PHY_MSCR1_REG,
+				   0, MII_88E1318S_PHY_MSCR1_PAD_ODD);
 	if (err < 0)
 		return err;
 

@@ -850,20 +891,15 @@ static int m88e1111_config_init(struct phy_device *phydev)
 
 static int m88e1121_config_init(struct phy_device *phydev)
 {
-	int err, oldpage;
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_LED_PAGE);
-	if (oldpage < 0)
-		return oldpage;
+	int err;
 
 	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
-	err = phy_write(phydev, MII_88E1121_PHY_LED_CTRL,
-			MII_88E1121_PHY_LED_DEF);
+	err = marvell_write_paged(phydev, MII_MARVELL_LED_PAGE,
+				  MII_88E1121_PHY_LED_CTRL,
+				  MII_88E1121_PHY_LED_DEF);
 	if (err < 0)
 		return err;
 
-	marvell_set_page(phydev, oldpage);
-
 	/* Set marvell,reg-init configuration from device tree */
 	return marvell_config_init(phydev);
 }

@@ -1382,100 +1418,102 @@ static int m88e1121_did_interrupt(struct phy_device *phydev)
 static void m88e1318_get_wol(struct phy_device *phydev,
 			     struct ethtool_wolinfo *wol)
 {
+	int oldpage, ret = 0;
+
 	wol->supported = WAKE_MAGIC;
 	wol->wolopts = 0;
 
-	if (marvell_set_page(phydev, MII_MARVELL_WOL_PAGE) < 0)
-		return;
+	oldpage = marvell_select_page(phydev, MII_MARVELL_WOL_PAGE);
+	if (oldpage < 0)
+		goto error;
 
-	if (phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL) &
-	    MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
+	ret = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+	if (ret & MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE)
 		wol->wolopts |= WAKE_MAGIC;
 
-	if (marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE) < 0)
-		return;
+error:
+	marvell_restore_page(phydev, oldpage, ret);
 }
 
 static int m88e1318_set_wol(struct phy_device *phydev,
 			    struct ethtool_wolinfo *wol)
 {
-	int err, oldpage, temp;
+	int err = 0, oldpage, temp;
 
-	oldpage = marvell_get_page(phydev);
+	oldpage = marvell_save_page(phydev);
+	if (oldpage < 0)
+		goto error;
 
 	if (wol->wolopts & WAKE_MAGIC) {
 		/* Explicitly switch to page 0x00, just to be sure */
-		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
+		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
 		if (err < 0)
-			return err;
+			goto error;
 
 		/* Enable the WOL interrupt */
 		temp = phy_read(phydev, MII_88E1318S_PHY_CSIER);
 		temp |= MII_88E1318S_PHY_CSIER_WOL_EIE;
-		err = phy_write(phydev, MII_88E1318S_PHY_CSIER, temp);
+		err = __phy_write(phydev, MII_88E1318S_PHY_CSIER, temp);
 		if (err < 0)
-			return err;
+			goto error;
 
-		err = marvell_set_page(phydev, MII_MARVELL_LED_PAGE);
+		err = marvell_write_page(phydev, MII_MARVELL_LED_PAGE);
 		if (err < 0)
-			return err;
+			goto error;
 
 		/* Setup LED[2] as interrupt pin (active low) */
 		temp = phy_read(phydev, MII_88E1318S_PHY_LED_TCR);
 		temp &= ~MII_88E1318S_PHY_LED_TCR_FORCE_INT;
 		temp |= MII_88E1318S_PHY_LED_TCR_INTn_ENABLE;
 		temp |= MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW;
-		err = phy_write(phydev, MII_88E1318S_PHY_LED_TCR, temp);
+		err = __phy_write(phydev, MII_88E1318S_PHY_LED_TCR, temp);
 		if (err < 0)
-			return err;
+			goto error;
 
-		err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
+		err = marvell_write_page(phydev, MII_MARVELL_WOL_PAGE);
 		if (err < 0)
-			return err;
+			goto error;
 
 		/* Store the device address for the magic packet */
-		err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD2,
+		err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD2,
 				((phydev->attached_dev->dev_addr[5] << 8) |
 				 phydev->attached_dev->dev_addr[4]));
 		if (err < 0)
-			return err;
-		err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD1,
+			goto error;
+		err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD1,
 				((phydev->attached_dev->dev_addr[3] << 8) |
 				 phydev->attached_dev->dev_addr[2]));
 		if (err < 0)
-			return err;
-		err = phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD0,
+			goto error;
+		err = __phy_write(phydev, MII_88E1318S_PHY_MAGIC_PACKET_WORD0,
 				((phydev->attached_dev->dev_addr[1] << 8) |
 				 phydev->attached_dev->dev_addr[0]));
 		if (err < 0)
-			return err;
+			goto error;
 
 		/* Clear WOL status and enable magic packet matching */
-		temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+		temp = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
 		temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
 		temp |= MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE;
-		err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
+		err = __phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
 		if (err < 0)
-			return err;
+			goto error;
 	} else {
-		err = marvell_set_page(phydev, MII_MARVELL_WOL_PAGE);
+		err = marvell_write_page(phydev, MII_MARVELL_WOL_PAGE);
 		if (err < 0)
-			return err;
+			goto error;
 
 		/* Clear WOL status and disable magic packet matching */
-		temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+		temp = __phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
 		temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
 		temp &= ~MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE;
-		err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
+		err = __phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
 		if (err < 0)
-			return err;
+			goto error;
 	}
 
-	err = marvell_set_page(phydev, oldpage);
-	if (err < 0)
-		return err;
-
-	return 0;
+error:
+	return marvell_restore_page(phydev, oldpage, err);
 }
 
 static int marvell_get_sset_count(struct phy_device *phydev)

@@ -1503,14 +1541,10 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
 {
 	struct marvell_hw_stat stat = marvell_hw_stats[i];
 	struct marvell_priv *priv = phydev->priv;
-	int oldpage, val;
+	int val;
 	u64 ret;
 
-	oldpage = marvell_get_set_page(phydev, stat.page);
-	if (oldpage < 0)
-		return UINT64_MAX;
-
-	val = phy_read(phydev, stat.reg);
+	val = marvell_read_paged(phydev, stat.page, stat.reg);
 	if (val < 0) {
 		ret = UINT64_MAX;
 	} else {

@@ -1519,8 +1553,6 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
 		ret = priv->stats[i];
 	}
 
-	marvell_set_page(phydev, oldpage);
-
 	return ret;
 }
 

@@ -1537,18 +1569,14 @@ static void marvell_get_stats(struct phy_device *phydev,
 static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 {
 	int oldpage;
-	int ret;
+	int ret = 0;
 	int val;
 
 	*temp = 0;
 
-	mutex_lock(&phydev->lock);
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
-	if (oldpage < 0) {
-		mutex_unlock(&phydev->lock);
-		return oldpage;
-	}
+	oldpage = marvell_select_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
+	if (oldpage < 0)
+		goto error;
 
 	/* Enable temperature sensor */
 	ret = phy_read(phydev, MII_88E1121_MISC_TEST);

@@ -1578,10 +1606,7 @@ static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 	*temp = ((val & MII_88E1121_MISC_TEST_TEMP_MASK) - 5) * 5000;
 
 error:
-	marvell_set_page(phydev, oldpage);
-	mutex_unlock(&phydev->lock);
-
-	return ret;
+	return marvell_restore_page(phydev, oldpage, ret);
 }
 
 static int m88e1121_hwmon_read(struct device *dev,

@@ -1655,118 +1680,64 @@ static const struct hwmon_chip_info m88e1121_hwmon_chip_info = {
 
 static int m88e1510_get_temp(struct phy_device *phydev, long *temp)
 {
-	int oldpage;
 	int ret;
 
 	*temp = 0;
 
-	mutex_lock(&phydev->lock);
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
-	if (oldpage < 0) {
-		mutex_unlock(&phydev->lock);
-		return oldpage;
-	}
-
-	ret = phy_read(phydev, MII_88E1510_TEMP_SENSOR);
+	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				 MII_88E1510_TEMP_SENSOR);
 	if (ret < 0)
-		goto error;
+		return ret;
 
 	*temp = ((ret & MII_88E1510_TEMP_SENSOR_MASK) - 25) * 1000;
 
-error:
-	marvell_set_page(phydev, oldpage);
-	mutex_unlock(&phydev->lock);
-
-	return ret;
+	return 0;
 }
 
 static int m88e1510_get_temp_critical(struct phy_device *phydev, long *temp)
 {
-	int oldpage;
 	int ret;
 
 	*temp = 0;
 
-	mutex_lock(&phydev->lock);
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
-	if (oldpage < 0) {
-		mutex_unlock(&phydev->lock);
-		return oldpage;
-	}
-
-	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				 MII_88E1121_MISC_TEST);
 	if (ret < 0)
-		goto error;
+		return ret;
 
 	*temp = (((ret & MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) >>
 		  MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT) * 5) - 25;
 	/* convert to mC */
 	*temp *= 1000;
 
-error:
-	marvell_set_page(phydev, oldpage);
-	mutex_unlock(&phydev->lock);
-
-	return ret;
+	return 0;
 }
 
 static int m88e1510_set_temp_critical(struct phy_device *phydev, long temp)
 {
-	int oldpage;
-	int ret;
-
-	mutex_lock(&phydev->lock);
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
-	if (oldpage < 0) {
-		mutex_unlock(&phydev->lock);
-		return oldpage;
-	}
-
-	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
-	if (ret < 0)
-		goto error;
-
 	temp = temp / 1000;
 	temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
-	ret = phy_write(phydev, MII_88E1121_MISC_TEST,
-			(ret & ~MII_88E1510_MISC_TEST_TEMP_THRESHOLD_MASK) |
-			(temp << MII_88E1510_MISC_TEST_TEMP_THRESHOLD_SHIFT));
 
-error:
-	marvell_set_page(phydev, oldpage);
-	mutex_unlock(&phydev->lock);
-
-	return ret;
+	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);
 }
 
 static int m88e1510_get_temp_alarm(struct phy_device *phydev, long *alarm)
 {
-	int oldpage;
 	int ret;
 
 	*alarm = false;
 
-	mutex_lock(&phydev->lock);
-
-	oldpage = marvell_get_set_page(phydev, MII_MARVELL_MISC_TEST_PAGE);
-	if (oldpage < 0) {
-		mutex_unlock(&phydev->lock);
-		return oldpage;
-	}
-
-	ret = phy_read(phydev, MII_88E1121_MISC_TEST);
+	ret = marvell_read_paged(phydev, MII_MARVELL_MISC_TEST_PAGE,
+				 MII_88E1121_MISC_TEST);
 	if (ret < 0)
-		goto error;
-	*alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
+		return ret;
 
-error:
-	marvell_set_page(phydev, oldpage);
-	mutex_unlock(&phydev->lock);
+	*alarm = !!(ret & MII_88E1510_MISC_TEST_TEMP_IRQ);
 
-	return ret;
+	return 0;
 }
 
 static int m88e1510_hwmon_read(struct device *dev,
Signature · 1 line
2.7.4
maintainer
#9 lore raw
In reply to Russell King - ARM Linux

Hi Russell

Quoted 6 lines

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);

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.

Quoted 3 lines

- 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.

Quoted 2 lines

- 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.

Quoted 6 lines

Another potential question is whether using the mdiobus lock (which excludes all other MII bus access) is best - while it has the advantage of also ensuring atomicity with userspace accesses, it means that no one else can access an independent PHY on the same bus while a paged access is on-going. It feels like a big hammer, but I'm not convinced that we will see a lot of contention on it.

I think you are right about there being little contention on the lock. I suspect most paged accesses are performed during initial setup and configuration. I guess the once per second poll does not need to use paged registers. And the weight of that hammer can be reduced a lot by using interrupts instead of polling.

Andrew

author reviewer
#10 lore raw
Quoted 9 lines · Andrew Lunn

Hi Russell

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines
Quoted 3 lines

- 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.

Quoted 7 lines
Quoted 2 lines

- 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(-)
drivers/net/phy/marvell.c +50 -20
index 5548ac8fa239..183a60a06099 100644
@@ -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,
drivers/net/phy/phy-core.c +118 -0

121 lines not on this page · raw message

include/linux/phy.h +8 -0
index 1db6e8eec85b..fa6f17d2a2de 100644
@@ -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,
Signature · 3 lines
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
#11 lore raw
Quoted 22 lines · Russell King - ARM Linux
Quoted 9 lines · Andrew Lunn

Hi Russell

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

I have not come across a PHY yet that required writing a page across two 16-bit quantities, in general, the page fits within less than 16-bit actually to fit within one MDIO write. That does not mean it cannot exist obviously, but having about 32-bit x pages of address space within a PHY sounds a bit extreme.

Signature · 1 line
Florian
author reviewer
#12 lore raw
Quoted 27 lines · Florian Fainelli
Quoted 22 lines · Russell King - ARM Linux
Quoted 9 lines · Andrew Lunn

Hi Russell

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

I have not come across a PHY yet that required writing a page across two 16-bit quantities, in general, the page fits within less than 16-bit actually to fit within one MDIO write. That does not mean it cannot exist obviously, but having about 32-bit x pages of address space within a PHY sounds a bit extreme.

True, and phylib at the moment contains nothing beyond a single register. I was thinking more of paging bits across several registers - such a case would not lend itself well to this implementation as you'd have to read every paging-capable register and write every paging capable register in the phy_driver page accessor methods.

The good news is, having read through several drivers that contain the caseless "page" string, there are no drivers that need anything but a simple paging case, so it's not a concern. Those which seem to use page accesses are:

at803x: this only uses a single bit in a register for one access.

dp83640: looks like it implements its own locking and banks registers

	0x10-0x1e.  Multiple accesses throughout the driver.

marvell: we know about this one which is the problem case.

microchip: looks like it banks the registers 0x10-0x1e, and uses this
for mdix control.

mscc: looks like it banks the registers 0x10-0x1e. Several accesses
throughout the driver, some under the phydev lock but others unclear whether they are locked. Could be a problem.

realtek: looks like it banks the registers 0x10-0x1e. Probably racy -
interrupt handling uses paged accesses which may run in a threaded interrupt handler.

vitesse: "/* map extended registers set 0x10 - 0x1e */" in one place
for mdix control via config_aneg.

Signature · 3 lines
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
maintainer
#13 lore raw
Quoted 3 lines

The good news is, having read through several drivers that contain the caseless "page" string, there are no drivers that need anything but a simple paging case, so it's not a concern.

Hi Russell

Also grep for bank. rockchip.c.

Andrew

#14 lore raw
In reply to Russell King - ARM Linux
Quoted 10 lines · Russell King - ARM Linux

Hi,

While doing final testing of the mvneta changes for phylink, a very easy to trigger race condition was found with the Marvell PHY driver which manifested itself as the link going down when a hibernate cycle terminates.

The issue turned out to be a race between two threads accessing the PHY - one trying to do a status read and the other configuring the PHY.

Quoted 6 lines

The result is the configuration thread tries to read-modify-write a paged register in a non-copper page, but the status read thread switches the PHY back to the copper page half-way through.

Various solutions involving phy->lock were considered, but found to create more lock dependency issues than were nice to deal with.

I can certainly imagine that, because you would ideally want a phy_device wide lock which serializes the page entry/exit, which needs to be able to run within the PHY state machine (so with phydev->lock held), but also from a context where phydev->lock is not held, wheee.

Quoted 19 lines

The solution proposed here uses the mdiobus lock to ensure that accesses to paged registers become atomic with respect to all other bus accesses, including those from userspace.

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.

- 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 helpers would be re-usable, saving replications of that code, and making it more likely for phy authors to safely access the PHY.

Quoted 6 lines

Another potential question is whether using the mdiobus lock (which excludes all other MII bus access) is best - while it has the advantage of also ensuring atomicity with userspace accesses, it means that no one else can access an independent PHY on the same bus while a paged access is on-going. It feels like a big hammer, but I'm not convinced that we will see a lot of contention on it.

Regarding that last topic, this could become a fairly contended lock on a switch with lots (e.g: > 5-6) of built-in PHYs, all being polled (which is usually the case right now). One would expect that the polling should be limited to 2 BMSR reads to minimize the bus utilization.

Quoted 10 lines

Comments?

 drivers/net/phy/marvell.c  | 365 +++++++++++++++++++++------------------------
 drivers/net/phy/mdio_bus.c |  65 ++++++--
 drivers/net/phy/phy-core.c |  11 +-
 include/linux/mdio.h       |   3 +
 include/linux/phy.h        |  26 ++++
 5 files changed, 256 insertions(+), 214 deletions(-)
Signature · 1 line
Florian
maintainer
#15 lore raw
Quoted 11 lines
Quoted 6 lines

Another potential question is whether using the mdiobus lock (which excludes all other MII bus access) is best - while it has the advantage of also ensuring atomicity with userspace accesses, it means that no one else can access an independent PHY on the same bus while a paged access is on-going. It feels like a big hammer, but I'm not convinced that we will see a lot of contention on it.

Regarding that last topic, this could become a fairly contended lock on a switch with lots (e.g: > 5-6) of built-in PHYs, all being polled (which is usually the case right now). One would expect that the polling should be limited to 2 BMSR reads to minimize the bus utilization.

Hi Florian

In this case, we probably are not doing pages reads, just normal reads. So there should not be any more contention than there already is.

Andrew