Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1500102 > unrolled thread

Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry  v3 per ACPI 6.1 Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-10-13 11:00 +0200
    Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry  v3 per ACPI 6.1 "Baicar, Tyler" <tbaicar@codeaurora.org> - 2016-10-13 21:50 +0200
      Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry  v3 per ACPI 6.1 Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-10-14 18:30 +0200
        Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry  v3 per ACPI 6.1 Mark Rutland <mark.rutland@arm.com> - 2016-10-14 18:50 +0200

#1500102 — Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

On 12/10/16 23:10, Baicar, Tyler wrote:
> Hello Suzuki,
>
> Thank you for the feedback! Responses below.
>
>
> On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
>> On 07/10/16 22:31, Tyler Baicar wrote:
>>> Currently when a RAS error is reported it is not timestamped.
>>> The ACPI 6.1 spec adds the timestamp field to the generic error
>>> data entry v3 structure. The timestamp of when the firmware
>>> generated the error is now being reported.
>>>
>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>>
>> Please could you keep the people who reviewed/commented on your series in the past,
>> whenever you post a new version ?
> Do you mean to just send the new version to their e-mail directly in addition to the lists? If so, I will do that next time.

If you send a new version of a series to the list, it is a good idea to keep
the people who commented (significantly) on your previous version in Cc, especially
when you have addressed their feedback. That will help them to keep track of the
series. People can always see the new version in the list, but then it is so easy
to miss something in the 100s of emails you get each day. I am sure people have
special filters for the emails based on if they are in Cc/To etc.

>
> I know you provided good feedback on the previous patchset, but I did not have anyone specifically respond to add "reviewed-by:...". I don't think I should add reviewed-by for someone unless they specifically add it in a response :)

No, I haven't yet "Reviewed-by" your patches. I had some comments on it, which means
I expected it to be addressed as you committed in your response.

>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index 3021f0e..c8488f1 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>> @@ -80,6 +80,10 @@

