[PATCH net-next 0/7] introduce read_poll_timeout

[PATCH net-next 0/7] introduce read_poll_timeout

[Dejin Zheng]

This patch sets is introduce read_poll_timeout macro, it is an extension
of readx_poll_timeout macro. the accessor function op just supports only
one parameter in the readx_poll_timeout macro, but this macro can
supports multiple variable parameters for it. so functions like
phy_read(struct phy_device *phydev, u32 regnum) and
phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
use this poll timeout framework.

the first patch introduce read_poll_timeout macro, and the second patch
redefined readx_poll_timeout macro by read_poll_timeout(), and the other
patches are examples using read_poll_timeout macro.


Dejin Zheng (7):
  iopoll: introduce read_poll_timeout macro
  iopoll: redefined readx_poll_timeout macro to simplify the code
  net: phy: introduce phy_read_mmd_poll_timeout macro
  net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the
    code
  net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the
    code
  net: phy: introduce phy_read_poll_timeout macro
  net: phy: use phy_read_poll_timeout() to simplify the code

 drivers/net/phy/aquantia_main.c | 16 +++++++--------
 drivers/net/phy/bcm84881.c      | 24 ++++++----------------
 drivers/net/phy/phy_device.c    | 18 ++++++-----------
 include/linux/iopoll.h          | 36 ++++++++++++++++++++++++++-------
 include/linux/phy.h             |  7 +++++++
 5 files changed, 55 insertions(+), 46 deletions(-)

-- 
2.25.0

[PATCH net-next 1/7] iopoll: introduce read_poll_timeout macro

[Dejin Zheng]

this macro is an extension of readx_poll_timeout macro. the accessor
function op just supports only one parameter in the readx_poll_timeout
macro, but this macro can supports multiple variable parameters for
it. so functions like phy_read(struct phy_device *phydev, u32 regnum)
and phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
also use this poll timeout framework.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 include/linux/iopoll.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 35e15dfd4155..7d44a2e20267 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -13,6 +13,46 @@
 #include <linux/errno.h>
 #include <linux/io.h>
 
+/**
+ * read_poll_timeout - Periodically poll an address until a condition is
+ *			met or a timeout occurs
+ * @op: accessor function (takes @args as its arguments)
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0
+ *            tight-loops).  Should be less than ~20ms since usleep_range
+ *            is used (see Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @args: arguments for @op poll
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val. Must not
+ * be called from atomic context if sleep_us or timeout_us are used.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, args...)	\
+({ \
+	u64 __timeout_us = (timeout_us); \
+	unsigned long __sleep_us = (sleep_us); \
+	ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+	might_sleep_if((__sleep_us) != 0); \
+	for (;;) { \
+		(val) = op(args); \
+		if (cond) \
+			break; \
+		if (__timeout_us && \
+		    ktime_compare(ktime_get(), __timeout) > 0) { \
+			(val) = op(args); \
+			break; \
+		} \
+		if (__sleep_us) \
+			usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
+	} \
+	(cond) ? 0 : -ETIMEDOUT; \
+})
+
 /**
  * readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
  * @op: accessor function (takes @addr as its only argument)
-- 
2.25.0

[PATCH net-next 2/7] iopoll: redefined readx_poll_timeout macro to simplify the code

[Dejin Zheng]

redefined readx_poll_timeout macro by read_poll_timeout to
simplify the code.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 include/linux/iopoll.h | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 7d44a2e20267..29c016cd6249 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -72,25 +72,7 @@
  * macros defined below rather than this macro directly.
  */
 #define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us)	\
-({ \
-	u64 __timeout_us = (timeout_us); \
-	unsigned long __sleep_us = (sleep_us); \
-	ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
-	might_sleep_if((__sleep_us) != 0); \
-	for (;;) { \
-		(val) = op(addr); \
-		if (cond) \
-			break; \
-		if (__timeout_us && \
-		    ktime_compare(ktime_get(), __timeout) > 0) { \
-			(val) = op(addr); \
-			break; \
-		} \
-		if (__sleep_us) \
-			usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
-	} \
-	(cond) ? 0 : -ETIMEDOUT; \
-})
+	read_poll_timeout(op, val, cond, sleep_us, timeout_us, addr)
 
 /**
  * readx_poll_timeout_atomic - Periodically poll an address until a condition is met or a timeout occurs
-- 
2.25.0

[PATCH net-next 3/7] net: phy: introduce phy_read_mmd_poll_timeout macro

[Dejin Zheng]

it is sometimes necessary to poll a phy register by phy_read_mmd()
function until its value satisfies some condition. introduce
phy_read_mmd_poll_timeout() macros that do this.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 include/linux/phy.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36d9dea04016..a30e9008647f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -24,6 +24,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/u64_stats_sync.h>
 #include <linux/irqreturn.h>
+#include <linux/iopoll.h>
 
 #include <linux/atomic.h>
 
@@ -784,6 +785,9 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
  */
 int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
 
