On Wed, Aug 24, 2016 at 12:30:21PM +0200, Lukas Wunner wrote:
(sorry for the delay, it turned into a busy pair of weeks)
> On Tue, Aug 23, 2016 at 12:13:52PM -0400, Peter Jones wrote:
> > This adds the EFI Spec version from the system table to sysfs as
> > /sys/firmware/efi/spec_version .
> >
> > Userland tools need this information in order to work around
> > specification deficiencies in 2.4 and earlier firmwares.
>
> Some details or examples on the nature of those specification
> deficiencies would help an uninitiated reader understand the
> necessity of this patch.
>
> >
> > Signed-off-by: Peter Jones <pjones@xxxxxxxxxx>
> > ---
> > Documentation/ABI/testing/sysfs-firmware-efi | 6 +++
> > arch/ia64/kernel/efi.c | 3 ++
> > drivers/firmware/efi/arm-init.c | 2 +
> > drivers/firmware/efi/efi.c | 71 ++++++++++++++++++++++++++++
> > 4 files changed, 82 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > index e794eac..4eec7c2 100644
> > --- a/Documentation/ABI/testing/sysfs-firmware-efi
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > @@ -28,3 +28,9 @@ Description: Displays the physical addresses of all EFI Configuration
> > versions are always printed first, i.e. ACPI20 comes
> > before ACPI.
> > Users: dmidecode
> > +
> > +What: /sys/firmware/efi/spec_version
> > +Date: August 2016
> > +Contact: linux-efi@xxxxxxxxxxxxxxx
> > +Description: Displays the UEFI Specification revision the firmware claims to
> > + be based upon.
> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> > index 1212956..a48377f 100644
> > --- a/arch/ia64/kernel/efi.c
> > +++ b/arch/ia64/kernel/efi.c
> > @@ -475,6 +475,7 @@ efi_init (void)
> > u64 efi_desc_size;
> > char *cp, vendor[100] = "unknown";
> > int i;
> > + efi_table_hdr_t *hdr;
>
> Unused new variable?
A remnant I apparently have failed to remove from an old version of the
patch.
>
> >
> > set_bit(EFI_BOOT, &efi.flags);
> > set_bit(EFI_64BIT, &efi.flags);
> > @@ -531,6 +532,8 @@ efi_init (void)
> > efi.systab->hdr.revision >> 16,
> > efi.systab->hdr.revision & 0xffff, vendor);
> >
> > + efi.spec_version = efi.systab->hdr.version;
> > +
>
> I think this deserves a mention in the commit message that the
> spec_version struct field was previously not filled for ia64 arch
> and that you're fixing that while you're at it. Alternatively,
> move this to a separate commit.
Sure.
>
> > palo_phys = EFI_INVALID_TABLE_ADDR;
> >
> > if (efi_config_init(arch_tables) != 0)
> > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> > index b89fc23..8e8cb8c 100644
> > --- a/drivers/firmware/efi/arm-init.c
> > +++ b/drivers/firmware/efi/arm-init.c
> > @@ -133,6 +133,8 @@ static int __init uefi_init(void)
> >
> > efi.spec_version = (u32)efi.systab->hdr.revision;
> >
> > + early_memunmap(tmp, sizeof(efi_table_hdr_t));
> > +
>
> What is the connection of this change to exposing the spec_version
> in sysfs? Seems like a fix that should be moved to a separate commit.
This is another remnant from that same previous version; I'll fix it
before resending as well.
>
> > table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
> > config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
> > table_size);
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 5a2631a..5947d19 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -27,6 +27,7 @@
> > #include <linux/slab.h>
> > #include <linux/acpi.h>
> > #include <linux/ucs2_string.h>
> > +#include <linux/bcd.h>
> >
> > #include <asm/early_ioremap.h>
> >
> > @@ -124,6 +125,74 @@ static struct kobj_attribute efi_attr_systab =
> >
> > #define EFI_FIELD(var) efi.var
> >
> > +static ssize_t efi_version_show(struct kobject *kobj,
> > + struct kobj_attribute *attr,
> > + char *buf,
> > + u32 revision)
> > +{
> > + char *str = buf;
> > + u16 major;
> > + u8 minor, tiny;
> > +
> > + if (!kobj || !buf)
> > + return -EINVAL;
> > +
> > + /* The spec says:
> > + * The revision of the EFI Specification to which this table
> > + * conforms. The upper 16 bits of this field contain the major
> > + * revision value, and the lower 16 bits contain the minor revision
> > + * value. The minor revision values are binary coded decimals and are
> > + * limited to the range of 00..99.
> > + *
> > + * When printed or displayed UEFI spec revision is referred as (Major
> > + * revision).(Minor revision upper decimal).(Minor revision lower
> > + * decimal) or (Major revision).(Minor revision upper decimal) in
> > + * case Minor revision lower decimal is set to 0. For example:
> > + *
> > + * A specification with the revision value ((2<<16) | (30)) would be
> > + * referred as 2.3;
> > + * A specification with the revision value ((2<<16) | (31)) would be
> > + * referred as 2.3.1
> > + *
> > + * Then later it says:
> > + * Related Definitions
> > + * #define EFI_SYSTEM_TABLE_SIGNATURE 0x5453595320494249
> > + * #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40))
> > + * #define EFI_2_31_SYSTEM_TABLE_REVISION ((2<<16) | (31))
> > + * #define EFI_2_30_SYSTEM_TABLE_REVISION ((2<<16) | (30))
> > + * #define EFI_2_20_SYSTEM_TABLE_REVISION ((2<<16) | (20))
> > + * #define EFI_2_10_SYSTEM_TABLE_REVISION ((2<<16) | (10))
> > + * #define EFI_2_00_SYSTEM_TABLE_REVISION ((2<<16) | (00))
> > + * #define EFI_1_10_SYSTEM_TABLE_REVISION ((1<<16) | (10))
> > + * #define EFI_1_02_SYSTEM_TABLE_REVISION ((1<<16) | (02))
> > + * #define EFI_SPECIFICATION_VERSION EFI_SYSTEM_TABLE_REVISION
> > + * #define EFI_SYSTEM_TABLE_REVISION EFI_2_40_SYSTEM_TABLE_REVISION
> > + *
> > + * (Apparently this bit of the spec failed to get updated for 2.5
> > + * and 2.6; UefiSpec.h in Tiano has been updated, though.)
> > + */
>
> I'd only include a 2-3 line summary of the spec and move this documentation
> to the commit message in order to keep the code concise.
This is the first time I think I've ever been told by anybody to comment
less :) But I think this comment explains why it's formatting it this
way pretty well, without really breaking up the code meaningfully at
all. I'd rather leave it.
>
> > +
> > + major = (revision & 0xffff0000) >> 16;
> > + minor = bcd2bin((revision & 0x0000ff00) >> 8);
> > + tiny = bcd2bin(revision & 0x000000ff);
>
> You can improve readability by inserting a few blanks like this:
>
> major = (revision & 0xffff0000) >> 16;
> minor = bcd2bin((revision & 0x0000ff00) >> 8);
> tiny = bcd2bin( revision & 0x000000ff);
>
> I'd also use minor_u and minor_l or somesuch instead of "tiny"
> but that's just my preferred bikeshed color.
I've never understood why people think that makes things more readable,
but I can read it fine either way, so sure, I'll take these suggestions
for the next version of the patch.
> > +
> > + if (tiny == 0)
> > + str += sprintf(str, "%d.%d\n", major, minor);
> > + else
> > + str += sprintf(str, "%d.%d.%d\n", major, minor, tiny);
> > +
> > + return str - buf;
>
> Hm, why not return the result of sprintf directly (cast to ssize_t),
> or if you want to handle negative return values properly, store the
> result in an int?
Fair enough.
> > +}
> > +
> > +#define EFI_VERSION_ATTR_SHOW(name) \
> > +static ssize_t name##_show(struct kobject *kobj, \
> > + struct kobj_attribute *attr, char *buf) \
> > +{ \
> > + return efi_version_show(kobj, attr, buf, EFI_FIELD(name)); \
> > +}
> > +
> > +EFI_VERSION_ATTR_SHOW(spec_version);
> > +
>
> Unless you plan to expose other versions like this, you can save on
> code by not using a macro here.
Yep; originally I'd had 3, but it turned out not to be worthwhile.
I'll send an updated version.
--
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
