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

[Guenter Roeck <linux@roeck-us.net>]

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