cvs commit: src/sys/dev/re if_re.c

[John Baldwin]

On Thursday 18 August 2005 02:43 pm, Maxim Sobolev wrote:
> John Baldwin wrote:
> > On Thursday 18 August 2005 10:29 am, Maxim Sobolev wrote:
> >>sobomax     2005-08-18 14:29:01 UTC
> >>
> >>  FreeBSD src repository
> >>
> >>  Modified files:
> >>    sys/dev/re           if_re.c
> >>  Log:
> >>  In re_shutdown() mark interface as down since otherwise we will panic
> >> if interrupt comes in later on, which can happen in some uncommon cases.
> >>
> >>  Another possible fix is to call re_detach() instead of re_stop(), like
> >>  ve(4) does, but I am not sure if the latter is really RTTD, so that
> >> stick with this one-liner for now.
> >>
> >>  PR:             kern/80005
> >>  Approved by:    silence on -arch, no reply from selected network gurus
> >
> > The PR reports problems while the machine is running, not a panic during
> > shutdown.  I couldn't get the PR database to load the PR with the panic
> > from
>
> Sorry, it has been a typo - in fact I was reffering to kern/85005.
>
> > vr(4) yesterday.  It's very unclear why clearing IFF_UP makes any
> > difference. Ah, perhaps re_intr() should be fixed to check
> > IFF_DRV_RUNNING rather than IFF_UP?  Then the re_stop() in re_shutdown()
> > would be sufficient.
>
> Yes, this will help. However, I am not quite sure, what is the point to
> do interrupt processing if interface is down? Perhaps re_intr() should
> check both IFF_DRV_RUNNING and IFF_UP?

IFF_DRV_RUNNING will be clear once foo_stop() is called.  I think IFF_UP is 
the wrong flag actually as it might still be set in the shutdown case, but 
shutdown normally calls foo_stop() so you should be able to just check 
IFF_DRV_RUNNING.  Note that when you down an interface, you get an ioctl that 
results in foo_stop() being called from foo_ioctl() and the upper layer then 
clears IFF_UP.

> > I also think that the change to vr(4) should be reverted (way too big of
> > a sledge hammer) and instead its interrupt handler can check
> > IFF_DRV_RUNNING and bail if it is clear as well.
>
> Well, see kern/85005. IMO some generic approach should be worked out and
> implemented since I think many other network drivers may be affected by
> the same problem.

I've still yet to see what the real panic is.  For one thing, if the foo_stop 
method does its jobs, the ethernet hardware shouldn't be generating 
interrupts.  The stop method should be shutting the card down (i.e. turning 
off the receiver and transmitter for example).  Is your ethernet driver 
sharing an interrupt with another device and the other device is 
interrupting?  In that case, the ethernet driver would have the same panic if 
you did an 'ifconfig foo0 down' and then the other device interrupted.  So, I 
think clearing IFF_UPP in foo_shutdown() is wrong.  foo_stop() should really 
be sufficient, and foo_intr() should be able to handle a spurious interrupt 
while the interface is stopped without panicing since it already needs to do 
so to handle the shared interrupt case.

-- 
John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org