Date: Thu, 10 Apr 2003 17:26:49 -0700 (PDT) From: Nate Lawson <nate@root.org> To: Maxime Henrion <mux@freebsd.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c Message-ID: <Pine.BSF.4.21.0304101719210.32690-100000@root.org> In-Reply-To: <20030411001316.GN1750@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 11 Apr 2003, Maxime Henrion wrote: > Nate Lawson wrote: > > On Thu, 10 Apr 2003, Maxime Henrion wrote: > > > Modified files: > > > sys/dev/fxp if_fxp.c > > > Log: > > > - Clean up the fxp_release() and fxp_detach() functions. > > > > There's a version of this in the diff I just posted to current@. > > Feel free to commit it. I'd like to keep it bundled with the MPSAFE changes so it will have to wait a little while for more testing. > > I have been testing my diff by loading and unloading fxp while doing a > > large transfer and I cannot replicate this. Are you sure it's not a local > > problem? I have never had a deadlock or a crash and loading fxp again > > always works. > > I have no idea, this is why I was saying it's maybe not fxp(4)'s fault > and it may indeed be a local problem. It would help if others gave their insight but if loading/unloading work for me (and have been for months), it suggests it is local. I don't have a problem with making the unmap conditional on the tag (first part of commit). > > Um, fxp_detach() should not be called for any case where the device isn't > > alive. fxp_detach should ONLY be called once attach has succeeded which > > by definition means the device is alive. bus_child_present() is the > > bus-specific method to see that the hardware is actually there; > > device_is_alive only tells you that the device_t node is present in the > > tree. fxp_release may be called in error cases. Rather than working > > around your problem this way, please find what is calling fxp_detach when > > the device is not alive. > > This way of doing things (ie: using device_is_alive()) is an almost > verbatim copy of what you did in every network driver in sys/pci/. > Now if it's wrong, feel free to change it, but I guess you'll have > to change all those drivers too then. The reason why I did that for the other drivers is that there was not a separate "release" function which deallocated resources. Instead, I had a common routine which did detach and release. For the error case, you do need to check if the device was alive before calling *stop() on potentially non-working hardware. But for fxp, this is done explicitly -- routines which can only be done on a "live" device go in fxp_detach() and those that need to be done in both the error and detach cases go in fxp_release(). Thus the second part of your commit is unnecessary. -Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0304101719210.32690-100000>