Re: [PATCH] x86/MCE: Implement clearbank callback for AMD

[Christoph Egger <Christoph.Egger@amd.com>]

On 10/25/12 12:13, Christoph Egger wrote:
> On 10/25/12 12:03, Jan Beulich wrote:
>>>>> On 25.10.12 at 11:29, Christoph Egger <Christoph.Egger@amd.com> wrote:
>>> On 10/25/12 10:55, Jan Beulich wrote:
>>>>>>> On 25.10.12 at 10:18, Christoph Egger <Christoph.Egger@amd.com> wrote:
>>>>> On 10/24/12 14:18, Jan Beulich wrote:
>>>>>>>>> On 12.10.12 at 16:46, Christoph Egger <Christoph.Egger@amd.com> wrote:
>>>>>>> +static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
>>>>>>> +{
>>>>>>> +	switch (who) {
>>>>>>> +	case MCA_MCE_SCAN:
>>>>>>> +	case MCA_MCE_HANDLER:
>>>>>>> +		break;
>>>>>>> +	default:
>>>>>>> +		return 1;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/* For fatal error, it shouldn't be cleared so that sticky bank
>>>>>>> +	 * have chance to be handled after reboot by polling.
>>>>>>> +	 */
>>>>>>> +	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
>>>>>>> +		return 0;
>>>>>>> +	/* Spurious need clear bank */
>>>>>>> +	if ( !(status & MCi_STATUS_OVER)
>>>>>>> +	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
>>>>>>> +		return 1;
>>>>>>> +
>>>>>>> +	return 1;
>>>>>>> }
>>>>>>
>>>>>> So what's the purpose of first conditionally returning 1, and then
>>>>>> also doing so unconditionally? Do anticipate to insert code between
>>>>>> the two parts within the very near future? Otherwise I'd drop the
>>>>>> whole if() construct.
>>>>>
>>>>> This function is derived from  intel_need_clearbank_scan().
>>>>> I just took over the relevant parts for AMD.
>>>>
>>>> But that would suggest that the final return be "return 0" rather
>>>> than "return 1".
>>>>
>>>> Further, the Intel code does no extra checking for the
>>>> MCA_MCE_HANDLER case, so I'd like you to confirm that this is
>>>> indeed to be different for your CPUs.
>>>
>>> I just noticed that MCA_MCE_HANDLER is not used at all and can be
>>> removed entirely.
>>
>> Will do.
> 
> Ok, thanks.
> 
>> But how about the "return" related question above?
> 
> I just run some tests.
> Remove this part:
> 
>> +	/* Spurious need clear bank */
>> +	if ( !(status & MCi_STATUS_OVER)
>> +	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
>> +		return 1;
> 

And I changed the switch (who) to:

   if (who != MCA_MCE_SCAN)
       return 1;

Christoph



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