[GIT PULL] EFI changes for v4.2

[GIT PULL] EFI changes for v4.2

[Matt Fleming]

Hi folks,

Please pull the following new material for v4.2. The most interesting
change is support for the EFI System Resource Table contributed by Peter
Jones. ESRT is new in the UEFI 2.5 specification (released April 2015)
and is used by Fedora's fwupdate tool when performing firmware updates.

The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:

  Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next

for you to fetch changes up to c208358c2cc832eeb5b341a465eee30be19cd5a0:

  efi: Add 'systab' information to Documentation/ABI (2015-05-27 15:40:20 +0100)

----------------------------------------------------------------
 * Use idiomatic negative error values in efivar_create_sysfs_entry()
   instead of returning '1' to indicate error - Dan Carpenter

 * New support to expose the EFI System Resource Tables in sysfs, which
   provides information for performing firmware updates - Peter Jones

 * Documentation cleanup in the EFI handover protocol section which
   falsely claimed that 'cmdline_size' needed to be filled out by the
   boot loader - Alex Smith

 * Align the order of SMBIOS tables in /sys/firmware/efi/systab to match
   the way that we do things for ACPI and add documentation to
   Documentation/ABI - Jean Delvare

----------------------------------------------------------------
Alex Smith (1):
      x86, doc: Remove cmdline_size from list of fields to be filled in for EFI handover

Dan Carpenter (1):
      efi: efivar_create_sysfs_entry() should return negative error codes

Jean Delvare (1):
      efi: dmi: List SMBIOS3 table before SMBIOS table

Matt Fleming (1):
      efi: Add 'systab' information to Documentation/ABI

Peter Jones (2):
      efi: Add esrt support
      efi/esrt: Fix some compiler warnings

 Documentation/ABI/testing/sysfs-firmware-efi      |  10 +
 Documentation/ABI/testing/sysfs-firmware-efi-esrt |  81 ++++
 Documentation/x86/boot.txt                        |   1 -
 arch/x86/platform/efi/efi.c                       |   2 +
 drivers/firmware/efi/Makefile                     |   2 +-
 drivers/firmware/efi/efi.c                        |  91 ++++-
 drivers/firmware/efi/efivars.c                    |  11 +-
 drivers/firmware/efi/esrt.c                       | 471 ++++++++++++++++++++++
 include/linux/efi.h                               |   8 +
 9 files changed, 667 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-esrt
 create mode 100644 drivers/firmware/efi/esrt.c

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [GIT PULL] EFI changes for v4.2

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> Hi folks,
> 
> Please pull the following new material for v4.2. The most interesting
> change is support for the EFI System Resource Table contributed by Peter
> Jones. ESRT is new in the UEFI 2.5 specification (released April 2015)
> and is used by Fedora's fwupdate tool when performing firmware updates.
> 
> The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:
> 
>   Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next
> 
> for you to fetch changes up to c208358c2cc832eeb5b341a465eee30be19cd5a0:
> 
>   efi: Add 'systab' information to Documentation/ABI (2015-05-27 15:40:20 +0100)
> 
> ----------------------------------------------------------------
>  * Use idiomatic negative error values in efivar_create_sysfs_entry()
>    instead of returning '1' to indicate error - Dan Carpenter
> 
>  * New support to expose the EFI System Resource Tables in sysfs, which
>    provides information for performing firmware updates - Peter Jones
> 
>  * Documentation cleanup in the EFI handover protocol section which
>    falsely claimed that 'cmdline_size' needed to be filled out by the
>    boot loader - Alex Smith
> 
>  * Align the order of SMBIOS tables in /sys/firmware/efi/systab to match
>    the way that we do things for ACPI and add documentation to
>    Documentation/ABI - Jean Delvare
> 
> ----------------------------------------------------------------
> Alex Smith (1):
>       x86, doc: Remove cmdline_size from list of fields to be filled in for EFI handover
> 
> Dan Carpenter (1):
>       efi: efivar_create_sysfs_entry() should return negative error codes
> 
> Jean Delvare (1):
>       efi: dmi: List SMBIOS3 table before SMBIOS table
> 
> Matt Fleming (1):
>       efi: Add 'systab' information to Documentation/ABI
> 
> Peter Jones (2):
>       efi: Add esrt support
>       efi/esrt: Fix some compiler warnings
> 
>  Documentation/ABI/testing/sysfs-firmware-efi      |  10 +
>  Documentation/ABI/testing/sysfs-firmware-efi-esrt |  81 ++++
>  Documentation/x86/boot.txt                        |   1 -
>  arch/x86/platform/efi/efi.c                       |   2 +
>  drivers/firmware/efi/Makefile                     |   2 +-
>  drivers/firmware/efi/efi.c                        |  91 ++++-
>  drivers/firmware/efi/efivars.c                    |  11 +-
>  drivers/firmware/efi/esrt.c                       | 471 ++++++++++++++++++++++
>  include/linux/efi.h                               |   8 +
>  9 files changed, 667 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-esrt
>  create mode 100644 drivers/firmware/efi/esrt.c

