Usage of mdelay() inside Interrupt

Usage of mdelay() inside Interrupt

[Armin Wolf]

Hi,

while browsing the sourcefile of lib8390.c in
drivers/net/ethernet/8390/, i noticed that inside
of ei_rx_overrun(), which is called from inside
a Interrupt handler, mdelay() is being used.
So i wonder if the usage of mdelay() inside the
Interrupt handler may cause problems since waiting
~10ms in Interrupt context seems a bit odd.

Re: Usage of mdelay() inside Interrupt

[Andrew Lunn]

On Thu, May 28, 2020 at 11:15:18PM +0200, Armin Wolf wrote:
> Hi,
> 
> while browsing the sourcefile of lib8390.c in
> drivers/net/ethernet/8390/, i noticed that inside
> of ei_rx_overrun(), which is called from inside
> a Interrupt handler, mdelay() is being used.
> So i wonder if the usage of mdelay() inside the
> Interrupt handler may cause problems since waiting
> ~10ms in Interrupt context seems a bit odd.

Hi Armin

It is legal. But it is not ideal. But reading the comments around it
suggests the hardware is very particular about how you recover from an
overrun, and maybe this is the most robust solution?

Overruns should not happen too often. If the statistics counter
stats.rx_over_errors indicates it is happening a lot then maybe more
investigation is needed, because it is going to be bad for
performance.

	Andrew

Re: Usage of mdelay() inside Interrupt

[Maciej W. Rozycki]

On Fri, 29 May 2020, Andrew Lunn wrote:

> > while browsing the sourcefile of lib8390.c in
> > drivers/net/ethernet/8390/, i noticed that inside
> > of ei_rx_overrun(), which is called from inside
> > a Interrupt handler, mdelay() is being used.
> > So i wonder if the usage of mdelay() inside the
> > Interrupt handler may cause problems since waiting
> > ~10ms in Interrupt context seems a bit odd.
[...]
> It is legal. But it is not ideal. But reading the comments around it
> suggests the hardware is very particular about how you recover from an
> overrun, and maybe this is the most robust solution?

 This code is very old and dates back to 1996.  At that time it was 
busy-looping inline even:

    /*
     * Wait a full Tx time (1.2ms) + some guard time, NS says 1.6ms total.
     * Early datasheets said to poll the reset bit, but now they say that
     * it "is not a reliable indicator and subsequently should be ignored."
     * We wait at least 10ms.
     */
    wait_start_time = jiffies;
    while (jiffies - wait_start_time <= 1*HZ/100)
	barrier();

Previously it had:

    /* Wait for the reset to complete. This should happen almost instantly,
	   but could take up to 1.5msec in certain rare instances.  There is no
	   easy way of timing something in that range, so we use 'jiffies' as
	   a sanity check. */
    while ((inb_p(e8390_base+EN0_ISR) & ENISR_RESET) == 0)
		if (jiffies - reset_start_time > 1) {
			printk("%s: reset did not complete at ei_rx_overrun.\n",
				   dev->name);
			NS8390_init(dev, 1);
			return;
		}

going back to 1.1.x days.  I don't have older history to hand.  Nowadays I 
think it would make sense to rewrite it to use a timer or suchlike so that 
the post-reset part runs asynchronously, but such hardware is not exactly 
common anymore I'd imagine (I should have an ISA NE2000 clone somewhere 
IIRC, which I haven't used in like 20 years) and clearly nobody bothered.

  Maciej