[PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Vivien Didelot]

The current _mv88e6xxx_stats_wait function does not sleep while testing
the stats busy bit. Fix this by using the generic _mv88e6xxx_wait
function.

Note that it requires to move _mv88e6xxx_wait on top of
_mv88e6xxx_stats_wait to avoid undefined reference compilation error.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 52 +++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index f6c7409..7753db1 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -557,19 +557,31 @@ static bool mv88e6xxx_6352_family(struct dsa_switch *ds)
 	return false;
 }
 
+/* Must be called with SMI lock held */
+static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
+			   u16 mask)
+{
+	unsigned long timeout = jiffies + HZ / 10;
+
+	while (time_before(jiffies, timeout)) {
+		int ret;
+
+		ret = _mv88e6xxx_reg_read(ds, reg, offset);
+		if (ret < 0)
+			return ret;
+		if (!(ret & mask))
+			return 0;
+
+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;
+}
+
 /* Must be called with SMI mutex held */
 static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
 {
-	int ret;
-	int i;
-
-	for (i = 0; i < 10; i++) {
-		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_STATS_OP);
-		if ((ret & GLOBAL_STATS_OP_BUSY) == 0)
-			return 0;
-	}
-
-	return -ETIMEDOUT;
+	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_STATS_OP,
+			       GLOBAL_STATS_OP_BUSY);
 }
 
 /* Must be called with SMI mutex held */
@@ -856,26 +868,6 @@ error:
 }
 #endif /* CONFIG_NET_DSA_HWMON */
 
-/* Must be called with SMI lock held */
-static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
-			   u16 mask)
-{
-	unsigned long timeout = jiffies + HZ / 10;
-
-	while (time_before(jiffies, timeout)) {
-		int ret;
-
-		ret = _mv88e6xxx_reg_read(ds, reg, offset);
-		if (ret < 0)
-			return ret;
-		if (!(ret & mask))
-			return 0;
-
-		usleep_range(1000, 2000);
-	}
-	return -ETIMEDOUT;
-}
-
 static int mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-- 
2.4.5

[PATCH 2/2] net: dsa: mv88e6xxx: call _mv88e6xxx_stats_wait with SMI lock held

[Vivien Didelot]

At switch setup, _mv88e6xxx_stats_wait was called without holding the
SMI mutex. Fix this by requesting the lock for this call.

Also, return the _mv88e6xxx_stats_wait code, since it may fail.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 7753db1..cbbc33a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1961,6 +1961,7 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds)
 int mv88e6xxx_setup_global(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ret;
 	int i;
 
 	/* Set the default address aging time to 5 minutes, and
@@ -2059,9 +2060,11 @@ int mv88e6xxx_setup_global(struct dsa_switch *ds)
 	REG_WRITE(REG_GLOBAL, GLOBAL_STATS_OP, GLOBAL_STATS_OP_FLUSH_ALL);
 
 	/* Wait for the flush to complete. */
-	_mv88e6xxx_stats_wait(ds);
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_stats_wait(ds);
+	mutex_unlock(&ps->smi_mutex);
 
-	return 0;
+	return ret;
 }
 
 int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
-- 
2.4.5

Re: [PATCH 2/2] net: dsa: mv88e6xxx: call _mv88e6xxx_stats_wait with SMI lock held

[Guenter]

On Fri, Jul 10, 2015 at 12:57:29PM -0400, Vivien Didelot wrote:
> At switch setup, _mv88e6xxx_stats_wait was called without holding the
> SMI mutex. Fix this by requesting the lock for this call.
> 
> Also, return the _mv88e6xxx_stats_wait code, since it may fail.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Good catch.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Guenter]

