[PATCH 08/12] sbbr/rsdp: Add initial rsdp acpi tests as per sbbr.
Supreeth Venkatesh
supreeth.venkatesh at arm.com
Thu Mar 2 23:58:40 UTC 2017
On Thu, 2017-03-02 at 23:52 +0000, Colin Ian King wrote:
> I'm getting white space issues when I apply the patch:
>
> Applying: sbbr/rsdp: Add initial rsdp acpi tests as per sbbr.
> .git/rebase-apply/patch:120: space before tab in indent.
> fwts_passed(fw, "SBBR: Structure of RSDP Table is
> consistent with
> ACPI 6.0 or later and uses 64 bit xsdt addresses.");
> .git/rebase-apply/patch:124: space before tab in indent.
> fwts_failed(fw, LOG_LEVEL_CRITICAL,
> .git/rebase-apply/patch:125: space before tab in indent.
> "SBBR RSDP:",
> .git/rebase-apply/patch:126: space before tab in indent.
> "Structure of RSDP Table is not
> consistent with ACPI 6.0 or
> later and/or does not use 64 bit xsdt addresses.");
> warning: 4 lines add whitespace errors.
>
Sorry, I may have messed up. Do you have recommendation like
patchcheck.py or something similar which checks style and whitespace
issues to check patches before sending it out?
> On 02/03/17 22:26, Supreeth Venkatesh wrote:
> >
> > Server Base Boot Requirements (SBBR) specification is intended for
> > SBSA-
> > compliant 64-bit ARMv8 servers.
> > It defines the base firmware requirements for out-of-box support of
> > any
> > ARM SBSA-compatible Operating System or hypervisor.
> > The requirements in this specification are expected to be minimal
> > yet
> > complete for booting a multi-core ARMv8 server platform, while
> > leaving
> > plenty of room for OEM or ODM innovations and design details.
> > For more information, download the SBBR specification here:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044
> > b/index.html
> >
> > This change introduces test cases as per SBBR specification to rsdp
> > acpi. These test cases may be subset/superset of rsdp acpi tests
> > already
> > existing. However, to preserve "sbbr" classification, new file is
> > created, even when most of the code is re-used from acpi/rsdp.
> >
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh at arm.com>
> > ---
> > src/sbbr/rsdp/rsdp.c | 134
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 134 insertions(+)
> > create mode 100644 src/sbbr/rsdp/rsdp.c
> >
> > diff --git a/src/sbbr/rsdp/rsdp.c b/src/sbbr/rsdp/rsdp.c
> > new file mode 100644
> > index 0000000..0a11f91
> > --- /dev/null
> > +++ b/src/sbbr/rsdp/rsdp.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Copyright (C) 2015-2017 Canonical
> > + * Copyright (C) 2017 ARM Ltd
> > + *
> > + * Portions of this code original from the Linux-ready Firmware
> > Developer Kit
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#include "fwts.h"
> > +
> > +#if defined(FWTS_HAS_SBBR)
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +#include <inttypes.h>
> > +#include <string.h>
> > +#include <ctype.h>
> > +
> > +static fwts_acpi_table_info *table;
> > +
> > +static int sbbr_rsdp_init(fwts_framework *fw)
> > +{
> > + if (fwts_acpi_find_table(fw, "RSDP", 0, &table) !=
> > FWTS_OK) {
> > + fwts_log_error(fw, "Cannot read ACPI tables.");
> > + return FWTS_ERROR;
> > + }
> > + if (!table) {
> > +
> > + fwts_log_error(fw,
> > + "ACPI RSDP is required for
> > the "
> > + "%s target architecture.",
> > + fwts_arch_get_name(fw-
> > >target_arch));
> > + return FWTS_ERROR;
> > + }
> formatting has extra tabs, please clean that up.
>
> >
> > +
> > + /* We know there is an RSDP now, so do a quick sanity
> > check */
> > + if (table->length == 0) {
> > + fwts_log_error(fw,
> > + "ACPI RSDP table has zero length");
> > + return FWTS_ERROR;
> > + }
> > + return FWTS_OK;
> > +}
> > +
> > +/*
> > + * RSDP Root System Description Pointer
> > + */
> > +static int sbbr_rsdp_test1(fwts_framework *fw)
> > +{
> > + fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp
> > *)table->data;
> > + uint8_t checksum;
> > +
> > + /*This includes only the first 20 bytes of this table,
> > bytes
> > + 0 to 19, including the checksum field. These bytes must
> > sum to
> > + zero. */
> I'd prefer if the comments in the code (and other patches) would
> match
> the general fwts block style, e.g.
>
> /*
> * This includes only the first 20 bytes of this table, bytes
> * 0 to 19, including the checksum field. These bytes must
> * sum to zero.
> */
>
> >
> > + const char RSDP_SIGNATURE[] = {'R', 'S', 'D', ' ', 'P',
> > 'T', 'R', ' '};
> > + bool signature_pass = false;
> you don't need to initialize the above boolean
>
> >
> > + const int CHECKSUM_BYTES = 20;
> > + bool checksum_pass = false;
> ..and this one ^
>
> >
> > + const int SBBR_RSDP_REVISION = 2;
> > + bool rsdp_revision_pass = false;
> ..and this one ^
>
> >
> > + const uint32_t SBBR_RSDP_LENGTH = 36;
> > + bool rsdp_length_pass = false;
> ..and this one ^
>
> >
> > + const int EXT_CHECKSUM_BYTES = 36;
> > + bool ext_checksum_pass = false;
> > + bool xsdt_address_pass = false;
> please don't make const variables all caps. lowercase is the
> convention.
>
> >
> > +
> > + fwts_log_info(fw, "RSDP Signature = %.8s", rsdp-
> > >signature);
> > + signature_pass = strncmp(rsdp->signature, RSDP_SIGNATURE,
> > sizeof(rsdp->signature))? false : true;
> quite a long line, can it be less than 80 cols wide
>
> >
> > +
> > + /* verify first checksum */
> > + checksum = fwts_checksum(table->data, CHECKSUM_BYTES);
> > + fwts_log_info(fw, "RSDP Checksum = 0x%x", checksum);
> > + checksum_pass = (checksum == 0)? true : false;
> this is better:
>
> checksum_pass = (checksum == 0);
>
> >
> > +
> > + fwts_log_info(fw, "RSDP Revision = 0x%x", rsdp->revision);
> > + rsdp_revision_pass = (rsdp->revision >=
> > SBBR_RSDP_REVISION)? true : false;
> space please between ) and ?
> ... SBBR_RSDP_REVISION) ? true : false;
> >
> > +
> > + fwts_log_info(fw, "RSDP Length = 0x%x", rsdp->length);
> > + rsdp_length_pass = (rsdp->length == SBBR_RSDP_LENGTH)?
> > true : false;
> space between )?
>
> >
> > +
> > + checksum = fwts_checksum(table->data, EXT_CHECKSUM_BYTES);
> > + fwts_log_info(fw, "RSDP Extended Checksum = 0x%x",
> > checksum);
> > + ext_checksum_pass = (checksum == 0)? true : false;
> space between )?
> >
> > +
> > + if ( (rsdp->xsdt_address != 0) &&
> > + (rsdp->rsdt_address == 0) )
> > + {
> > + xsdt_address_pass = true;
> > + }
> format the above as follows is preferred:
>
> if ((rsdp->xsdt_address != 0) &&
> (rsdp->rsdt_address == 0))
> xsdt_address_pass = true;
>
> >
> > +
> > + if ( (signature_pass == true) &&
> > + (checksum_pass == true) &&
> > + (rsdp_revision_pass == true) &&
> > + (rsdp_length_pass == true) &&
> > + (ext_checksum_pass == true) &&
> > + (xsdt_address_pass == true) )
> > + {
> > + fwts_passed(fw, "SBBR: Structure of RSDP Table is
> > consistent with ACPI 6.0 or later and uses 64 bit xsdt
> > addresses.");
> > + }
> > + else
> > + {
> > + fwts_failed(fw, LOG_LEVEL_CRITICAL,
> > + "SBBR RSDP:",
> > + "Structure of RSDP Table is
> > not consistent with ACPI 6.0 or later and/or does not use 64 bit
> > xsdt addresses.");
> > + }
> > +
> The formatting style above needs reworking,
>
> if (( ... ) &&
> ( ... ) &&
> ....
> ( ... ))
> fwts_passed(...)
> else
> fwts_failed(...)
>
>
> >
> > + return FWTS_OK;
> > +}
> > +
> > +static fwts_framework_minor_test sbbr_rsdp_tests[] = {
> > + { sbbr_rsdp_test1, "RSDP Root System Description Pointer
> > test." },
> > + { NULL, NULL }
> > +};
> > +
> > +static fwts_framework_ops sbbr_rsdp_ops = {
> > + .description = "SBBR RSDP Root System Description Pointer
> > tests.",
> > + .init = sbbr_rsdp_init,
> > + .minor_tests = sbbr_rsdp_tests
> > +};
> > +
> > +FWTS_REGISTER("sbbr_rsdp", &sbbr_rsdp_ops, FWTS_TEST_ANYTIME,
> > FWTS_FLAG_TEST_SBBR)
> > +
> > +#endif
> >
More information about the fwts-devel
mailing list