Re: Spurious timeouts in mvmdio

From: Sebastian Hesselbarth
Date: Tue Dec 03 2013 - 18:18:09 EST


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);


--
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/