Date: Thu, 18 Aug 2005 15:38:31 -0400 From: John Baldwin <jhb@FreeBSD.org> To: Maxim.Sobolev@portaone.com Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/re if_re.c Message-ID: <200508181538.32988.jhb@FreeBSD.org> In-Reply-To: <4304D6B9.8050603@portaone.com> References: <200508181429.j7IET16d038533@repoman.freebsd.org> <200508181238.05411.jhb@FreeBSD.org> <4304D6B9.8050603@portaone.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200508181538.32988.jhb>