On Fri, Jul 10, 2015 at 12:57:28PM -0400, Vivien Didelot wrote:
> The current _mv88e6xxx_stats_wait function does not sleep while testing
> the stats busy bit. Fix this by using the generic _mv88e6xxx_wait
> function.
> 
> Note that it requires to move _mv88e6xxx_wait on top of
> _mv88e6xxx_stats_wait to avoid undefined reference compilation error.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 52 +++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index f6c7409..7753db1 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -557,19 +557,31 @@ static bool mv88e6xxx_6352_family(struct dsa_switch *ds)
>  	return false;
>  }
>  
> +/* Must be called with SMI lock held */
> +static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
> +			   u16 mask)
> +{
> +	unsigned long timeout = jiffies + HZ / 10;
> +
> +	while (time_before(jiffies, timeout)) {
> +		int ret;
> +
> +		ret = _mv88e6xxx_reg_read(ds, reg, offset);
> +		if (ret < 0)
> +			return ret;
> +		if (!(ret & mask))
> +			return 0;
> +
> +		usleep_range(1000, 2000);
> +	}
> +	return -ETIMEDOUT;
> +}
> +
>  /* Must be called with SMI mutex held */
>  static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
>  {
> -	int ret;
> -	int i;
> -
> -	for (i = 0; i < 10; i++) {
> -		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_STATS_OP);
> -		if ((ret & GLOBAL_STATS_OP_BUSY) == 0)
> -			return 0;
> -	}

Hi Vivien,

is this really beneficial and/or needed ? It adds at least 1ms delay
to a loop which did not have any delay at all unless the register
read itself was sleeping. Is the original function seen to return
a timeout error under some circumstances ?

Thanks,
Guenter

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Vivien Didelot]

Hi Guenter,

On Jul 10, 2015, at 1:10 PM, Guenter Roeck linux@roeck-us.net wrote:

> On Fri, Jul 10, 2015 at 12:57:28PM -0400, Vivien Didelot wrote:
>> The current _mv88e6xxx_stats_wait function does not sleep while testing
>> the stats busy bit. Fix this by using the generic _mv88e6xxx_wait
>> function.
>> 
>> Note that it requires to move _mv88e6xxx_wait on top of
>> _mv88e6xxx_stats_wait to avoid undefined reference compilation error.
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx.c | 52 +++++++++++++++++++--------------------------
>>  1 file changed, 22 insertions(+), 30 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index f6c7409..7753db1 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -557,19 +557,31 @@ static bool mv88e6xxx_6352_family(struct dsa_switch *ds)
>>  	return false;
>>  }
>>  
>> +/* Must be called with SMI lock held */
>> +static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
>> +			   u16 mask)
>> +{
>> +	unsigned long timeout = jiffies + HZ / 10;
>> +
>> +	while (time_before(jiffies, timeout)) {
>> +		int ret;
>> +
>> +		ret = _mv88e6xxx_reg_read(ds, reg, offset);
>> +		if (ret < 0)
>> +			return ret;
>> +		if (!(ret & mask))
>> +			return 0;
>> +
>> +		usleep_range(1000, 2000);
>> +	}
>> +	return -ETIMEDOUT;
>> +}
>> +
>>  /* Must be called with SMI mutex held */
>>  static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
>>  {
>> -	int ret;
>> -	int i;
>> -
>> -	for (i = 0; i < 10; i++) {
>> -		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_STATS_OP);
>> -		if ((ret & GLOBAL_STATS_OP_BUSY) == 0)
>> -			return 0;
>> -	}
> 
> Hi Vivien,
> 
> is this really beneficial and/or needed ?

Except using existing generic code, no.

> It adds at least 1ms delay to a loop which did not have any delay at
> all unless the register read itself was sleeping.

I must have missed where is the benefit from spin reading 10 times this
register, rather than sleeping 1ms between tests. Does this busy bit
behaves differently from the phy, atu, scratch, or vtu busy bits?

> Is the original function seen to return a timeout error under some
> circumstances ?

I didn't experience it myself, but I guess it may happen. In addition to
that, the current implementation doesn't check eventual read error.
That's why I saw a benefit in using _mv88e6xxx_wait().

Thanks,
-v

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Guenter Roeck]

Hi Vivien,

On Fri, Jul 10, 2015 at 02:20:47PM -0400, Vivien Didelot wrote:
> > 
> > is this really beneficial and/or needed ?
> 
> Except using existing generic code, no.
> 
> > It adds at least 1ms delay to a loop which did not have any delay at
> > all unless the register read itself was sleeping.
> 
> I must have missed where is the benefit from spin reading 10 times this
> register, rather than sleeping 1ms between tests. Does this busy bit
> behaves differently from the phy, atu, scratch, or vtu busy bits?
> 
Benefit is reaction time, mostly. If the result isn't ready after the
first spin, the new code path adds a mandatory 1-2ms delay. This could
add up to a lot if that kind of retry is seen a lot.

