Re: Spurious timeouts in mvmdio

[Sebastian Hesselbarth]

On 12/04/2013 12:38 AM, Leigh Brown wrote:
> On 2013-12-03 23:17, Sebastian Hesselbarth wrote:
> > On 12/04/2013 12:20 AM, Leigh Brown wrote:
> > > On 2013-12-03 22:45, Sebastian Hesselbarth wrote:
> > > > On 12/03/2013 09:57 PM, Leigh Brown wrote:
> > > [...]
> > > > > Nicolas' patch should fix the issue, but I prefer the following as
> > > > > it is
> > > > > more
> > > > > correct, as it only adjusts the timeout when calling
> > > > > wait_event_timeout().  As
> > > > > I said above,I believe the polling code is correct.
> > > > >
> > > > > diff --git a/drivers/net/ethernet/marvell/mvmdio.c
> > > > > b/drivers/net/ethernet/marvell/mvmdio.c
> > > > > index 7354960..b187c08 100644
> > > > > --- a/drivers/net/ethernet/marvell/mvmdio.c
> > > > > +++ b/drivers/net/ethernet/marvell/mvmdio.c
> > > > > @@ -92,6 +92,14 @@ static int orion_mdio_wait_ready(struct mii_bus
> > > > > *bus)
> > > > >               if (time_is_before_jiffies(end))
> > > > >                   ++timedout;
> > > > >               } else {
> > > > > +            /*
> > > > > +             * wait_event_timeout does not guarantee a delay of at
> > > > > +             * least one whole jiffie, so timeout must be no less
> > > > > +             * than two.
> > > > > +             */
> > > > > +            if (timeout < 2)
> > > > > +                timeout = 2;
> > > >
> > > > If you always want to wait at least two jiffies, why not just increase
> > > > TIMEOUT makro to 20ms instead of messing here with it again?
> > > > As said on IRC log above, originally timeout was 100ms.
> > >
> > > You could do that, but would you not feel bad leaving a latent bug in
> > > the code?
> > > I know it's unlikely that someone would set HZ to 50, but if they did,
> > > the same
> > > bug would appear again.
> >
> > If you want to ensure timeout > 2, why not then just use:
> >
> > -     unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
> > +     unsigned long timeout = 1 + usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
>
> This will make it correct when using interrupts but it will make the
> loop wait one
> jiffie longer than it should when polling.

Leigh,

it is not about "waiting longer than it should" but increasing the
_timeout_ to more than one jiffy. If you poll and everything is fine
with mdio, you exit way before timeout. If mdio hangs, it does not
really matter if you timeout after 10, 20, or even 100ms.

Anyway, I am fine with every patch that increases timeout to more than
one jiffy. Also, Russell just sent a patch to separate irq/polling
completely. Maybe give it a try.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/