Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1499073
| From | Suzuki K Poulose <Suzuki.Poulose@arm.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1 |
| Date | 2016-10-11 19:40 +0200 |
| Message-ID | <sr9c0-195-81@gated-at.bofh.it> (permalink) |
| References | <spL21-6h3-1@gated-at.bofh.it> <spL22-6h3-31@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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 ?
> ---
> drivers/acpi/apei/ghes.c | 25 ++++++++++--
> drivers/firmware/efi/cper.c | 97 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 105 insertions(+), 17 deletions(-)
>
> 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 @@
> ((struct acpi_hest_generic_status *) \
> ((struct ghes_estatus_node *)(estatus_node) + 1))
>
> +#define acpi_hest_generic_data_version(gdata) \
> + (gdata->revision >> 8)
...
> +inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> +{
> + return acpi_hest_generic_data_version(gdata) >= 3 ?
> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> + gdata + 1;
> +}
> +
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..9fa1317 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> +#define acpi_hest_generic_data_version(gdata) \
> + (gdata->revision >> 8)
> +
...
> +static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> +{
> + return acpi_hest_generic_data_version(gdata) >= 3 ?
> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> + gdata + 1;
> +}
Could these go to a header file, so that we don't need duplicate definitions of these helpers in
different files ?
> +
> +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(¢ury, timestamp + 7, 1);
> + printk("%stime: ", pfx);
> + printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
What format is the (timestamp + 3) stored in ? Does it need conversion ?
> + printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> + bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> + bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> + bcd2bin(day));
> + }
minor nit: Would it be easier to order/parse the error messages if the date
is printed first followed by time ?
i.e,
17:20:14 2016-09-15 Mon
vs
2016-09-15 Mon 17:20:14
e.g, people looking at a huge log, looking for logs from a specific date might
find the latter more useful to skip the messages.
> +}
> +
> static void cper_estatus_print_section(
> - const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
> + const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
> {
> uuid_le *sec_type = (uuid_le *)gdata->section_type;
> __u16 severity;
> char newpfx[64];
>
> + if ((gdata->revision >> 8) >= 0x03)
Could we use the helper defined above ?
> @@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
> printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
> data_len = estatus->data_length;
> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> + if ((gdata->revision >> 8) >= 0x03)
Same as above, use the macro ?
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +
> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +
> while (data_len >= sizeof(*gdata)) {
> gedata_len = gdata->error_data_length;
> cper_estatus_print_section(newpfx, gdata, sec_no);
> - data_len -= gedata_len + sizeof(*gdata);
> - gdata = (void *)(gdata + 1) + gedata_len;
> + if(gdata_v3) {
> + data_len -= gedata_len + sizeof(*gdata_v3);
> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> + gdata = (struct acpi_hest_generic_data *)gdata_v3;
> + } else {
> + data_len -= gedata_len + sizeof(*gdata);
> + gdata = (void *)(gdata + 1) + gedata_len;
> + }
> sec_no++;
> }
...
>
> @@ -486,15 +543,29 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
> return rc;
> data_len = estatus->data_length;
> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> - while (data_len >= sizeof(*gdata)) {
> - gedata_len = gdata->error_data_length;
> - if (gedata_len > data_len - sizeof(*gdata))
> +
> + if ((gdata->revision >> 8) >= 0x03) {
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> + while (data_len >= sizeof(*gdata_v3)) {
> + gedata_len = gdata_v3->error_data_length;
> + if (gedata_len > data_len - sizeof(*gdata_v3))
> + return -EINVAL;
> + data_len -= gedata_len + sizeof(*gdata_v3);
> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> + }
> + if (data_len)
> + return -EINVAL;
> + } else {
> + while (data_len >= sizeof(*gdata)) {
> + gedata_len = gdata->error_data_length;
> + if (gedata_len > data_len - sizeof(*gdata))
> + return -EINVAL;
> + data_len -= gedata_len + sizeof(*gdata);
> + gdata = (void *)(gdata + 1) + gedata_len;
> + }
> + if (data_len)
As mentioned in the previous version, would it make sense to add some more
helpers to deal with record versions ? We seem to be doing the version switch and
code duplication at different places.
Does the following help ? Thoughts ?
#define acpi_hest_generic_data_error_length(gdata) (((struct acpi_hest_generic_data *)(gdata))->error_data_length)
#define acpi_hest_generic_data_size(gdata) \
((acpi_hest_generic_data_version(gdata) >= 3) ? \
sizeof(struct acpi_hest_generic_data_v300) : \
sizeof(struct acpi_hest_generic_data))
#define acpi_hest_generic_data_record_size(gdata)
(acpi_hest_generic_data_size(gdata) + \
acpi_hest_generic_data_error_length(gdata))
#define acpi_hest_generic_data_next(gdata) \
((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
Suzuki
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH V3 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64 Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
[PATCH V3 07/10] efi: print unrecognized CPER section Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
[PATCH V3 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
[PATCH V3 10/10] arm64: KVM: add guest SEA support Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
[PATCH V3 03/10] efi: parse ARMv8 processor error Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
[PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
Re: [PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption Punit Agrawal <punit.agrawal@arm.com> - 2016-10-12 20:20 +0200
[PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8 Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8 Punit Agrawal <punit.agrawal@arm.com> - 2016-10-12 20:10 +0200
[PATCH V3 04/10] arm64: exception: handle Synchronous External Abort Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
Re: [PATCH V3 04/10] arm64: exception: handle Synchronous External Abort Punit Agrawal <punit.agrawal@arm.com> - 2016-10-12 19:50 +0200
[PATCH V3 09/10] trace, ras: add ARM processor error trace event Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
Re: [PATCH V3 09/10] trace, ras: add ARM processor error trace event Steven Rostedt <rostedt@goodmis.org> - 2016-10-07 23:50 +0200
Re: [PATCH V3 09/10] trace, ras: add ARM processor error trace event "Baicar, Tyler" <tbaicar@codeaurora.org> - 2016-10-12 23:30 +0200
[PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1 Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +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-11 19:40 +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 00:30 +0200
Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1 Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-10-11 21:10 +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 00:20 +0200
[PATCH V3 06/10] acpi: apei: panic OS with fatal error status block Tyler Baicar <tbaicar@codeaurora.org> - 2016-10-07 23:40 +0200
csiph-web