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

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

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

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

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

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

[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 <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/mdio_bus.c | 65 +++++++++++++++++++++++++++++++++++++---------
 include/linux/mdio.h       |  3 +++
 2 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2df7b62c1a36..20fd8128b03c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -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);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index ca08ab16ecdc..4be30adc033b 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -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);
-- 
2.7.4

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

[Florian Fainelli]

On 12/08/2017 07:48 AM, Russell King wrote:
> 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 <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Russell King]

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

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 21f75ae244b3..83d32644cb4d 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -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;
-- 
2.7.4

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

[Florian Fainelli]

On 12/08/2017 07:48 AM, Russell King wrote:
> Use unlocked accessors for indirect MMD accesses to clause 22 PHYs.
> This permits tracing of these accesses.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[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 <rmk+kernel@armlinux.org.uk>
---
 include/linux/phy.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 71d777fe6c3d..964803bd7324 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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
  *
-- 
2.7.4

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

[Florian Fainelli]

On 12/08/2017 07:48 AM, Russell King wrote:
> 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 <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  include/linux/phy.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 71d777fe6c3d..964803bd7324 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -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.
-- 
Florian

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

[Russell King - ARM Linux]

On Sat, Dec 09, 2017 at 10:22:14AM -0800, Florian Fainelli wrote:
> 
> 
> On 12/08/2017 07:48 AM, Russell King wrote:
> > 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 <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> > ---
> >  include/linux/phy.h | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 71d777fe6c3d..964803bd7324 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -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.

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.

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

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

[Russell King]

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 <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell.c | 365 +++++++++++++++++++++-------------------------
 1 file changed, 168 insertions(+), 197 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 4d02b27df044..87b76db456cf 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -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,
-- 
2.7.4

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

[Andrew Lunn]

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

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.

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

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

> 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

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

[Russell King - ARM Linux]

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

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

[Florian Fainelli]

On 12/08/2017 08:44 AM, Russell King - ARM Linux wrote:
> 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.

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

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

[Russell King - ARM Linux]

On Sat, Dec 09, 2017 at 10:22:58AM -0800, Florian Fainelli wrote:
> On 12/08/2017 08:44 AM, Russell King - ARM Linux wrote:
> > 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.
> 
> 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.

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

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

[Andrew Lunn]

> 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

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

[Florian Fainelli]

On 12/08/2017 07:47 AM, Russell King - ARM Linux wrote:
> 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.

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.

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

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.

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

-- 
Florian

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

[Andrew Lunn]

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