Shorten constant names for EFI variable attributes
Subsystems (2)
PATCH 1/1 Shorten constant names for EFI variable attributes
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
--- drivers/firmware/efivars.c | 20 ++++++++------------ drivers/firmware/google/gsmi.c | 8 ++------ include/linux/efi.h | 35 ++++++++++++++++++++--------------- 3 files changed, 30 insertions(+), 33 deletions(-)
drivers/firmware/efivars.c +8 -12
@@ -122,10 +122,7 @@ struct efivar_attribute {ssize_t (*store)(struct efivar_entry *entry,const char *buf,size_t count); }; -#define PSTORE_EFI_ATTRIBUTES \ - (EFI_VARIABLE_NON_VOLATILE | \ - EFI_VARIABLE_BOOTSERVICE_ACCESS | \ - EFI_VARIABLE_RUNTIME_ACCESS) +#define PSTORE_EFI_ATTRIBUTES (EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME)#define EFIVAR_ATTR (_name, _mode, _show, _store) \ struct efivar_attribute efivar_attr_##_name = { \
@@ -435,22 +432,21 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)if (status !=EFI_SUCCESS )return -EIO ; -if (var->Attributes &EFI_VARIABLE_NON_VOLATILE ) +if (var->Attributes &EFI_VAR_NV ) str +=sprintf (str,"EFI_VARIABLE_NON_VOLATILE\n" ); -if (var->Attributes &EFI_VARIABLE_BOOTSERVICE_ACCESS ) +if (var->Attributes &EFI_VAR_BOOT ) str +=sprintf (str,"EFI_VARIABLE_BOOTSERVICE_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_RUNTIME_ACCESS ) +if (var->Attributes &EFI_VAR_RUNTIME ) str +=sprintf (str,"EFI_VARIABLE_RUNTIME_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_HARDWARE_ERROR_RECORD ) +if (var->Attributes &EFI_VAR_HW_ERR ) str +=sprintf (str,"EFI_VARIABLE_HARDWARE_ERROR_RECORD\n" ); -if (var->Attributes &EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS ) +if (var->Attributes &EFI_VAR_AUTH_WRITE ) str +=sprintf (str,"EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS\n" ); -if (var->Attributes & -EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS ) +if (var->Attributes &EFI_VAR_TIMED_AUTH_WRITE ) str +=sprintf (str,"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_APPEND_WRITE ) +if (var->Attributes &EFI_VAR_APPEND ) str +=sprintf (str,"EFI_VARIABLE_APPEND_WRITE\n" );return str - buf; }
drivers/firmware/google/gsmi.c +2 -6
@@ -353,9 +353,7 @@ static efi_status_t gsmi_get_variable(efi_char16_t *name,memcpy (data, gsmi_dev.data_buf->start, *data_size);/* All variables are have the following attributes */ - *attr =EFI_VARIABLE_NON_VOLATILE | -EFI_VARIABLE_BOOTSERVICE_ACCESS | -EFI_VARIABLE_RUNTIME_ACCESS ; + *attr =EFI_VAR_NV |EFI_VAR_BOOT |EFI_VAR_RUNTIME ; }spin_unlock_irqrestore (&gsmi_dev.lock, flags);
@@ -430,9 +428,7 @@ static efi_status_t gsmi_set_variable(efi_char16_t *name, .name_ptr = gsmi_dev.name_buf->address, .data_ptr = gsmi_dev.data_buf->address, .data_len = (u32)data_size, - .attributes =EFI_VARIABLE_NON_VOLATILE | -EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, + .attributes = EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME, };size_t name_len =utf16_strlen (name,GSMI_BUF_SIZE /2 );efi_status_t ret =EFI_SUCCESS ;
include/linux/efi.h +20 -15
@@ -551,21 +551,26 @@ extern int __init efi_setup_pcdp_console(char *);/* * Variable Attributes */ -#define EFI_VARIABLE_NON_VOLATILE 0x0000000000000001 -#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002 -#define EFI_VARIABLE_RUNTIME_ACCESS 0x0000000000000004 -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x0000000000000008 -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x0000000000000010 -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x0000000000000020 -#define EFI_VARIABLE_APPEND_WRITE 0x0000000000000040 - -#define EFI_VARIABLE_MASK (EFI_VARIABLE_NON_VOLATILE | \ - EFI_VARIABLE_BOOTSERVICE_ACCESS | \ - EFI_VARIABLE_RUNTIME_ACCESS | \ - EFI_VARIABLE_HARDWARE_ERROR_RECORD | \ - EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \ - EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \ - EFI_VARIABLE_APPEND_WRITE) +#define EFI_VAR_NV 0x0000000000000001 +#define EFI_VAR_BOOT 0x0000000000000002 +#define EFI_VAR_RUNTIME 0x0000000000000004 +#define EFI_VAR_HW_ERR 0x0000000000000008 +#define EFI_VAR_AUTH_WRITE 0x0000000000000010 +#define EFI_VAR_TIMED_AUTH_WRITE 0x0000000000000020 +#define EFI_VAR_APPEND 0x0000000000000040 + +#define EFI_VARIABLE_NON_VOLATILE EFI_VAR_NV +#define EFI_VARIABLE_BOOTSERVICE_ACCESS EFI_VAR_BOOT +#define EFI_VARIABLE_RUNTIME_ACCESS EFI_VAR_RUNTIME +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD EFI_VAR_HW_ERR +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS EFI_VAR_AUTH_WRITE +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS \ + EFI_VAR_TIMED_AUTH_WRITE +#define EFI_VARIABLE_APPEND_WRITE EFI_VAR_APPEND + +#define EFI_VARIABLE_MASK (EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME | \ + EFI_VAR_HW_ERR | EFI_VAR_AUTH_WRITE | \ + EFI_VAR_TIMED_AUTH_WRITE | EFI_VAR_APPEND)/* * The type of search to perform when calling boottime->locate_handle */
Signature · 7 lines
1.7.9.5 -- =========== Khalid Aziz [email protected]
Quoted 6 lines · Khalid Aziz
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
I think these some from the EFI specifcation, so NAK IMO.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Quoted 8 lines · H. Peter Anvin
Quoted 6 lines · Khalid Aziz
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
Quoted 4 lines
I think these some from the EFI specifcation, so NAK IMO.
-hpa
This patch is based upon earlier discussion at <https://lkml.org/lkml/2012/7/13/320>.
You are right that EFI specification uses exactly these long names for the constants, but does that mean kernel must also use the exact same long constant names? I can see doing that for the sake of consistency. At the same time, can we make the kernel code more readable and retain compatibility with existing API by using aliases? I slightly prefer making kernel code more readable, but I could go either way.
Signature · 2 lines
Khalid Aziz [email protected]
Quoted 16 lines · Khalid Aziz
Quoted 8 lines · H. Peter Anvin
Quoted 6 lines · Khalid Aziz
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
Quoted 4 lines
I think these some from the EFI specifcation, so NAK IMO.
-hpa
This patch is based upon earlier discussion at
<https://lkml.org/lkml/2012/7/13/320>.
Quoted 7 lines
You are right that EFI specification uses exactly these long names for the constants, but does that mean kernel must also use the exact same long constant names? I can see doing that for the sake of consistency. At the same time, can we make the kernel code more readable and retain compatibility with existing API by using aliases? I slightly prefer making kernel code more readable, but I could go either way.
I think it makes the kernel code less readable, because now you not only need to understand the kernel code and the EFI spec, but also how the two maps onto each other. The fact that you then have to introduce aliases indicates to me that you're doing something actively broken.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Quoted 13 lines · H. Peter Anvin
Quoted 11 lines · Khalid Aziz
This patch is based upon earlier discussion at
<https://lkml.org/lkml/2012/7/13/320>.You are right that EFI specification uses exactly these long names for the constants, but does that mean kernel must also use the exact same long constant names? I can see doing that for the sake of consistency. At the same time, can we make the kernel code more readable and retain compatibility with existing API by using aliases? I slightly prefer making kernel code more readable, but I could go either way.
Quoted 4 lines
I think it makes the kernel code less readable, because now you not only need to understand the kernel code and the EFI spec, but also how the two maps onto each other. The fact that you then have to introduce aliases indicates to me that you're doing something actively broken.
-hpa
As I think more about it, existence of aliases could also potentially create confusion where someone adding new code to kernel chooses to use the long name instead. Maybe unless we can make a clean break from long names, it is not worth doing this and that is going to be problematic because of the existing usage in userspace programs.
Matthew, do you have a different point of view?
Signature · 2 lines
Khalid [email protected]
Quoted 11 lines · H. Peter Anvin
Quoted 6 lines · Khalid Aziz
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
I think these some from the EFI specifcation, so NAK IMO.
From the point of view of making efivars more readable, I'd certainly
prefer shorter constant names. Keeping an alias is necessary only because it's an existing exposed interface. The specification doesn't actually require the use of these specific names anywhere, and we've taken a more relaxed attitude in other bits of the EFI code.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 8 lines · Matthew Garrett
Quoted 7 lines · H. Peter Anvin
Quoted 6 lines · Khalid Aziz
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs. This patch depends on patch <https://lkml.org/lkml/2012/7/13/313>.
Quoted 7 lines
Quoted 1 line
I think these some from the EFI specifcation, so NAK IMO.
From the point of view of making efivars more readable, I'd certainly
prefer shorter constant names. Keeping an alias is necessary only because it's an existing exposed interface. The specification doesn't actually require the use of these specific names anywhere, and we've taken a more relaxed attitude in other bits of the EFI code.
Matthew,
I also do not believe that kernel must use the constant names mentioned in the specification especially when the name reaches 50 characters. We can not get away from having to create aliases. Do you think having aliases in efi.h can cause mixed use of long names and short names in future code in the kernel? Can we address this by suggesting to future code authors that they should use the short names in their code? Should we consider inclusion of this patch in the kernel?
Signature · 2 lines
Khalid Aziz [email protected]
Quoted 8 lines · Khalid Aziz
I also do not believe that kernel must use the constant names mentioned in the specification especially when the name reaches 50 characters. We can not get away from having to create aliases. Do you think having aliases in
efi.hcan cause mixed use of long names and short names in future code in the kernel? Can we address this by suggesting to future code authors that they should use the short names in their code? Should we consider inclusion of this patch in the kernel?
I'd be surprised if it were a problem - we should catch any of those cases in code review, or gate the aliases under #ifndef __KERNEL__
Signature · 1 line
Matthew Garrett | [email protected]
PATCH v2 1/1 Shorten constant names for EFI variable attributes
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs.
---
Change for v2: Made only the new constant names available for use in kernel by
adding #ifndef __KERNEL__ around aliases for old constant names
drivers/firmware/efivars.c | 20 ++++++++------------ drivers/firmware/google/gsmi.c | 8 ++------ include/linux/efi.h | 37 ++++++++++++++++++++++--------------- 3 files changed, 32 insertions(+), 33 deletions(-)
drivers/firmware/efivars.c +8 -12
@@ -122,10 +122,7 @@ struct efivar_attribute {ssize_t (*store)(struct efivar_entry *entry,const char *buf,size_t count); }; -#define PSTORE_EFI_ATTRIBUTES \ - (EFI_VARIABLE_NON_VOLATILE | \ - EFI_VARIABLE_BOOTSERVICE_ACCESS | \ - EFI_VARIABLE_RUNTIME_ACCESS) +#define PSTORE_EFI_ATTRIBUTES (EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME)#define EFIVAR_ATTR (_name, _mode, _show, _store) \ struct efivar_attribute efivar_attr_##_name = { \
@@ -435,22 +432,21 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)if (status !=EFI_SUCCESS )return -EIO ; -if (var->Attributes &EFI_VARIABLE_NON_VOLATILE ) +if (var->Attributes &EFI_VAR_NV ) str +=sprintf (str,"EFI_VARIABLE_NON_VOLATILE\n" ); -if (var->Attributes &EFI_VARIABLE_BOOTSERVICE_ACCESS ) +if (var->Attributes &EFI_VAR_BOOT ) str +=sprintf (str,"EFI_VARIABLE_BOOTSERVICE_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_RUNTIME_ACCESS ) +if (var->Attributes &EFI_VAR_RUNTIME ) str +=sprintf (str,"EFI_VARIABLE_RUNTIME_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_HARDWARE_ERROR_RECORD ) +if (var->Attributes &EFI_VAR_HW_ERR ) str +=sprintf (str,"EFI_VARIABLE_HARDWARE_ERROR_RECORD\n" ); -if (var->Attributes &EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS ) +if (var->Attributes &EFI_VAR_AUTH_WRITE ) str +=sprintf (str,"EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS\n" ); -if (var->Attributes & -EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS ) +if (var->Attributes &EFI_VAR_TIMED_AUTH_WRITE ) str +=sprintf (str,"EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS\n" ); -if (var->Attributes &EFI_VARIABLE_APPEND_WRITE ) +if (var->Attributes &EFI_VAR_APPEND ) str +=sprintf (str,"EFI_VARIABLE_APPEND_WRITE\n" );return str - buf; }
drivers/firmware/google/gsmi.c +2 -6
@@ -353,9 +353,7 @@ static efi_status_t gsmi_get_variable(efi_char16_t *name,memcpy (data, gsmi_dev.data_buf->start, *data_size);/* All variables are have the following attributes */ - *attr =EFI_VARIABLE_NON_VOLATILE | -EFI_VARIABLE_BOOTSERVICE_ACCESS | -EFI_VARIABLE_RUNTIME_ACCESS ; + *attr =EFI_VAR_NV |EFI_VAR_BOOT |EFI_VAR_RUNTIME ; }spin_unlock_irqrestore (&gsmi_dev.lock, flags);
@@ -430,9 +428,7 @@ static efi_status_t gsmi_set_variable(efi_char16_t *name, .name_ptr = gsmi_dev.name_buf->address, .data_ptr = gsmi_dev.data_buf->address, .data_len = (u32)data_size, - .attributes =EFI_VARIABLE_NON_VOLATILE | -EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, + .attributes = EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME, };size_t name_len =utf16_strlen (name,GSMI_BUF_SIZE /2 );efi_status_t ret =EFI_SUCCESS ;
include/linux/efi.h +22 -15
@@ -551,21 +551,28 @@ extern int __init efi_setup_pcdp_console(char *);/* * Variable Attributes */ -#define EFI_VARIABLE_NON_VOLATILE 0x0000000000000001 -#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002 -#define EFI_VARIABLE_RUNTIME_ACCESS 0x0000000000000004 -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x0000000000000008 -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x0000000000000010 -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x0000000000000020 -#define EFI_VARIABLE_APPEND_WRITE 0x0000000000000040 - -#define EFI_VARIABLE_MASK (EFI_VARIABLE_NON_VOLATILE | \ - EFI_VARIABLE_BOOTSERVICE_ACCESS | \ - EFI_VARIABLE_RUNTIME_ACCESS | \ - EFI_VARIABLE_HARDWARE_ERROR_RECORD | \ - EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \ - EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \ - EFI_VARIABLE_APPEND_WRITE) +#define EFI_VAR_NV 0x0000000000000001 +#define EFI_VAR_BOOT 0x0000000000000002 +#define EFI_VAR_RUNTIME 0x0000000000000004 +#define EFI_VAR_HW_ERR 0x0000000000000008 +#define EFI_VAR_AUTH_WRITE 0x0000000000000010 +#define EFI_VAR_TIMED_AUTH_WRITE 0x0000000000000020 +#define EFI_VAR_APPEND 0x0000000000000040 + +#ifndef __KERNEL__ +#define EFI_VARIABLE_NON_VOLATILE EFI_VAR_NV +#define EFI_VARIABLE_BOOTSERVICE_ACCESS EFI_VAR_BOOT +#define EFI_VARIABLE_RUNTIME_ACCESS EFI_VAR_RUNTIME +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD EFI_VAR_HW_ERR +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS EFI_VAR_AUTH_WRITE +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS \ + EFI_VAR_TIMED_AUTH_WRITE +#define EFI_VARIABLE_APPEND_WRITE EFI_VAR_APPEND +#endif + +#define EFI_VARIABLE_MASK (EFI_VAR_NV | EFI_VAR_BOOT | EFI_VAR_RUNTIME | \ + EFI_VAR_HW_ERR | EFI_VAR_AUTH_WRITE | \ + EFI_VAR_TIMED_AUTH_WRITE | EFI_VAR_APPEND)/* * The type of search to perform when calling boottime->locate_handle */
Signature · 1 line
1.7.8.3
Hi,
Quoted 6 lines · On Tue, 25 Sep 2012 09:41:00 -0600 Khalid Aziz <[email protected]> wrote:
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs.
Why do this? It just looks like churn for no real gain.
Signature · 2 lines
Cheers, Stephen Rothwell [email protected]
Quoted 4 lines · Stephen Rothwell
Hi,
Quoted 1 line · On Tue, 25 Sep 2012 09:41:00 -0600 Khalid Aziz <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
Replace very long constants for EFI variable attributes with shorter and more convenient names. Also create an alias for the current longer names so as to not break compatibility with current API since these constants are used by userspace programs.
Why do this? It just looks like churn for no real gain.
This is for code cleanup and does not impact functionality. This is based upon an earlier discussion -
<https://lkml.org/lkml/2012/7/13/320>. The goal is to make the code more readable. V1 of this patch was discussed at
<https://lkml.org/lkml/2012/7/20/414>.
Signature · 1 line
Khalid Aziz <[email protected]>
Quoted 5 lines · Khalid Aziz
This is for code cleanup and does not impact functionality. This is based upon an earlier discussion -
<https://lkml.org/lkml/2012/7/13/320>. The goal is to make the code more readable. V1 of this patch was discussed at
<https://lkml.org/lkml/2012/7/20/414>.
Right. Keeping the spec names makes it difficult to write code in a readable way.
Signature · 1 line
Matthew Garrett | [email protected]