Spurious timeouts in mvmdio

Commit Message

Leigh Brown Dec. 3, 2013, 11:38 p.m. UTC

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.

I think I might be being a little to anal about this, but I guess what 
you could do
is make the above change, then replace the call to 
time_is_before_jiffies() with
time_is_before_eq_jiffies() so that the behaviour when polling and when 
using
interrupts is the same.


@@ -89,7 +89,7 @@ static int orion_mdio_wait_ready(struct mii_bus *bus)
  			usleep_range(MVMDIO_SMI_POLL_INTERVAL_MIN,
  				     MVMDIO_SMI_POLL_INTERVAL_MAX);

-			if (time_is_before_jiffies(end))
+			if (time_is_before_eq_jiffies(end))
  				++timedout;
  	        } else {
  			wait_event_timeout(dev->smi_busy_wait,

Regards,

Leigh.

Comments

Sebastian Hesselbarth Dec. 3, 2013, 11:45 p.m. UTC | #1

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

Patch

diff --git a/drivers/net/ethernet/marvell/mvmdio.c 
b/drivers/net/ethernet/marvell/mvmdio.c
index 7354960..48b8ded 100644
--- a/drivers/net/ethernet/marvell/mvmdio.c
+++ b/drivers/net/ethernet/marvell/mvmdio.c
@@ -75,7 +75,7 @@  static int orion_mdio_smi_is_done(struct 
orion_mdio_dev *dev)
  static int orion_mdio_wait_ready(struct mii_bus *bus)
  {
  	struct orion_mdio_dev *dev = bus->priv;
-	unsigned long timeout = usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
+	unsigned long timeout = 1 + usecs_to_jiffies(MVMDIO_SMI_TIMEOUT);
  	unsigned long end = jiffies + timeout;
  	int timedout = 0;