From owner-cvs-all@FreeBSD.ORG Fri Apr 11 05:56:24 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 8139437B401; Fri, 11 Apr 2003 05:56:24 -0700 (PDT) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.FreeBSD.org (Postfix) with ESMTP id 0E36443F75; Fri, 11 Apr 2003 05:56:24 -0700 (PDT) (envelope-from mux@freebsd.org) Received: by elvis.mu.org (Postfix, from userid 1920) id E63EE2ED40E; Fri, 11 Apr 2003 05:56:23 -0700 (PDT) Date: Fri, 11 Apr 2003 14:56:23 +0200 From: Maxime Henrion To: Nate Lawson Message-ID: <20030411125623.GO1750@elvis.mu.org> References: <20030411001316.GN1750@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.1i cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Apr 2003 12:56:24 -0000 Nate Lawson wrote: > 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. Ok. This is fixed now, as is fixed the hangs I was seeing, which I now believe weren't caused by a local change. Thanks, Maxime