From owner-cvs-src@FreeBSD.ORG Tue Apr 8 11:08:49 2003 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E18C137B401; Tue, 8 Apr 2003 11:08:49 -0700 (PDT) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7231E43FAF; Tue, 8 Apr 2003 11:08:49 -0700 (PDT) (envelope-from mux@freebsd.org) Received: by elvis.mu.org (Postfix, from userid 1920) id 5A1402ED407; Tue, 8 Apr 2003 11:08:49 -0700 (PDT) Date: Tue, 8 Apr 2003 20:08:49 +0200 From: Maxime Henrion To: Nate Lawson Message-ID: <20030408180849.GG1750@elvis.mu.org> References: <20030408130518.AD52137B496@hub.freebsd.org> <20030408173058.GF1750@elvis.mu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20030408173058.GF1750@elvis.mu.org> 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-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Apr 2003 18:08:50 -0000 Maxime Henrion wrote: > Nate Lawson wrote: > > On Tue, 8 Apr 2003, Maxime Henrion wrote: > > > Modified files: > > > sys/dev/fxp if_fxp.c > > > Log: > > > Fix the hangs people have been seeing. For some reason, the > > > atomic operation change caused this, so backout it until I > > > can figure out what's wrong with it. > > > > > > Revision Changes Path > > > 1.163 +7 -11 src/sys/dev/fxp/if_fxp.c > > > > Thank you. > > > > > - atomic_clear_32((u_int32_t *)&last->tx_cb->cb_status, > > > - htobe32(bswap16(FXP_CB_COMMAND_S))); > > > - > > > + atomic_clear_short(&sc->fxp_desc.tx_last->tx_cb->cb_command, > > > + FXP_CB_COMMAND_S); > > > +#else > > > + sc->fxp_desc.tx_last->tx_cb->cb_command &= ~FXP_CB_COMMAND_S; > > > +#endif /*__alpha__*/ > > > > Um, are each of those cases semantically equivalent? > > Yes they are. The only difference, and that's probably where the > problem lies, is that we are now rewriting (but not changing) cb_status. > The cb_status word is probably mapped to a register in the fxp(4) card > and thus rewriting it might not be safe. That's indeed the problem. We shouldn't touch cb_status at all or it will acknowledge interrupts. The __alpha__ "fix" was actually needed because since alpha can't access memory in 16-bit granularity, the compiler used a 32-bit operation and thus was touching cb_status which cause the hang if done at a bad time. So my atomic change just extended the problem to all architectures instead of just __alpha__. In short, we actually don't need atomicity at all here and this use of atomic_clear_short() was very confusing. It's actually still confusing since I can't figure out how this could have worked anyway, and the commit log says that this fix was tested. However, atomic_clear_short() will also end up using 32-bit instructions. Cheers, Maxime