Date: Tue, 8 Apr 2003 20:08:49 +0200 From: Maxime Henrion <mux@freebsd.org> To: Nate Lawson <nate@root.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c Message-ID: <20030408180849.GG1750@elvis.mu.org> In-Reply-To: <20030408173058.GF1750@elvis.mu.org> References: <20030408130518.AD52137B496@hub.freebsd.org> <Pine.BSF.4.21.0304081010230.27414-100000@root.org> <20030408173058.GF1750@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030408180849.GG1750>