I don't now if there is a specific time limit for this busy bit,
and/or if it behaves differently than the others in terms of timing.

> > Is the original function seen to return a timeout error under some
> > circumstances ?
> 
> I didn't experience it myself, but I guess it may happen. In addition to
> that, the current implementation doesn't check eventual read error.
> That's why I saw a benefit in using _mv88e6xxx_wait().

Checking for a read error (or a timeout) is definitely a good thing.
I could also imagine that, for example, a "clear statistics" request
takes more time than currently supported. This is why I asked if you
had seen a timeout with the old code.

Personally I'd rather leave the wait loop alone and only introduce
error checking unless there is a reason to introduce a sleep,
but I'd like to hear Andrew's and/or Florian's opinion.

Thanks,
Guenter

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Vivien Didelot]

Hi Guenter,

On Jul 10, 2015, at 2:36 PM, Guenter Roeck linux@roeck-us.net wrote:

> Hi Vivien,
> 
> On Fri, Jul 10, 2015 at 02:20:47PM -0400, Vivien Didelot wrote:
>> > 
>> > is this really beneficial and/or needed ?
>> 
>> Except using existing generic code, no.
>> 
>> > It adds at least 1ms delay to a loop which did not have any delay at
>> > all unless the register read itself was sleeping.
>> 
>> I must have missed where is the benefit from spin reading 10 times this
>> register, rather than sleeping 1ms between tests. Does this busy bit
>> behaves differently from the phy, atu, scratch, or vtu busy bits?
>> 
> Benefit is reaction time, mostly. If the result isn't ready after the
> first spin, the new code path adds a mandatory 1-2ms delay. This could
> add up to a lot if that kind of retry is seen a lot.

To me, it looks like if this mandatory 1-2ms delay is an issue, then
_mv88e6xxx_wait must be fixed. Maybe reducing this delay is an option?

> I don't now if there is a specific time limit for this busy bit,
> and/or if it behaves differently than the others in terms of timing.
> 
>> > Is the original function seen to return a timeout error under some
>> > circumstances ?
>> 
>> I didn't experience it myself, but I guess it may happen. In addition to
>> that, the current implementation doesn't check eventual read error.
>> That's why I saw a benefit in using _mv88e6xxx_wait().
> 
> Checking for a read error (or a timeout) is definitely a good thing.
> I could also imagine that, for example, a "clear statistics" request
> takes more time than currently supported. This is why I asked if you
> had seen a timeout with the old code.
> 
> Personally I'd rather leave the wait loop alone and only introduce
> error checking unless there is a reason to introduce a sleep,
> but I'd like to hear Andrew's and/or Florian's opinion.

Andrew may not reply since he's on vacation, but I add Florian in Cc.

Thanks,
-v

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Guenter]

Hi Vivien,

On Fri, Jul 10, 2015 at 03:21:47PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> >> I must have missed where is the benefit from spin reading 10 times this
> >> register, rather than sleeping 1ms between tests. Does this busy bit
> >> behaves differently from the phy, atu, scratch, or vtu busy bits?
> >> 
> > Benefit is reaction time, mostly. If the result isn't ready after the
> > first spin, the new code path adds a mandatory 1-2ms delay. This could
> > add up to a lot if that kind of retry is seen a lot.
> 
> To me, it looks like if this mandatory 1-2ms delay is an issue, then
> _mv88e6xxx_wait must be fixed. Maybe reducing this delay is an option?
> 
Good point. The timeout is most definitely quite large and for sure on
the safe side. It might make sense to add some statistics gathering to
see how long the maximum observed delay actually is.

Guenter

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Andrew Lunn]

> Good point. The timeout is most definitely quite large and for sure on
> the safe side. It might make sense to add some statistics gathering to
> see how long the maximum observed delay actually is.

Hi All

Statistics are something which can be used a lot, i bursts and
interactivily. ATU, VTU etc, are much less often used. So different
delays might be justified.

I agree about doing some statistics gathering to determine actual
delays needed.

       Andrew

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Vivien Didelot]

Hi all,

----- On Jul 18, 2015, at 10:58 AM, Andrew Lunn andrew@lunn.ch wrote:

>> Good point. The timeout is most definitely quite large and for sure on
>> the safe side. It might make sense to add some statistics gathering to
>> see how long the maximum observed delay actually is.
> 
> Hi All
> 
> Statistics are something which can be used a lot, i bursts and
> interactivily. ATU, VTU etc, are much less often used. So different
> delays might be justified.
> 
> I agree about doing some statistics gathering to determine actual
> delays needed.
> 
>        Andrew

What do you think about something like this?

Thanks,
-v

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 4f3701f..6471807 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -563,9 +563,10 @@ static bool mv88e6xxx_6352_family(struct dsa_switch *ds)
 
 /* Must be called with SMI lock held */
 static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
-			   u16 mask)
+			   u16 mask, unsigned int msecs)
 {
 	unsigned long timeout = jiffies + HZ / 10;
+	unsigned long usecs = msecs * 1000;
 
 	while (time_before(jiffies, timeout)) {
 		int ret;
@@ -576,7 +577,8 @@ static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
 		if (!(ret & mask))
 			return 0;
 
-		usleep_range(1000, 2000);
+		if (usecs)
+			usleep_range(usecs, usecs + 1000);
 	}
 	return -ETIMEDOUT;
 }
@@ -585,7 +587,7 @@ static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
 static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
 {
 	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_STATS_OP,
-			       GLOBAL_STATS_OP_BUSY);
+			       GLOBAL_STATS_OP_BUSY, 0);
 }
 
 /* Must be called with SMI mutex held */
@@ -872,13 +874,14 @@ error:
 }
 #endif /* CONFIG_NET_DSA_HWMON */
 
-static int mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
+static int mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask,
+			  unsigned int msecs)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_wait(ds, reg, offset, mask);
+	ret = _mv88e6xxx_wait(ds, reg, offset, mask, msecs);
 	mutex_unlock(&ps->smi_mutex);
 
 	return ret;
@@ -887,33 +890,33 @@ static int mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask)
 static int _mv88e6xxx_phy_wait(struct dsa_switch *ds)
 {
 	return _mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_SMI_OP,
-			       GLOBAL2_SMI_OP_BUSY);
+			       GLOBAL2_SMI_OP_BUSY, 1);
 }
 
 int mv88e6xxx_eeprom_load_wait(struct dsa_switch *ds)
 {
 	return mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_EEPROM_OP,
-			      GLOBAL2_EEPROM_OP_LOAD);
+			      GLOBAL2_EEPROM_OP_LOAD, 1);
 }
 
 int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds)
 {
 	return mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_EEPROM_OP,
-			      GLOBAL2_EEPROM_OP_BUSY);
+			      GLOBAL2_EEPROM_OP_BUSY, 1);
 }
 
 /* Must be called with SMI lock held */
 static int _mv88e6xxx_atu_wait(struct dsa_switch *ds)
 {
 	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_ATU_OP,
-			       GLOBAL_ATU_OP_BUSY);
+			       GLOBAL_ATU_OP_BUSY, 1);
 }
 
 /* Must be called with SMI lock held */
 static int _mv88e6xxx_scratch_wait(struct dsa_switch *ds)
 {
 	return _mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_SCRATCH_MISC,
-			       GLOBAL2_SCRATCH_BUSY);
+			       GLOBAL2_SCRATCH_BUSY, 1);
 }
 
 /* Must be called with SMI mutex held */

Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

[Andrew Lunn]

On Sat, Jul 18, 2015 at 11:23:19AM -0400, Vivien Didelot wrote:
> Hi all,
> 
> ----- On Jul 18, 2015, at 10:58 AM, Andrew Lunn andrew@lunn.ch wrote:
> 
> >> Good point. The timeout is most definitely quite large and for sure on
> >> the safe side. It might make sense to add some statistics gathering to
> >> see how long the maximum observed delay actually is.
> > 
> > Hi All
> > 
> > Statistics are something which can be used a lot, i bursts and
> > interactivily. ATU, VTU etc, are much less often used. So different
> > delays might be justified.
> > 
> > I agree about doing some statistics gathering to determine actual
> > delays needed.
> > 
> >        Andrew
> 
> What do you think about something like this?

Hi Vivien

Lets get some actually statistics first. I would suggest for testing
you make _mv88e6xxx_wait() a busy loop and time how long it actually
takes for different busy bits to clear. We should also test it on
different families.

	  Andrew