+#define phy_read_mmd_poll_timeout(val, cond, sleep_us, timeout_us, args...) \
+	read_poll_timeout(phy_read_mmd, val, cond, sleep_us, timeout_us, args)
+
 /**
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
-- 
2.25.0

Re: [PATCH net-next 3/7] net: phy: introduce phy_read_mmd_poll_timeout macro

[Heiner Kallweit]

On 19.03.2020 17:39, Dejin Zheng wrote:
> it is sometimes necessary to poll a phy register by phy_read_mmd()
> function until its value satisfies some condition. introduce
> phy_read_mmd_poll_timeout() macros that do this.
> 
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
>  include/linux/phy.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36d9dea04016..a30e9008647f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -24,6 +24,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/u64_stats_sync.h>
>  #include <linux/irqreturn.h>
> +#include <linux/iopoll.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -784,6 +785,9 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
>   */
>  int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
>  
> +#define phy_read_mmd_poll_timeout(val, cond, sleep_us, timeout_us, args...) \
> +	read_poll_timeout(phy_read_mmd, val, cond, sleep_us, timeout_us, args)
> +
>  /**
>   * __phy_read_mmd - Convenience function for reading a register
>   * from an MMD on a given PHY.
> 
I'm not fully convinced. Usage of the new macro with its lots of
parameters makes the code quite hard to read. Therefore I'm also
not a big fan of readx_poll_timeout.

Even though I didn't invent it, I prefer the way DECLARE_RTL_COND
is used in the r8169 driver. The resulting code is much better
to read, and in case of a timeout a helpful warning is printed
automatically.

Re: [PATCH net-next 3/7] net: phy: introduce phy_read_mmd_poll_timeout macro

[Andrew Lunn]

On Thu, Mar 19, 2020 at 05:56:39PM +0100, Heiner Kallweit wrote:
> On 19.03.2020 17:39, Dejin Zheng wrote:
> > it is sometimes necessary to poll a phy register by phy_read_mmd()
> > function until its value satisfies some condition. introduce
> > phy_read_mmd_poll_timeout() macros that do this.
> > 
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> >  include/linux/phy.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 36d9dea04016..a30e9008647f 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -24,6 +24,7 @@
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/u64_stats_sync.h>
> >  #include <linux/irqreturn.h>
> > +#include <linux/iopoll.h>
> >  
> >  #include <linux/atomic.h>
> >  
> > @@ -784,6 +785,9 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
> >   */
> >  int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
> >  
> > +#define phy_read_mmd_poll_timeout(val, cond, sleep_us, timeout_us, args...) \
> > +	read_poll_timeout(phy_read_mmd, val, cond, sleep_us, timeout_us, args)
> > +
> >  /**
> >   * __phy_read_mmd - Convenience function for reading a register
> >   * from an MMD on a given PHY.
> > 
> I'm not fully convinced. Usage of the new macro with its lots of
> parameters makes the code quite hard to read. Therefore I'm also
> not a big fan of readx_poll_timeout.

One issue is that people often implement polling wrong. They don't
take into account where extra delays can happen, and return -ETIMEOUT
when in fact the operation was successful. readx_poll_timeout gives us
a core which is known to be good.

What i don't like here is phy_read_mmd_poll_timeout() takes args... We
know it should be passed a phydev, and device address and a reg. I
would prefer these to be explicit parameters. We can then get the
compiler to check the correct number of parameters are passed.

      Andrew

[PATCH net-next 4/7] net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the code

[Dejin Zheng]

use phy_read_mmd_poll_timeout() to replace the poll codes for
simplify the code in bcm84881_wait_init() function.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/net/phy/bcm84881.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 14d55a77eb28..c916bd0f6afa 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -22,26 +22,14 @@ enum {
 
 static int bcm84881_wait_init(struct phy_device *phydev)
 {
-	unsigned int tries = 20;
 	int ret, val;
 
-	do {
-		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
-		if (val < 0) {
-			ret = val;
-			break;
-		}
-		if (!(val & MDIO_CTRL1_RESET)) {
-			ret = 0;
-			break;
-		}
-		if (!--tries) {
-			ret = -ETIMEDOUT;
-			break;
-		}
-		msleep(100);
-	} while (1);
-
+	ret = phy_read_mmd_poll_timeout(val, val < 0 ||
+					!(val & MDIO_CTRL1_RESET), 100000,
+					2000000, phydev, MDIO_MMD_PMAPMD,
+					MDIO_CTRL1);
+	if (val < 0)
+		ret = val;
 	if (ret)
 		phydev_err(phydev, "%s failed: %d\n", __func__, ret);
 
-- 
2.25.0

Re: [PATCH net-next 4/7] net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the code

[Andrew Lunn]

On Fri, Mar 20, 2020 at 12:39:07AM +0800, Dejin Zheng wrote:
> use phy_read_mmd_poll_timeout() to replace the poll codes for
> simplify the code in bcm84881_wait_init() function.
> 
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
>  drivers/net/phy/bcm84881.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
> index 14d55a77eb28..c916bd0f6afa 100644
> --- a/drivers/net/phy/bcm84881.c
> +++ b/drivers/net/phy/bcm84881.c
> @@ -22,26 +22,14 @@ enum {
>  
>  static int bcm84881_wait_init(struct phy_device *phydev)
>  {
> -	unsigned int tries = 20;
>  	int ret, val;
>  
> -	do {
> -		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> -		if (val < 0) {
> -			ret = val;
> -			break;
> -		}
> -		if (!(val & MDIO_CTRL1_RESET)) {
> -			ret = 0;
> -			break;
> -		}
> -		if (!--tries) {
> -			ret = -ETIMEDOUT;
> -			break;
> -		}
> -		msleep(100);
> -	} while (1);
> -
> +	ret = phy_read_mmd_poll_timeout(val, val < 0 ||
> +					!(val & MDIO_CTRL1_RESET), 100000,

It would be good if the "val < 0" was not here. What the user is
really interested in is !(val & MDIO_CTRL1_RESET). "val < 0" is the
error handling for phy_read_mmd(). This would look a lot nicer if that
error handling was inside phy_read_mmd_poll_timeout() and ret would
already contain the error from phy_read_mmd(). Not sure you can do
this and still make use of readx_poll_timeout().

     Andrew

[PATCH net-next 5/7] net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the code

[Dejin Zheng]

use phy_read_mmd_poll_timeout() to replace the poll codes for
simplify the code in aqr107_wait_reset_complete() function.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 31927b2c7d5a..fdd037383217 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -451,16 +451,14 @@ static int aqr107_set_tunable(struct phy_device *phydev,
  */
 static int aqr107_wait_reset_complete(struct phy_device *phydev)
 {
-	int val, retries = 100;
-
-	do {
-		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
-		if (val < 0)
-			return val;
-		msleep(20);
-	} while (!val && --retries);
+	int val, ret;
 
-	return val ? 0 : -ETIMEDOUT;
+	ret = phy_read_mmd_poll_timeout(val, val < 0 || val != 0, 20000,
+					2000000, phydev, MDIO_MMD_VEND1,
+					VEND1_GLOBAL_FW_ID);
+	if (val < 0)
+		ret = val;
+	return ret;
 }
 
 static void aqr107_chip_info(struct phy_device *phydev)
