[PATCH 2/2] ne.c msleep not mdelay

[PATCH 2/2] ne.c msleep not mdelay

[David Fries]

mdelay(10) replaced by msleep(10) to give up the CPU, it's just
waiting for an interrupt, so timing isn't critical.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
---
 drivers/net/ne.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index b0bf47e..bcbeff7 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -529,7 +529,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
 		outb_p(0x00, ioaddr + EN0_RCNTLO);
 		outb_p(0x00, ioaddr + EN0_RCNTHI);
 		outb_p(E8390_RREAD+E8390_START, ioaddr); /* Trigger it... */
-		mdelay(10);	/* wait 10ms for interrupt to propagate */
+		msleep(10);	/* wait 10ms for interrupt to propagate */
 		outb_p(0x00, ioaddr + EN0_IMR); 		/* Mask it again. */
 		dev->irq = probe_irq_off(cookie);
 		if (ei_debug > 2)
-- 
1.4.4.4

Re: [PATCH 2/2] ne.c msleep not mdelay

[Alan Cox]

On Fri, 29 Aug 2008 21:44:53 -0500
David Fries <david@fries.net> wrote:

> mdelay(10) replaced by msleep(10) to give up the CPU, it's just
> waiting for an interrupt, so timing isn't critical.

It is too critical for a reschedule to occur.

NAK this one.

Re: [PATCH 2/2] ne.c msleep not mdelay

[Atsushi Nemoto]

On Sat, 30 Aug 2008 09:59:06 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > mdelay(10) replaced by msleep(10) to give up the CPU, it's just
> > waiting for an interrupt, so timing isn't critical.
> 
> It is too critical for a reschedule to occur.
> 
> NAK this one.

There are already some msleep() in probe_irq_on() which is called just
before here.  And this part is not protected by any spinlock or
preempt_disable.

So, if rescheduling was dangerous here, we already have potential
problems, no?

---
Atsushi Nemoto

Re: [PATCH 2/2] ne.c msleep not mdelay

[Alan Cox]

> There are already some msleep() in probe_irq_on() which is called just
> before here.  And this part is not protected by any spinlock or
> preempt_disable.
> 
> So, if rescheduling was dangerous here, we already have potential
> problems, no?

Yes we do but I didn't manage to stop the other mistakes getting in when
they did. If you take a schedule you get results back from the probe_irq
that tend to suck in other random ISA device events (ISA being edge
triggered nobody was ever careful about spurious irq)

Re: [PATCH 2/2] ne.c msleep not mdelay

[David Fries]

On Sat, Aug 30, 2008 at 11:36:47PM +0900, Atsushi Nemoto wrote:
> On Sat, 30 Aug 2008 09:59:06 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > > mdelay(10) replaced by msleep(10) to give up the CPU, it's just
> > > waiting for an interrupt, so timing isn't critical.
> > 
> > It is too critical for a reschedule to occur.
> > 
> > NAK this one.
> 
> There are already some msleep() in probe_irq_on() which is called just
> before here.  And this part is not protected by any spinlock or
> preempt_disable.

There is a probing_active mutex.  probe_irq_off has the comment
"nothing prevents two IRQ probe callers from overlapping."  Looks to me
like probing_active would prevent multiple probes from happening.

> So, if rescheduling was dangerous here, we already have potential
> problems, no?

I was just going to make the argument that any task that could be run
during msleep, could just as easily run on the other cpu in mdelay (if
there was one).

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

Re: [PATCH 2/2] ne.c msleep not mdelay

[Alan Cox]

> I was just going to make the argument that any task that could be run
> during msleep, could just as easily run on the other cpu in mdelay (if
> there was one).

ISA bus auto-probing doesn't actually work at all well on SMP. The number
of people who have ever been affected by that has always been effectively
nil.

Re: [PATCH 2/2] ne.c msleep not mdelay

[Atsushi Nemoto]

On Sat, 30 Aug 2008 15:44:47 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > So, if rescheduling was dangerous here, we already have potential
> > problems, no?
> 
> Yes we do but I didn't manage to stop the other mistakes getting in when
> they did. If you take a schedule you get results back from the probe_irq
> that tend to suck in other random ISA device events (ISA being edge
> triggered nobody was ever careful about spurious irq)

OK, I see your point.  Thanks.

---
Atsushi Nemoto