Re: amd_mce.c redundant if check?

[Chip]

On Wed, Aug 20, 2014 at 11:18:21AM -0600, Adam Duskett wrote:

> I have recently come upon this section of code in
> arch/x86/kernel/cpu/mcheck/mce_amd.c that seems to be a redundant
> unnecessary if check.
>
>
> From line 170 - 176:
>
> if (tr->set_lvt_off) {
> if (lvt_off_valid(tr->b, tr->lvt_off, lo, hi)) {
> /* set new lvt offset */
> hi &= ~MASK_LVTOFF_HI;
> hi |= tr->lvt_off << 20;
> }
> }
>
>
> This seems like it's not actually doing anything because it's setting
> the same value that the bit-field already has to itself.

I brought this up to Adam the other day, so he posted the question to this 
list today to elicit a response from the original developer(s).  I realize 
the quickest response is to ask the original poster (Adam) to investigate 
further, such as with pen and paper, but that is not a proper response to a 
legitimate question.  Here is the #define that is referenced, and the two 
routines in question.  This is current in kernel version 3.16 in 
arch/x86/kernel/cpu/mcheck/mce_amd.c.

#define MASK_LVTOFF_HI    0x00F00000

static int lvt_off_valid(struct threshold_block *b, int apic, u32 lo, u32 
hi)
{
       int msr = (hi & MASK_LVTOFF_HI) >> 20;

       if (apic < 0) {
               pr_err(FW_BUG "cpu %d, failed to setup threshold interrupt "
                      "for bank %d, block %d (MSR%08X=0x%x%08x)\n", b->cpu,
                      b->bank, b->block, b->address, hi, lo);
               return 0;
       }

       if (apic != msr) {
               pr_err(FW_BUG "cpu %d, invalid threshold interrupt offset %d 
"
                      "for bank %d, block %d (MSR%08X=0x%x%08x)\n",
                      b->cpu, apic, b->bank, b->block, b->address, hi, lo);
               return 0;
       }

       return 1;
};

/*
* Called via smp_call_function_single(), must be called with correct
* cpu affinity.
*/
static void threshold_restart_bank(void *_tr)
{
       struct thresh_restart *tr = _tr;
       u32 hi, lo;

       rdmsr(tr->b->address, lo, hi);

       if (tr->b->threshold_limit < (hi & THRESHOLD_MAX))
               tr->reset = 1;  /* limit cannot be lower than err count */

       if (tr->reset) {                /* reset err count and overflow bit 
*/
               hi =
                   (hi & ~(MASK_ERR_COUNT_HI | MASK_OVERFLOW_HI)) |
                   (THRESHOLD_MAX - tr->b->threshold_limit);
       } else if (tr->old_limit) {     /* change limit w/o reset */
               int new_count = (hi & THRESHOLD_MAX) +
                   (tr->old_limit - tr->b->threshold_limit);

               hi = (hi & ~MASK_ERR_COUNT_HI) |
                   (new_count & THRESHOLD_MAX);
       }

       /* clear IntType */
       hi &= ~MASK_INT_TYPE_HI;

       if (!tr->b->interrupt_capable)
               goto done;

       if (tr->set_lvt_off) {
               if (lvt_off_valid(tr->b, tr->lvt_off, lo, hi)) {
                       /* set new lvt offset */
                       hi &= ~MASK_LVTOFF_HI;
                       hi |= tr->lvt_off << 20;
               }
       }

       if (tr->b->interrupt_enable)
               hi |= INT_TYPE_APIC;

done:

       hi |= MASK_COUNT_EN_HI;
       wrmsr(tr->b->address, lo, hi);
}


If one were to actually analyze the source file from which this snippet 
comes (lines 117 - 185), one would realize the call to lvt_off_valid() is 
given tr->lvt_off as the input "apic" value that is compared to the content 
in "hi" at bit positions 23:20 (MSR bits 55:52); this field is called LVT 
Offset (LVTOFF).  The value for tr->lvt_off is usually from 0 to 4, 
inclusive.  If this value is equal to the LVTOFF value in "hi", then 
lvt_off_valid() returns 1 for true.  If the value for tr->lvt_off differs 
from the LVTOFF value in "hi", then lvt_off_valid() returns 0 for false.

Now, if the return from lvt_off_valid() is false, then nothing is changed in 
"hi".  However, if the return is true, which means the value in tr->lvt_off 
is equal to the LVTOFF value in "hi", then the LVTOFF value in "hi" is 
replaced with the value in tr->lvt_off.  One has to wonder, then, why bother 
actually calling lvt_off_valid() in the first place when the end result is 
that "hi" does not change.  What is the rationale for having the code 
snippet at lines 170 - 176 when that condition check does nothing to change 
"hi"?

--
Chip 

--
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/