-- 
2.25.0

[PATCH net-next 6/7] net: phy: introduce phy_read_poll_timeout macro

[Dejin Zheng]

it is sometimes necessary to poll a phy register by phy_read()
function until its value satisfies some condition. introduce
phy_read_poll_timeout() macros that do this.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a30e9008647f..a28cc16fdaac 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -714,6 +714,9 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
 	return mdiobus_read(phydev->mdio.bus, phydev->mdio.addr, regnum);
 }
 
+#define phy_read_poll_timeout(val, cond, sleep_us, timeout_us, args...) \
+	read_poll_timeout(phy_read, val, cond, sleep_us, timeout_us, args)
+
 /**
  * __phy_read - convenience function for reading a given PHY register
  * @phydev: the phy_device struct
-- 
2.25.0

[PATCH net-next 7/7] net: phy: use phy_read_poll_timeout() to simplify the code

[Dejin Zheng]

use phy_read_poll_timeout() to replace the poll codes for
simplify the code in phy_poll_reset() function.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/net/phy/phy_device.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a585faf8b844..bdef427593c9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1059,23 +1059,17 @@ EXPORT_SYMBOL(phy_disconnect);
 static int phy_poll_reset(struct phy_device *phydev)
 {
 	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
-	unsigned int retries = 12;
-	int ret;
-
-	do {
-		msleep(50);
-		ret = phy_read(phydev, MII_BMCR);
-		if (ret < 0)
-			return ret;
-	} while (ret & BMCR_RESET && --retries);
-	if (ret & BMCR_RESET)
-		return -ETIMEDOUT;
+	int ret, val;
 
+	ret = phy_read_poll_timeout(val, val < 0 || !(val & BMCR_RESET),
+				    50000, 600000, phydev, MII_BMCR);
+	if (val < 0)
+		ret = val;
 	/* Some chips (smsc911x) may still need up to another 1ms after the
 	 * BMCR_RESET bit is cleared before they are usable.
 	 */
 	msleep(1);