> I think that should work to avoid duplication. I will move them to a header file in the next patchset.
>>> +
>>> +static void cper_estatus_print_section_v300(const char *pfx,
>>> +    const struct acpi_hest_generic_data_v300 *gdata)
>>> +{
>>> +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
>>> +
>>> +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>>> +        timestamp = (__u8 *)&(gdata->time_stamp);
>>> +        memcpy(&sec, timestamp, 1);
>>> +        memcpy(&min, timestamp + 1, 1);
>>> +        memcpy(&hour, timestamp + 2, 1);
>>> +        memcpy(&day, timestamp + 4, 1);
>>> +        memcpy(&mon, timestamp + 5, 1);
>>> +        memcpy(&year, timestamp + 6, 1);
>>> +        memcpy(&century, timestamp + 7, 1);
>>> +        printk("%stime: ", pfx);
>>> +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>>
>> What format is the (timestamp + 3) stored in ? Does it need conversion ?
> The third byte of the timestamp is currently only used to determine if the time is precise or not. Bit 0 is used to specify that and the other bits in this byte are marked as reserved. This is shown in table 247 of the UEFI spec 2.6:
>
> Byte 3:
>   Bit 0 – Timestamp is precise if this bit is set and correlates to the time of the error event.
>   Bit 7:1 – Reserved

Is it always the same endianness as that of the CPU ?

Cheers
Suzuki

[toc] | [next] | [standalone]

#1500521

Hello Suzuki,

On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
> On 12/10/16 23:10, Baicar, Tyler wrote:
>> Hello Suzuki,
>>
>> Thank you for the feedback! Responses below.
>>
>> On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
>>> On 07/10/16 22:31, Tyler Baicar wrote:
>>>> Currently when a RAS error is reported it is not timestamped.
>>>> The ACPI 6.1 spec adds the timestamp field to the generic error
>>>> data entry v3 structure. The timestamp of when the firmware
>>>> generated the error is now being reported.
>>>>
>>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>>>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>>>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>>>
>>> Please could you keep the people who reviewed/commented on your 
>>> series in the past,
>>> whenever you post a new version ?
>> Do you mean to just send the new version to their e-mail directly in 
>> addition to the lists? If so, I will do that next time.
>
> If you send a new version of a series to the list, it is a good idea 
> to keep
> the people who commented (significantly) on your previous version in 
> Cc, especially
> when you have addressed their feedback. That will help them to keep 
> track of the
> series. People can always see the new version in the list, but then it 
> is so easy
> to miss something in the 100s of emails you get each day. I am sure 
> people have
> special filters for the emails based on if they are in Cc/To etc.
>
Okay, understood. I'll make sure to add those who have commented in the 
cc/to list to avoid the e-mail filters.
>>
>> I know you provided good feedback on the previous patchset, but I did 
>> not have anyone specifically respond to add "reviewed-by:...". I 
>> don't think I should add reviewed-by for someone unless they 
>> specifically add it in a response :)
>
> No, I haven't yet "Reviewed-by" your patches. I had some comments on 
> it, which means
> I expected it to be addressed as you committed in your response.
>
>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>> index 3021f0e..c8488f1 100644
>>>> --- a/drivers/acpi/apei/ghes.c
>>>> +++ b/drivers/acpi/apei/ghes.c
>>>> @@ -80,6 +80,10 @@
>
>> I think that should work to avoid duplication. I will move them to a 
>> header file in the next patchset.
>>>> +
>>>> +static void cper_estatus_print_section_v300(const char *pfx,
>>>> +    const struct acpi_hest_generic_data_v300 *gdata)
>>>> +{
>>>> +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
>>>> +
>>>> +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>>>> +        timestamp = (__u8 *)&(gdata->time_stamp);
>>>> +        memcpy(&sec, timestamp, 1);
>>>> +        memcpy(&min, timestamp + 1, 1);
>>>> +        memcpy(&hour, timestamp + 2, 1);
>>>> +        memcpy(&day, timestamp + 4, 1);
>>>> +        memcpy(&mon, timestamp + 5, 1);
>>>> +        memcpy(&year, timestamp + 6, 1);
>>>> +        memcpy(&century, timestamp + 7, 1);
>>>> +        printk("%stime: ", pfx);
>>>> +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>>>
>>> What format is the (timestamp + 3) stored in ? Does it need 
>>> conversion ?
>> The third byte of the timestamp is currently only used to determine 
>> if the time is precise or not. Bit 0 is used to specify that and the 
>> other bits in this byte are marked as reserved. This is shown in 
>> table 247 of the UEFI spec 2.6:
>>
>> Byte 3:
>>   Bit 0 – Timestamp is precise if this bit is set and correlates to 
>> the time of the error event.
>>   Bit 7:1 – Reserved
>
> Is it always the same endianness as that of the CPU ?
It is a fair assumption that the firmware populating this record will 
use a CPU of the same endianness. There is no mechanism in the spec to 
indicate otherwise.

Thanks,
Tyler
>
> Cheers
> Suzuki
>
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1501087

On 13/10/16 20:37, Baicar, Tyler wrote:
> Hello Suzuki,
>
> On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
>> On 12/10/16 23:10, Baicar, Tyler wrote:

>>>> Please could you keep the people who reviewed/commented on your series in the past,
>>>> whenever you post a new version ?
>>> Do you mean to just send the new version to their e-mail directly in addition to the lists? If so, I will do that next time.
>>
>> If you send a new version of a series to the list, it is a good idea to keep
>> the people who commented (significantly) on your previous version in Cc, especially
>> when you have addressed their feedback. That will help them to keep track of the
>> series. People can always see the new version in the list, but then it is so easy
>> to miss something in the 100s of emails you get each day. I am sure people have
>> special filters for the emails based on if they are in Cc/To etc.
>>
> Okay, understood. I'll make sure to add those who have commented in the cc/to list to avoid the e-mail filters.

Thanks !

>> Is it always the same endianness as that of the CPU ?
> It is a fair assumption that the firmware populating this record will use a CPU of the same endianness. There is no mechanism in the spec to indicate otherwise.

Yep, you are right. The EFI expects the EL2/EL1 to be of the same endianness

Cheers
Suzuki

[toc] | [prev] | [next] | [standalone]

#1501092

On Fri, Oct 14, 2016 at 05:28:58PM +0100, Suzuki K Poulose wrote:
> On 13/10/16 20:37, Baicar, Tyler wrote:
> >On 10/13/2016 2:50 AM, Suzuki K Poulose wrote:
> >>Is it always the same endianness as that of the CPU ?
> >
> >It is a fair assumption that the firmware populating this record will
> >use a CPU of the same endianness. There is no mechanism in the spec
> >to indicate otherwise.
> 
> Yep, you are right. The EFI expects the EL2/EL1 to be of the same endianness

To be clear, it is specifically required in the ACPI spec that elements
are in little-endian. Per the ACPI 6.1 spec, page 109:

	All numeric values in ACPI-defined tables, blocks, and
	structures are always encoded in little endian
	format.

Given that CPER, HEST, etc are defined within the ACPI specification,
they are covered by this requirement.

Elements outside of the ACPI spec are not necessarily covered by this
requirement, but their specifications should state their endianness.

Thanks,
Mark.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web