| Message ID | 1436547449-26927-1-git-send-email-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)