| Message ID | alpine.SOC.1.00.1010032228450.28867@math.ut.ee |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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
> > 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().
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
> 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?
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
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); } } }
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(-)