On 12/14/11 13:42, Jan Beulich wrote:
>>>> On 14.12.11 at 11:17, Christoph Egger<Christoph.Egger@amd.com>  wrote:
>> +struct mpbhdr {
>> +    uint32_t type;
>> +    uint32_t len;
>> +    const uint8_t data;
>
> What's this?

This little structure eliminates the uses buf_pos[x] throughout the
code which is much easier to read and understand.

> If anything, this should be data[] (and no need for const).

Then it is data[]

>> @@ -141,16 +142,19 @@ static int apply_microcode(int cpu)
>>      struct ucode_cpu_info *uci =&per_cpu(ucode_cpu_info, cpu);
>>      uint32_t rev;
>>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>> +    struct microcode_header_amd *hdr = mc_amd->mpb;
>
> Using mc_amd here, but ...
>
>>      /* We should bind the task to the CPU */
>>      BUG_ON(raw_smp_processor_id() != cpu);
>>
>>      if ( mc_amd == NULL )
>>          return -EINVAL;
>
> ... the NULL check is only here.
>
>> +    if ( mc_amd->mpb == NULL )
>> +        return -EINVAL;
>
> And quite obviously you should preferably use the local variable
> here...
>
>> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code);
>> +    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);
>
> ... and here.

done (assuming you mean 'hdr').

>> +    printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n",
>> +           (unsigned long)bufsize, mpbuf->len, off);
>
> The cast is pointless; to avoid it, use %zu as format string or declare
> the parameter as 'unsigned long'.

done.

>> +    if (mc_amd->mpb_size<  mpbuf->len) {
>> +        xfree(mc_amd->mpb);
>> +        mc_amd->mpb = xmalloc_bytes(mpbuf->len);
>> +        mc_amd->mpb_size = mpbuf->len;
>
> NULL check missing here (and in such event the clearing of
> ->mpb_size).

done.

>> +    memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);
>
> Unnecessary cast.

Right.

>>          printk(KERN_ERR "microcode: error! Wrong "
>> -               "microcode patch file magic\n");
>> +            "microcode patch file magic\n");
>
> Why are you still mis-adjusting indentation here?

oh...

>> +    mc_amd = xmalloc_bytes(sizeof(*mc_amd));
>
> As said during the first review round - this ought to be
>
>      mc_amd = xmalloc(struct microcode_amd);

sorry, didn't realize that it was not only relevant to the shown line
of code.

>> +    while ( (ret = get_next_ucode_from_buffer_amd(mc_amd,
>> +        buf, bufsize,&offset)) == 0 )
>
> Bad indentation.

Fixed.

>> +    ASSERT(src != NULL);
>
> Pointless.

Maybe. Maybe not. Anyway, I removed it.

>> +        if (mc_amd != NULL) {
>
> While this NULL check is needed, ...
>
>> +            if (mc_amd->equiv_cpu_table)
>
> ... this and ...
>
>> +                xfree(mc_amd->equiv_cpu_table);
>> +            if (mc_amd->mpb)
>
> ... this are pointless.
>
>> +                xfree(mc_amd->mpb);
>> +            xfree(mc_amd);
>> +        }
>> +
>> +        mc_amd = xmalloc(struct microcode_amd);
>
>      uci->mc.mc_amd = mc_amd;
>
>>          if ( !mc_amd )
>
> (as was the case in the original code). No need to do this once in the
> success path and once in the error one.
>
>> +    uci->mc.mc_amd = mc_amd = NULL;
>> +    return -ENOMEM;
>
> Even if it was necessary to keep it this way, initializing a local variable
> immediately before returning is bogus (and calling for a compiler
> warning and hence a build failure due to -Werror sooner or later).

Fixed.

New version attached.
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

> Jan
>
>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632