-	return 0;
+	return ret;
 }
 
 int phy_init_hw(struct phy_device *phydev)
-- 
2.25.0

Re: [PATCH net-next 0/7] introduce read_poll_timeout

[Florian Fainelli]

Le 2020-03-19 à 09:39, Dejin Zheng a écrit :
> This patch sets is introduce read_poll_timeout macro, it is an extension
> of readx_poll_timeout macro. the accessor function op just supports only
> one parameter in the readx_poll_timeout macro, but this macro can
> supports multiple variable parameters for it. so functions like
> phy_read(struct phy_device *phydev, u32 regnum) and
> phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
> use this poll timeout framework.
> 
> the first patch introduce read_poll_timeout macro, and the second patch
> redefined readx_poll_timeout macro by read_poll_timeout(), and the other
> patches are examples using read_poll_timeout macro.
> 
> 
> Dejin Zheng (7):
>   iopoll: introduce read_poll_timeout macro
>   iopoll: redefined readx_poll_timeout macro to simplify the code
>   net: phy: introduce phy_read_mmd_poll_timeout macro
>   net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the
>     code
>   net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the
>     code
>   net: phy: introduce phy_read_poll_timeout macro
>   net: phy: use phy_read_poll_timeout() to simplify the code
> 
>  drivers/net/phy/aquantia_main.c | 16 +++++++--------
>  drivers/net/phy/bcm84881.c      | 24 ++++++----------------
>  drivers/net/phy/phy_device.c    | 18 ++++++-----------
>  include/linux/iopoll.h          | 36 ++++++++++++++++++++++++++-------
>  include/linux/phy.h             |  7 +++++++
>  5 files changed, 55 insertions(+), 46 deletions(-)

Your diffstat is positive, so what's the point of doing this? What
problem are you trying to solve?
-- 
Florian

Re: [PATCH net-next 0/7] introduce read_poll_timeout

[Dejin Zheng]

On Thu, Mar 19, 2020 at 09:42:42AM -0700, Florian Fainelli wrote:
> Le 2020-03-19 à 09:39, Dejin Zheng a écrit :
> > This patch sets is introduce read_poll_timeout macro, it is an extension
> > of readx_poll_timeout macro. the accessor function op just supports only
> > one parameter in the readx_poll_timeout macro, but this macro can
> > supports multiple variable parameters for it. so functions like
> > phy_read(struct phy_device *phydev, u32 regnum) and
> > phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
> > use this poll timeout framework.
> > 
> > the first patch introduce read_poll_timeout macro, and the second patch
> > redefined readx_poll_timeout macro by read_poll_timeout(), and the other
> > patches are examples using read_poll_timeout macro.
> > 
> > 
> > Dejin Zheng (7):
> >   iopoll: introduce read_poll_timeout macro
> >   iopoll: redefined readx_poll_timeout macro to simplify the code
> >   net: phy: introduce phy_read_mmd_poll_timeout macro
> >   net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the
> >     code
> >   net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the
> >     code
> >   net: phy: introduce phy_read_poll_timeout macro
> >   net: phy: use phy_read_poll_timeout() to simplify the code
> > 
> >  drivers/net/phy/aquantia_main.c | 16 +++++++--------
> >  drivers/net/phy/bcm84881.c      | 24 ++++++----------------
> >  drivers/net/phy/phy_device.c    | 18 ++++++-----------
> >  include/linux/iopoll.h          | 36 ++++++++++++++++++++++++++-------
> >  include/linux/phy.h             |  7 +++++++
> >  5 files changed, 55 insertions(+), 46 deletions(-)
> 
> Your diffstat is positive, so what's the point of doing this? What
> problem are you trying to solve?

Since I added a lot of code comments(20 lines) in the first patch, so the
diffstat is positive.

this patches just want to fix an issue that people often implement polling
is wrong. we use a poll core which is known to be good by readx_poll_timeout
gives us. It can support multiple parameters after extending
readx_poll_timeout, so phy_read() and phy_read_mmd() also can use this
poll core. 

> -- 
> Florian