Re: [PATCH V3] x86, amd_ucode: Support multiple container files appended together

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 6/30/2014 4:32 AM, Jan Beulich wrote:
>> Yes, we found a match and yes, we applied the patch successfully.
>> But, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE )
>> and return -EINVAL
>> which is assigned to the variable 'error'
>> (Assuming ofc that there is a second container there which we don't need
>> to parse because
>> we have already succeeded in patch application)
>>
>> This is what I wanted to convey from
>>
>> "astale  bogus error val is returned from get_ucode_from_buffer_amd."
>>
>> But, we need to return 0 on success; which is why this change is needed
>> here..
> I think I understand now: This talks about the case of a _subsequent_
> container (never really looked at) following, causing the unwanted
> -EINVAL. Whereas your comment said "earlier container", implying (to
> me) that it talks about one that earlier code did look at.
>
>>>>        }
>>>>    
>>>>        if ( save_error )
>>>>        {
>>>> +        /*
>>>> +         * By the time 'microcode_init' runs, we have already updated the
>>>> +         * patch level on all (currently) running cpus.
>>>> +         * But, get_ucode_from_buffer_amd will return -EINVAL as
>>>> +         * if ( mpbuf->type != UCODE_UCODE_TYPE ) fails in this case:
>>>> +         * Multiple containers are present && update succeeded with the
>>>> +         * first container file itself.
>>>> +         *
>>>> +         * Only this time, there is no 'applied_offset' as well.
>>>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>>> +         * Hence, this check is necessary to return 0 for success.
>>>> +         */
>>>> +        if ( (error != save_error) && (offset < bufsize) )
>>>> +            error = 0;
>>> Same for this change/comment.
>>>
>> Hmm.. I'm having trouble trying to re-word this comment then..
>>
>> Given the situation where  - we have already applied the patch update after
>> 'microcode_presmp_init' and 'microcode_resume_cpu';
>>      |
>>      v
>> Now 'microcode_init' runs and calls into 'cpu_request_microcode';
>>      |
>>      v
>> We use 1st while loop to find_equiv_cpu_id() and match it with the container
>>      |
>>      v
>> For this particular case, we assume it's a match on the 1st container;
>> so break
>>      |
>>      v
>> Enter while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>>      |
>>      v
>> At some point, it will find the correct patch; but this time there is no
>> need to update
>>      |
>>      v
>> The behavior is now similar to what I have described above. i.e
>> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf,
>> bufsize,&offset)) == 0 )
>> is going to, at some point hit if ( mpbuf->type != UCODE_UCODE_TYPE )
>> and return -EINVAL
>> which is assigned to the variable 'error'
>>      |
>>      v
>> But, now (as stated in the comment..)
>>
>> * Only this time, there is no 'applied_offset' as well.
>> +         * So, 'save_error' = 1. But error = -EINVAL.
>>
>>      |
>>      v
>> And since we need to return 0 for success, this change is needed here.
> So since this is similar to the previous comment, rather than
> duplicating information, perhaps just refer to the earlier one, adding
> _only_ the information of the different aspect(s) here. And use the
> right words: To me at least the "Only this time" implies something
> different than what I think you mean - "Except that this time ..."
> would be the words I'd use (but a native English speaker may need
> to be consulted in case you view this differently).
>
>

Ok. I have followed a similar step-wise approach  (think this is simpler 
to follow) in the comments
for V4. Hopefully it's more precisely worded..

I am also working to document container file format, where I will also 
talk about why a correct patch can
be on only one or the other container, but not both.

Thanks,
-Aravind.