On Thu, May 06, 2010 at 12:42:35PM -0600, Shane McDonald wrote: > Date: Thu, 6 May 2010 12:42:35 -0600 > From: Shane McDonald <mcdonald.shane@xxxxxxxxx> > To: "Kevin D. Kissell" <kevink@xxxxxxxxxxxxx> > Cc: Sergei Shtylyov <sshtylyov@xxxxxxxxxx>, > Atsushi Nemoto <anemo@xxxxxxxxxxxxx>, ralf@xxxxxxxxxxxxxx, > linux-mips@xxxxxxxxxxxxxx > Subject: Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable > by > ctc1 > Content-Type: text/plain; charset=ISO-8859-1 > > On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <kevink@xxxxxxxxxxxxx> wrote: > > Sergei Shtylyov wrote: > >> Kevin D. Kissell wrote: > >> > >>> I'm cool with the patch as is, but in the general spirit of regarding > >>> numeric constants other than 0 and 1 as instruments of Satan, it > >>> would probably be even better if those reserved bits were defined > >>> (FPU_CSR_RSVD, or whatever is compatible with existing convention for > >>> such bits) along with the other FCSR bit masks in mipsregs.h, so that > >>> the assigment looks like: > >>> > >>> ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) | > >>> ieee_rm[value & 0x3]; > >> > >> 0x3 is still neither 0 nor 1, and so remains an instrument of Satan. > >> How about #defining it also? :-) > > > > In some software engineering cultures, that would be considered a good > > idea or even mandatory. This being the Linux kernel, I think it's OK, > > because, as Shane remarked, it's a mask that's local to the module and > > whose value is "obvious", and such things are pretty commonly handled as > > numeric constants in the kernel. > > > > There actually *is* a #define for that field, FPU_CSR_RD, which could be > > used here in place of the 0x3, but I'm a little torn about its use. On > > one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what > > we're doing, but on the other hand, it's less obvious that "value & > > FPU_CSR_RD" is generating an integer in the range 0-3 for an index. So > > I'm absolutely fine with the code as written, but wouldn't complain if > > someone wanted to make it use the symbolic constant. > > I'm torn here, which is why I hadn't changed that at all. I'd rather not > use FPU_CSR_RD, because that defines a value in the rounding mode > bits (which happens to have all the bits set), rather than a mask for the > bits. I'd prefer to define a new constant FPU_CSR_RM with the > value 0x00000003 -- a better name might be FPU_CSR_RM_MASK, > but that's inconsistent with the names of the other FCSR fields. > And, as Kevin said, it doesn't make it clear that we're trying to generate > an index in the range of 0 - 3. I guess we could also define a constant > for the number of bits to shift to get the RM bits into the low order bits, > something like > > #define FPU_CSR_RM_SHIFT 0 > > at which point our code could look like > > ctx->fcr31 = (value & ~(FPU_CSR_RSVD | FPU_CSR_RM)) | > ieee_rm[(value & FPU_CSR_RM) >> FPU_CSR_RM_SHIFT]; > > I'm a little wary of adding the FPU_CSR_RM_SHIFT, because there aren't > any other SHIFT defines for the FCSR, and because the shift value is 0. > But, the above code doesn't look as bad as I originally thought it would, > and it probably is clearer. > > Comments? > > There's also use of the 0x3 magic number in a number of other cases > in this file. Do I make a similar change to those cases in this patch, > or should I create a separate patch for that? Or would that just be > one of those minor style change patches that no one likes? I'd certainly consider it. It's logically a separate change so I'd suggest a separate patch for -queue. Your earlier patch is 2.6.34 and -stable material however and that's another reason why this should be separate patches. Ralf