Pulled, thanks a lot Matt!

I'm wondering about this commit:

  f2f6b587c553 ("efi/esrt: Fix some compiler warnings")

It says, in part:

 This patch also fixes one other warning about an uninitialized variable
 some compiler versions seem to see.  You can't actually hit the code
 path where it would be uninitialized, because there's a prior test that
 would error out, but gcc hasn't figured that out.  Anyway, it now has a
 test and returns the error at both places.

@@ -167,7 +167,6 @@ static struct kset *esrt_kset;
 
 static int esre_create_sysfs_entry(void *esre, int entry_num)
 {
-       int rc = 0;
        struct esre_entry *entry;
        char name[20];
 
@@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
        entry->kobj.kset = esrt_kset;
 
        if (esrt->fw_resource_version == 1) {
+               int rc = 0;
+
                entry->esre.esre1 = esre;
                rc = kobject_init_and_add(&entry->kobj, &esre1_ktype, NULL,
                                          "%s", name);
-       }
-       if (rc) {
-               kfree(entry);
-               return rc;
+               if (rc) {
+                       kfree(entry);
+                       return rc;
+               }
        }
 
        list_add_tail(&entry->list, &entry_list);

How can a compiler ever have warned about 'rc' being uninitialized? It's defined 
straight at function entry, with initialization to 0. It can never be 
uninitialized.

I pulled it, because I agree with the change itself, as it's always better to 
define and use variables in the narrowest scope possible, but I think it's a 
cleanup, not a compiler warning fix.

Thanks,

	Ingo

Re: [GIT PULL] EFI changes for v4.2

[Matt Fleming]

On Tue, 02 Jun, at 08:45:57AM, Ingo Molnar wrote:
> 
> Pulled, thanks a lot Matt!
> 
> I'm wondering about this commit:
> 
>   f2f6b587c553 ("efi/esrt: Fix some compiler warnings")
> 
> It says, in part:
> 
>  This patch also fixes one other warning about an uninitialized variable
>  some compiler versions seem to see.  You can't actually hit the code
>  path where it would be uninitialized, because there's a prior test that
>  would error out, but gcc hasn't figured that out.  Anyway, it now has a
>  test and returns the error at both places.
> 
> @@ -167,7 +167,6 @@ static struct kset *esrt_kset;
>  
>  static int esre_create_sysfs_entry(void *esre, int entry_num)
>  {
> -       int rc = 0;
>         struct esre_entry *entry;
>         char name[20];
>  
> @@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
>         entry->kobj.kset = esrt_kset;
>  
>         if (esrt->fw_resource_version == 1) {
> +               int rc = 0;
> +
>                 entry->esre.esre1 = esre;
>                 rc = kobject_init_and_add(&entry->kobj, &esre1_ktype, NULL,
>                                           "%s", name);
> -       }
> -       if (rc) {
> -               kfree(entry);
> -               return rc;
> +               if (rc) {
> +                       kfree(entry);
> +                       return rc;
> +               }
>         }
>  
>         list_add_tail(&entry->list, &entry_list);
> 
> How can a compiler ever have warned about 'rc' being uninitialized? It's defined 
> straight at function entry, with initialization to 0. It can never be 
> uninitialized.
 
Hmm... yeah good point. I admit that I missed that when merging this
patch because I was focusing more on the fix for the kbuild warnings
related to phys_addr_t.

Peter? Where did you see gcc complaining about this?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [GIT PULL] EFI changes for v4.2

[Peter Jones]

On Tue, Jun 02, 2015 at 08:45:57AM +0200, Ingo Molnar wrote:
> @@ -167,7 +167,6 @@ static struct kset *esrt_kset;
>  
>  static int esre_create_sysfs_entry(void *esre, int entry_num)
>  {
> -       int rc = 0;
>         struct esre_entry *entry;
>         char name[20];
>  
> @@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
>         entry->kobj.kset = esrt_kset;
>  
>         if (esrt->fw_resource_version == 1) {
> +               int rc = 0;
> +
>                 entry->esre.esre1 = esre;
>                 rc = kobject_init_and_add(&entry->kobj, &esre1_ktype, NULL,
>                                           "%s", name);
> -       }
> -       if (rc) {
> -               kfree(entry);
> -               return rc;
> +               if (rc) {
> +                       kfree(entry);
> +                       return rc;
> +               }
>         }
>  
>         list_add_tail(&entry->list, &entry_list);
> 
> How can a compiler ever have warned about 'rc' being uninitialized? It's defined 
> straight at function entry, with initialization to 0. It can never be 
> uninitialized.
>
> I pulled it, because I agree with the change itself, as it's always better to 
> define and use variables in the narrowest scope possible, but I think it's a 
> cleanup, not a compiler warning fix.

Well, apparently I failed to explain it well - the warning was about
"esre" rather than "rc".  Basically before we were testing the version in
register_entries() (i.e. this function's caller) and never calling the
this function if it's not version 1.  The compiler didn't figure out
that when we set "entry->esre.esre1 = esre;", esre can not be null
because the function wouldn't be called.  Adding the explicit check
on the version here silenced the warning about entry plausibly being
NULL.

I'm guessing that this is because it's checking that the same
conditional test is involved - that the initialization is in the same
"...version == 1" test that the usage is.  But that's just a guess.

Would you like another patch to add this email to the commit message, or
do you want to add it in your tree, or what?

-- 
        Peter

Re: [GIT PULL] EFI changes for v4.2

[Ingo Molnar]

* Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, Jun 02, 2015 at 08:45:57AM +0200, Ingo Molnar wrote:
> > @@ -167,7 +167,6 @@ static struct kset *esrt_kset;
> >  
> >  static int esre_create_sysfs_entry(void *esre, int entry_num)
> >  {
> > -       int rc = 0;
> >         struct esre_entry *entry;
> >         char name[20];
> >  
> > @@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
> >         entry->kobj.kset = esrt_kset;
> >  
> >         if (esrt->fw_resource_version == 1) {
> > +               int rc = 0;
> > +
> >                 entry->esre.esre1 = esre;
> >                 rc = kobject_init_and_add(&entry->kobj, &esre1_ktype, NULL,
> >                                           "%s", name);
> > -       }
> > -       if (rc) {
> > -               kfree(entry);
> > -               return rc;
> > +               if (rc) {
> > +                       kfree(entry);
> > +                       return rc;
> > +               }
> >         }
> >  
> >         list_add_tail(&entry->list, &entry_list);
> > 
> > How can a compiler ever have warned about 'rc' being uninitialized? It's defined 
> > straight at function entry, with initialization to 0. It can never be 
> > uninitialized.
> >
> > I pulled it, because I agree with the change itself, as it's always better to 
> > define and use variables in the narrowest scope possible, but I think it's a 
> > cleanup, not a compiler warning fix.
> 
> Well, apparently I failed to explain it well - the warning was about
> "esre" rather than "rc".  Basically before we were testing the version in
> register_entries() (i.e. this function's caller) and never calling the
> this function if it's not version 1.  The compiler didn't figure out
> that when we set "entry->esre.esre1 = esre;", esre can not be null
> because the function wouldn't be called.  Adding the explicit check
> on the version here silenced the warning about entry plausibly being
> NULL.
> 
> I'm guessing that this is because it's checking that the same
> conditional test is involved - that the initialization is in the same
> "...version == 1" test that the usage is.  But that's just a guess.
> 
> Would you like another patch to add this email to the commit message, or do you 
> want to add it in your tree, or what?

No need, I already pulled, because the changes themselves seemed OK - just wanted 
to ask in case there's something subtle going on.

Thanks,

	Ingo