netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: fec: Adjust ENET MDIO timeouts
@ 2011-12-27 14:46 Fabio Estevam
  2011-12-27 18:13 ` David Miller
  2011-12-27 18:53 ` [PATCH v2] " Fabio Estevam
  0 siblings, 2 replies; 4+ messages in thread
From: Fabio Estevam @ 2011-12-27 14:46 UTC (permalink / raw)
  To: netdev; +Cc: davem, shawn.guo, kernel, Rogerio Pimentel, Fabio Estevam

On extensive NFS boots on a mx6qsabrelite board it was noted that "FEC: MDIO read timeout" were occuring,
which caused failure on loading the FEC driver.

The original FEC_MII_TIMEOUT was set to 1 ms, which is too low when passed to the usecs_to_jiffies macro.

On ARM one jiffy is 10ms, so use msecs_to_jiffies instead and use a timeout of 30ms, which corresponds to 3 jiffies.

After running extensive NFS boots, the MDIO timeouts do not occur anymore with this change.

Signed-off-by: Rogerio Pimentel <rogerio.pimentel@freescale.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/net/ethernet/freescale/fec.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index c136230..170a015 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -255,7 +255,7 @@ struct fec_enet_private {
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 
-#define FEC_MII_TIMEOUT		1000 /* us */
+#define FEC_MII_TIMEOUT		30 /* ms */
 
 /* Transmitter timeout */
 #define TX_TIMEOUT (2 * HZ)
@@ -902,7 +902,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 
 	/* wait for end of transfer */
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
+			msecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		printk(KERN_ERR "FEC: MDIO read timeout\n");
@@ -930,7 +930,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 	/* wait for end of transfer */
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
+			msecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
 		fep->mii_timeout = 1;
 		printk(KERN_ERR "FEC: MDIO write timeout\n");
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: fec: Adjust ENET MDIO timeouts
  2011-12-27 14:46 [PATCH] net: fec: Adjust ENET MDIO timeouts Fabio Estevam
@ 2011-12-27 18:13 ` David Miller
  2011-12-27 18:53 ` [PATCH v2] " Fabio Estevam
  1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2011-12-27 18:13 UTC (permalink / raw)
  To: festevam; +Cc: netdev, shawn.guo, kernel, rogerio.pimentel, fabio.estevam

From: Fabio Estevam <festevam@gmail.com>
Date: Tue, 27 Dec 2011 12:46:38 -0200

> -#define FEC_MII_TIMEOUT		1000 /* us */
> +#define FEC_MII_TIMEOUT		30 /* ms */

I don't see why you must use msecs_to_jiffies(), why not just change
FEC_MII_TIMEOUT to 30000?

That's a one-line, easy to validate, change.

Whereas if you also change the units of the timeout I have to go
through the entire driver to audit your patch, making sure none of the
FEC_MII_TIMEOUT uses in the driver have been missed.

When you make multiple changes at once, it leads to more work for the
patch reviewer, please avoid this whenever possible.

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] net: fec: Adjust ENET MDIO timeouts
  2011-12-27 14:46 [PATCH] net: fec: Adjust ENET MDIO timeouts Fabio Estevam
  2011-12-27 18:13 ` David Miller
@ 2011-12-27 18:53 ` Fabio Estevam
  2011-12-27 19:07   ` David Miller
  1 sibling, 1 reply; 4+ messages in thread
From: Fabio Estevam @ 2011-12-27 18:53 UTC (permalink / raw)
  To: netdev; +Cc: davem, shawn.guo, kernel, Rogerio Pimentel, Fabio Estevam

On extensive NFS boots on a mx6qsabrelite board it was noted that "FEC: MDIO read timeout" were occuring,
which caused failure on loading the FEC driver.

The original FEC_MII_TIMEOUT was set to 1 ms, which is too low when passed to the usecs_to_jiffies macro.

On ARM one jiffy is 10ms, so use a timeout of 30ms, which corresponds to 3 jiffies.

After running extensive NFS boots, the MDIO timeouts do not occur anymore with this change.

Signed-off-by: Rogerio Pimentel <rogerio.pimentel@freescale.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Keep the timeout unit in us.

 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index c136230..c54cc62 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -255,7 +255,7 @@ struct fec_enet_private {
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 
-#define FEC_MII_TIMEOUT		1000 /* us */
+#define FEC_MII_TIMEOUT		30000 /* us */
 
 /* Transmitter timeout */
 #define TX_TIMEOUT (2 * HZ)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] net: fec: Adjust ENET MDIO timeouts
  2011-12-27 18:53 ` [PATCH v2] " Fabio Estevam
@ 2011-12-27 19:07   ` David Miller
  0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2011-12-27 19:07 UTC (permalink / raw)
  To: festevam; +Cc: netdev, shawn.guo, kernel, rogerio.pimentel, fabio.estevam

From: Fabio Estevam <festevam@gmail.com>
Date: Tue, 27 Dec 2011 16:53:41 -0200

> On extensive NFS boots on a mx6qsabrelite board it was noted that "FEC: MDIO read timeout" were occuring,
> which caused failure on loading the FEC driver.
> 
> The original FEC_MII_TIMEOUT was set to 1 ms, which is too low when passed to the usecs_to_jiffies macro.
> 
> On ARM one jiffy is 10ms, so use a timeout of 30ms, which corresponds to 3 jiffies.
> 
> After running extensive NFS boots, the MDIO timeouts do not occur anymore with this change.
> 
> Signed-off-by: Rogerio Pimentel <rogerio.pimentel@freescale.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-12-27 19:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-27 14:46 [PATCH] net: fec: Adjust ENET MDIO timeouts Fabio Estevam
2011-12-27 18:13 ` David Miller
2011-12-27 18:53 ` [PATCH v2] " Fabio Estevam
2011-12-27 19:07   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).