Patchwork tms380tr: fix long delays in initialization

login
register
mail settings
Submitter Meelis Roos
Date Oct. 3, 2010, 7:34 p.m.
Message ID <alpine.SOC.1.00.1010032228450.28867@math.ut.ee>
Download mbox | patch
Permalink /patch/66602/
State Rejected
Delegated to: David Miller
Headers show

Comments

Meelis Roos - Oct. 3, 2010, 7:34 p.m.
tms380tr driver tries to use udelay (meaning busy loop) for several half 
second delays during hardware initialization. Crazy overly long busy 
wait delays mean no delay at all so driver initialization fails without 
waiting. Fix it by using msleep() for long delays and leave it to 
udelay() for short delays.

Signed-off-by: Meelis Roos <mroos@linux.ee>

---
 drivers/net/tokenring/tms380tr.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)
David Miller - Oct. 4, 2010, 3:01 a.m.
From: Meelis Roos <mroos@linux.ee>
Date: Sun, 3 Oct 2010 22:34:30 +0300 (EEST)

> tms380tr driver tries to use udelay (meaning busy loop) for several half 
> second delays during hardware initialization. Crazy overly long busy 
> wait delays mean no delay at all so driver initialization fails without 
> waiting. Fix it by using msleep() for long delays and leave it to 
> udelay() for short delays.
> 
> Signed-off-by: Meelis Roos <mroos@linux.ee>

You can't use msleep() here because this code can be invoked
from interrupts and thus cannot sleep.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Meelis Roos - Oct. 4, 2010, 8:39 a.m.
> > tms380tr driver tries to use udelay (meaning busy loop) for several half 
> > second delays during hardware initialization. Crazy overly long busy 
> > wait delays mean no delay at all so driver initialization fails without 
> > waiting. Fix it by using msleep() for long delays and leave it to 
> > udelay() for short delays.
> > 
> > Signed-off-by: Meelis Roos <mroos@linux.ee>
> 
> You can't use msleep() here because this code can be invoked
> from interrupts and thus cannot sleep.

I checked these two functions that contain long delays that I changed - 
tms380tr_bringup_diags() and tms380tr_init_adapter() - to be called only 
from tms380tr_chipset_init() that is only called from tms380tr_open() so 
no call paths from interrupts AFAICS. Short delyas from interrupt 
context are not changed in any way, they still use udelay().
David Miller - Oct. 4, 2010, 4:42 p.m.
From: Meelis Roos <mroos@linux.ee>
Date: Mon, 4 Oct 2010 11:39:02 +0300 (EEST)

>> > tms380tr driver tries to use udelay (meaning busy loop) for several half 
>> > second delays during hardware initialization. Crazy overly long busy 
>> > wait delays mean no delay at all so driver initialization fails without 
>> > waiting. Fix it by using msleep() for long delays and leave it to 
>> > udelay() for short delays.
>> > 
>> > Signed-off-by: Meelis Roos <mroos@linux.ee>
>> 
>> You can't use msleep() here because this code can be invoked
>> from interrupts and thus cannot sleep.
> 
> I checked these two functions that contain long delays that I changed - 
> tms380tr_bringup_diags() and tms380tr_init_adapter() - to be called only 
> from tms380tr_chipset_init() that is only called from tms380tr_open() so 
> no call paths from interrupts AFAICS. Short delyas from interrupt 
> context are not changed in any way, they still use udelay().

tms380tr_init_adapter() gets called from tms380tr_chk_irq() which is
invoked from the interrupt handler.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Meelis Roos - Oct. 4, 2010, 9:15 p.m.
> tms380tr_init_adapter() gets called from tms380tr_chk_irq() which is
> invoked from the interrupt handler.

