[PATCH net-next 0/3] Fix mv88e6xxx wait function

[PATCH net-next 0/3] Fix mv88e6xxx wait function

[Andrew Lunn]

The mv88e6xxx wait function can be upset of the system has nots of
other things to do and a sleep takes a lot longer than expected. Fix
this be using a fixed number of iterations, rather than a fixed
walkclock time.

Witht that change made, it is possible to consoliate another
wait function.

A wait actually timing out should not happen and when it does, it
means something serious is wrong. Make sure an error is logged,
since not all callers will log an error.

Andrew Lunn (3):
  dsa: mv88e6xxx: Timeout based on iterations, not time
  dsa: mv88e6xxx: Use mv88e6xx_wait in mv88e6xxx_update()
  dsa: mv88e6xxx: Make mv88e6xxx_wait() timeout verbose

 drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

-- 
2.8.1

[PATCH net-next 1/3] dsa: mv88e6xxx: Timeout based on iterations, not time

[Andrew Lunn]

The mv88e6xxx driver times out operations on the switch based on
looping until an elapsed wall clock time is reached. However, if
usleep_range() sleeps much longer than expected, it could timeout with
an error without actually checking to see if the devices has completed
the operation. So replace the elapsed time with a fixed upper bound on
the number of loops.

Testing on various switches has shown that switches takes either 0 or
1 iteration, so a maximum of 16 iterations is a safe limit.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index a230fcba5b64..ac8e9af4879f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -309,9 +309,9 @@ static int mv88e6xxx_serdes_write(struct mv88e6xxx_chip *chip, int reg, u16 val)
 static int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg,
 			  u16 mask)
 {
-	unsigned long timeout = jiffies + HZ / 10;
+	int i;
 
-	while (time_before(jiffies, timeout)) {
+	for (i = 0; i < 16; i++) {
 		u16 val;
 		int err;
 
@@ -375,7 +375,7 @@ static int _mv88e6xxx_reg_write(struct mv88e6xxx_chip *chip, int addr,
 static int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip)
 {
 	int ret;
-	unsigned long timeout;
+	int i;
 
 	ret = _mv88e6xxx_reg_read(chip, REG_GLOBAL, GLOBAL_CONTROL);
 	if (ret < 0)
@@ -386,8 +386,7 @@ static int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip)
 	if (ret)
 		return ret;
 
-	timeout = jiffies + 1 * HZ;
-	while (time_before(jiffies, timeout)) {
+	for (i = 0; i < 16; i++) {
 		ret = _mv88e6xxx_reg_read(chip, REG_GLOBAL, GLOBAL_STATUS);
 		if (ret < 0)
 			return ret;
@@ -403,8 +402,7 @@ static int mv88e6xxx_ppu_disable(struct mv88e6xxx_chip *chip)
 
 static int mv88e6xxx_ppu_enable(struct mv88e6xxx_chip *chip)
 {
-	int ret, err;
-	unsigned long timeout;
+	int ret, err, i;
 
 	ret = _mv88e6xxx_reg_read(chip, REG_GLOBAL, GLOBAL_CONTROL);
 	if (ret < 0)
@@ -415,8 +413,7 @@ static int mv88e6xxx_ppu_enable(struct mv88e6xxx_chip *chip)
 	if (err)
 		return err;
 
-	timeout = jiffies + 1 * HZ;
-	while (time_before(jiffies, timeout)) {
+	for (i = 0; i < 16; i++) {
 		ret = _mv88e6xxx_reg_read(chip, REG_GLOBAL, GLOBAL_STATUS);
 		if (ret < 0)
 			return ret;
-- 
2.8.1

[PATCH net-next 2/3] dsa: mv88e6xxx: Use mv88e6xx_wait in mv88e6xxx_update()

[Andrew Lunn]

Now that mv88e6xx_wait() iterated on a counter than a fixed time
interval, it implements the same mechanism as mv88e6xxx_update() uses.
So use it in mv88e6xx_wait().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index ac8e9af4879f..8c846bce4edf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -333,20 +333,12 @@ static int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, int reg,
 			    u16 update)
 {
 	u16 val;
-	int i, err;
+	int err;
 
 	/* Wait until the previous operation is completed */
-	for (i = 0; i < 16; ++i) {
-		err = mv88e6xxx_read(chip, addr, reg, &val);
-		if (err)
-			return err;
-
-		if (!(val & BIT(15)))
-			break;
-	}
-
-	if (i == 16)
-		return -ETIMEDOUT;
+	err = mv88e6xxx_wait(chip, addr, reg, BIT(15));
+	if (err)
+		return err;
 
 	/* Set the Update bit to trigger a write operation */
 	val = BIT(15) | update;
-- 
2.8.1

[PATCH net-next 3/3] dsa: mv88e6xxx: Make mv88e6xxx_wait() timeout verbose

[Andrew Lunn]

When mv88e6xxx_wait() returns a timeout, something bad has
happened. Make sure it is noticed by logging an error.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8c846bce4edf..014b52bd72f1 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -325,6 +325,7 @@ static int mv88e6xxx_wait(struct mv88e6xxx_chip *chip, int addr, int reg,
 		usleep_range(1000, 2000);
 	}
 
+	dev_err(chip->dev, "Timeout while waiting for switch\n");
 	return -ETIMEDOUT;
 }
 
-- 
2.8.1

Re: [PATCH net-next 0/3] Fix mv88e6xxx wait function

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

> The mv88e6xxx wait function can be upset of the system has nots of
> other things to do and a sleep takes a lot longer than expected. Fix
> this be using a fixed number of iterations, rather than a fixed
> walkclock time.
>
> Witht that change made, it is possible to consoliate another
> wait function.
>
> A wait actually timing out should not happen and when it does, it
> means something serious is wrong. Make sure an error is logged,
> since not all callers will log an error.
>
> Andrew Lunn (3):
>   dsa: mv88e6xxx: Timeout based on iterations, not time
>   dsa: mv88e6xxx: Use mv88e6xx_wait in mv88e6xxx_update()
>   dsa: mv88e6xxx: Make mv88e6xxx_wait() timeout verbose
>
>  drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

David, this series superseeds http://patchwork.ozlabs.org/patch/660270/.

Thanks,

        Vivien

Re: [PATCH net-next 0/3] Fix mv88e6xxx wait function

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 19 Aug 2016 00:01:54 +0200

> The mv88e6xxx wait function can be upset of the system has nots of
> other things to do and a sleep takes a lot longer than expected. Fix
> this be using a fixed number of iterations, rather than a fixed
> walkclock time.
> 
> Witht that change made, it is possible to consoliate another
> wait function.
> 
> A wait actually timing out should not happen and when it does, it
> means something serious is wrong. Make sure an error is logged,
> since not all callers will log an error.

Series applied, thanks.