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 <firstname.lastname@example.org>
> To: "Kevin D. Kissell" <email@example.com>
> Cc: Sergei Shtylyov <firstname.lastname@example.org>,
> Atsushi Nemoto <email@example.com>, firstname.lastname@example.org,
> Subject: Re: [MIPS] FPU emulator: allow Cause bits of FCSR to be writeable
> Content-Type: text/plain; charset=ISO-8859-1
> On Thu, May 6, 2010 at 9:46 AM, Kevin D. Kissell <email@example.com> 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.
> 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