Oops, yes, sorry for repeatedly not noticing it even when I was 
explicitly searching for it :(

What is the general course from here? Defer the long reset to a work 
queue, or some other kind of background task?
David Miller - Oct. 4, 2010, 9:33 p.m.
From: Meelis Roos <mroos@linux.ee>
Date: Tue, 5 Oct 2010 00:15:17 +0300 (EEST)

>> tms380tr_init_adapter() gets called from tms380tr_chk_irq() which is
>> invoked from the interrupt handler.
> 
> Oops, yes, sorry for repeatedly not noticing it even when I was 
> explicitly searching for it :(
> 
> What is the general course from here? Defer the long reset to a work 
> queue, or some other kind of background task?

Yes, very likely a workqueue is the best course of action.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/tokenring/tms380tr.c b/drivers/net/tokenring/tms380tr.c
index aaec637..e6dc2df 100644
--- a/drivers/net/tokenring/tms380tr.c
+++ b/drivers/net/tokenring/tms380tr.c
@@ -177,6 +177,7 @@  static void 	tms380tr_update_rcv_stats(struct net_local *tp,
 			unsigned char DataPtr[], unsigned int Length);
 /* "W" */
 void	 	tms380tr_wait(unsigned long time);
+static void	tms380tr_wait_long(unsigned long time);
 static void 	tms380tr_write_rpl_status(RPL *rpl, unsigned int Status);
 static void 	tms380tr_write_tpl_status(TPL *tpl, unsigned int Status);
 
@@ -1216,20 +1217,19 @@  static void tms380tr_set_multicast_list(struct net_device *dev)
 }
 
 /*
- * Wait for some time (microseconds)
+ * Wait for some time (microseconds) - busy wait
  */
 void tms380tr_wait(unsigned long time)
 {
-#if 0
-	long tmp;
-	
-	tmp = jiffies + time/(1000000/HZ);
-	do {
-		tmp = schedule_timeout_interruptible(tmp);
-	} while(time_after(tmp, jiffies));
-#else
 	udelay(time);
-#endif
+}
+
+/*
+ * Wait for long time (microseconds) - schedule
+ */
+static void tms380tr_wait_long(unsigned long time)
+{
+	msleep(time / 1000);
 }
 
 /*
@@ -1352,9 +1352,9 @@  static int tms380tr_bringup_diags(struct net_device *dev)
 	int loop_cnt, retry_cnt;
 	unsigned short Status;
 
-	tms380tr_wait(HALF_SECOND);
+	tms380tr_wait_long(HALF_SECOND);
 	tms380tr_exec_sifcmd(dev, EXEC_SOFT_RESET);
-	tms380tr_wait(HALF_SECOND);
+	tms380tr_wait_long(HALF_SECOND);
 
 	retry_cnt = BUD_MAX_RETRIES;	/* maximal number of retrys */
 
@@ -1365,7 +1365,7 @@  static int tms380tr_bringup_diags(struct net_device *dev)
 		loop_cnt = BUD_MAX_LOOPCNT;	/* maximum: three seconds*/
 		do {			/* Inspect BUD results */
 			loop_cnt--;
-			tms380tr_wait(HALF_SECOND);
+			tms380tr_wait_long(HALF_SECOND);
 			Status = SIFREADW(SIFSTS);
 			Status &= STS_MASK;
 
@@ -1384,7 +1384,7 @@  static int tms380tr_bringup_diags(struct net_device *dev)
 			printk(KERN_INFO "%s: Adapter Software Reset.\n", 
 				dev->name);
 			tms380tr_exec_sifcmd(dev, EXEC_SOFT_RESET);
-			tms380tr_wait(HALF_SECOND);
+			tms380tr_wait_long(HALF_SECOND);
 		}
 	} while(retry_cnt > 0);
 
@@ -1457,7 +1457,7 @@  static int tms380tr_init_adapter(struct net_device *dev)
 		do {
 			Status = 0;
 			loop_cnt--;
-			tms380tr_wait(HALF_SECOND);
+			tms380tr_wait_long(HALF_SECOND);
 
 			/* Mask interesting status bits */
 			Status = SIFREADW(SIFSTS);
@@ -1506,7 +1506,7 @@  static int tms380tr_init_adapter(struct net_device *dev)
 				{
 					/* Reset adapter and try init again */
 					tms380tr_exec_sifcmd(dev, EXEC_SOFT_RESET);
-					tms380tr_wait(HALF_SECOND);
+					tms380tr_wait_long(HALF_SECOND);
 				}
 			}
 		}