Re: [PATCH] X86: coding style fixes to arch/x86/math-errors.c

[Paolo Ciarrocchi]

On Jan 27, 2008 10:45 PM, Dmitri Vorobiev <dmitri.vorobiev@xxxxxxxxx> wrote:
> > -  math_abort(FPU_info,SIGILL);
> > +  math_abort(FPU_info , SIGILL);
>
> Looks like you're trading bad for worse here.

Should I make it read like math_abort(FPU_info, SIGILL); ?

>
> >  }
> >
> >
> > @@ -88,61 +86,69 @@ void FPU_printall(void)
> >  {
> >    int i;
> >    static const char *tag_desc[] = { "Valid", "Zero", "ERROR", "Empty",
> > -                              "DeNorm", "Inf", "NaN" };
> > +                                     "DeNorm", "Inf", "NaN" };
> >    u_char byte1, FPU_modrm;
> >    unsigned long address = FPU_ORIG_EIP;
> >
> >    RE_ENTRANT_CHECK_OFF;
> >    /* No need to check access_ok(), we have previously fetched these bytes. */
> >    printk("At %p:", (void *) address);
> > -  if ( FPU_CS == __USER_CS )
> > -    {
> > +  if (FPU_CS == __USER_CS) {
> >  #define MAX_PRINTED_BYTES 20
> > -      for ( i = 0; i < MAX_PRINTED_BYTES; i++ )
> > -     {
> > +      for (i = 0; i < MAX_PRINTED_BYTES; i++) {
> >         FPU_get_user(byte1, (u_char __user *) address);
> > -       if ( (byte1 & 0xf8) == 0xd8 )
> > -         {
> > +       if ((byte1 & 0xf8) == 0xd8) {
> >             printk(" %02x", byte1);
> >             break;
> >           }
> >         printk(" [%02x]", byte1);
> >         address++;
> >       }
> > -      if ( i == MAX_PRINTED_BYTES )
> > +      if (i == MAX_PRINTED_BYTES)
> >       printk(" [more..]\n");
> > -      else
> > -     {
> > +      else {
> >         FPU_get_user(FPU_modrm, 1 + (u_char __user *) address);
> > -
> > +
> >         if (FPU_modrm >= 0300)
> > -         printk(" %02x (%02x+%d)\n", FPU_modrm, FPU_modrm & 0xf8, FPU_modrm & 7);
> > +         printk(" %02x (%02x+%d)\n", FPU_modrm, FPU_modrm & 0xf8,
> > +                     FPU_modrm & 7);
> >         else
> >           printk(" /%d, mod=%d rm=%d\n",
> >                  (FPU_modrm >> 3) & 7, (FPU_modrm >> 6) & 3, FPU_modrm & 7);
> >       }
> >      }
> >    else
> > -    {
> >        printk("%04x\n", FPU_CS);
> > -    }
> >
> >    partial_status = status_word();
> >
> >  #ifdef DEBUGGING
> > -if ( partial_status & SW_Backward )    printk("SW: backward compatibility\n");
> > -if ( partial_status & SW_C3 )          printk("SW: condition bit 3\n");
> > -if ( partial_status & SW_C2 )          printk("SW: condition bit 2\n");
> > -if ( partial_status & SW_C1 )          printk("SW: condition bit 1\n");
> > -if ( partial_status & SW_C0 )          printk("SW: condition bit 0\n");
> > -if ( partial_status & SW_Summary )     printk("SW: exception summary\n");
> > -if ( partial_status & SW_Stack_Fault ) printk("SW: stack fault\n");
> > -if ( partial_status & SW_Precision )   printk("SW: loss of precision\n");
> > -if ( partial_status & SW_Underflow )   printk("SW: underflow\n");
> > -if ( partial_status & SW_Overflow )    printk("SW: overflow\n");
> > -if ( partial_status & SW_Zero_Div )    printk("SW: divide by zero\n");
> > -if ( partial_status & SW_Denorm_Op )   printk("SW: denormalized operand\n");
> > -if ( partial_status & SW_Invalid )     printk("SW: invalid operation\n");
> > +if (partial_status & SW_Backward)
> > +  printk("SW: backward compatibility\n");
> > +if (partial_status & SW_C3)
> > +  printk("SW: condition bit 3\n");
> > +if (partial_status & SW_C2)
> > +  printk("SW: condition bit 2\n");
> > +if (partial_status & SW_C1)
> > +  printk("SW: condition bit 1\n");
> > +if (partial_status & SW_C0)
> > +  printk("SW: condition bit 0\n");
> > +if (partial_status & SW_Summary)
> > +  printk("SW: exception summary\n");
> > +if (partial_status & SW_Stack_Fault)
> > +  printk("SW: stack fault\n");
> > +if (partial_status & SW_Precision)
> > +  printk("SW: loss of precision\n");
> > +if (partial_status & SW_Underflow)
> > +  printk("SW: underflow\n");
> > +if (partial_status & SW_Overflow)
> > +  printk("SW: overflow\n");
> > +if (partial_status & SW_Zero_Div)
> > +  printk("SW: divide by zero\n");
> > +if (partial_status & SW_Denorm_Op)
> > +  printk("SW: denormalized operand\n");
> > +if (partial_status & SW_Invalid)
> > +  printk("SW: invalid operation\n");
>
> While you're at it, please think about adding proper KERN_ facility to these printk() calls.

Yes, basically all the warnings are about missing KERN_.
Do you want me to add KERN_ERR  to all the above printk?

> >  #endif /* DEBUGGING */
> >
> >    printk(" SW: b=%d st=%ld es=%d sf=%d cc=%d%d%d%d ef=%d%d%d%d%d%d\n",
>
> Ditto.
>
> > @@ -155,7 +161,7 @@ if ( partial_status & SW_Invalid )     printk("SW: invalid operation\n");
> >        partial_status & SW_Precision?1:0, partial_status & SW_Underflow?1:0,
> >        partial_status & SW_Overflow?1:0, partial_status & SW_Zero_Div?1:0,
> >        partial_status & SW_Denorm_Op?1:0, partial_status & SW_Invalid?1:0);
> > -
> > +
> >  printk(" CW: ic=%d rc=%ld%ld pc=%ld%ld iem=%d     ef=%d%d%d%d%d%d\n",
>
> Ditto.
>
[...]

> >  #ifdef __DEBUG__
> > -  math_abort(FPU_info,SIGFPE);
> > +  math_abort(FPU_info , SIGFPE);
>
> Why do you need this extra space before the comma?

My mistake.

> This file seems to be in need of formatting lines to use tabs instead of spaces.
>

Ciao,
-- 
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/