Andrew,
I broke up the efivars driver update patch I had sent out
quite a while ago into several smaller patches. This
includes several fixes and suggestions that were pointed
out. The patches are broken down as follows:
1 - remove all traces of efivars from arch/ia64/
2 - add new sysfs based efivars driver into
drivers/firmware with accompanying Kconfig/Makefile
changes to make it fully functional for ia64 again.
3 - cleans up x86 references to the /proc version of
the efivars driver.
The patch is against 2.6.6-rc2 and has been tested on a
Tiger4 system and an x86 system with EFI. The second and
third patches will follow. Please apply.
thanks,
matt
diff -urN linux-2.6.6-rc2/arch/ia64/Kconfig linux-2.6.6-rc2-mod/arch/ia64/Kconfig
--- linux-2.6.6-rc2/arch/ia64/Kconfig 2004-04-20 14:30:12.000000000 -0700
+++ linux-2.6.6-rc2-mod/arch/ia64/Kconfig 2004-04-20 23:50:48.389320376 -0700
@@ -294,16 +294,6 @@
To use this option, you have to ensure that the "/proc file system
support" (CONFIG_PROC_FS) is enabled, too.
-config EFI_VARS
- tristate "/proc/efi/vars support"
- help
- If you say Y here, you are able to get EFI (Extensible Firmware
- Interface) variable information in /proc/efi/vars. You may read,
- write, create, and destroy EFI variables through this interface.
-
- To use this option, you have to check that the "/proc file system
- support" (CONFIG_PROC_FS) is enabled, too.
-
source "fs/Kconfig.binfmt"
endmenu
diff -urN linux-2.6.6-rc2/arch/ia64/kernel/efi.c linux-2.6.6-rc2-mod/arch/ia64/kernel/efi.c
--- linux-2.6.6-rc2/arch/ia64/kernel/efi.c 2004-04-20 14:28:22.000000000 -0700
+++ linux-2.6.6-rc2-mod/arch/ia64/kernel/efi.c 2004-04-20 23:24:15.000000000 -0700
@@ -24,7 +24,6 @@
#include <linux/init.h>
#include <linux/types.h>
#include <linux/time.h>
-#include <linux/proc_fs.h>
#include <linux/efi.h>
#include <asm/io.h>
@@ -40,19 +39,6 @@
struct efi efi;
EXPORT_SYMBOL(efi);
static efi_runtime_services_t *runtime;
-
-/*
- * efi_dir is allocated here, but the directory isn't created
- * here, as proc_mkdir() doesn't work this early in the bootup
- * process. Therefore, each module, like efivars, must test for
- * if (!efi_dir) efi_dir = proc_mkdir("efi", NULL);
- * prior to creating their own entries under /proc/efi.
- */
-#ifdef CONFIG_PROC_FS
-struct proc_dir_entry *efi_dir;
-EXPORT_SYMBOL(efi_dir);
-#endif
-
static unsigned long mem_limit = ~0UL;
#define efi_call_virt(f, args...) (*(f))(args)
@@ -747,10 +733,3 @@
return 0;
}
-static void __exit
-efivars_exit (void)
-{
-#ifdef CONFIG_PROC_FS
- remove_proc_entry(efi_dir->name, NULL);
-#endif
-}
diff -urN linux-2.6.6-rc2/arch/ia64/kernel/efivars.c linux-2.6.6-rc2-mod/arch/ia64/kernel/efivars.c
--- linux-2.6.6-rc2/arch/ia64/kernel/efivars.c 2004-04-20 14:29:08.000000000 -0700
+++ linux-2.6.6-rc2-mod/arch/ia64/kernel/efivars.c 1969-12-31 16:00:00.000000000 -0800
@@ -1,549 +0,0 @@
-/*
- * EFI Variables - efivars.c
- *
- * Copyright (C) 2001 Dell Computer Corporation <[email protected]>
- *
- * This code takes all variables accessible from EFI runtime and
- * exports them via /proc
- *
- * Reads to /proc/efi/vars/varname return an efi_variable_t structure.
- * Writes to /proc/efi/vars/varname must be an efi_variable_t structure.
- * Writes with DataSize = 0 or Attributes = 0 deletes the variable.
- * Writes with a new value in VariableName+VendorGuid creates
- * a new variable.
- *
- *
- * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- *
- * Changelog:
- *
- * 10 Feb 2004 - Stephane Eranian <[email protected]>
- * Provide FPSWA version number via /proc/efi/fpswa
- *
- * 10 Dec 2002 - Matt Domsch <[email protected]>
- * fix locking per Peter Chubb's findings
- *
- * 25 Mar 2002 - Matt Domsch <[email protected]>
- * move uuid_unparse() to include/asm-ia64/efi.h:efi_guid_unparse()
- *
- * 12 Feb 2002 - Matt Domsch <[email protected]>
- * use list_for_each_safe when deleting vars.
- * remove ifdef CONFIG_SMP around include <linux/smp.h>
- * v0.04 release to [email protected]
- *
- * 20 April 2001 - Matt Domsch <[email protected]>
- * Moved vars from /proc/efi to /proc/efi/vars, and made
- * efi.c own the /proc/efi directory.
- * v0.03 release to [email protected]
- *
- * 26 March 2001 - Matt Domsch <[email protected]>
- * At the request of Stephane, moved ownership of /proc/efi
- * to efi.c, and now efivars lives under /proc/efi/vars.
- *
- * 12 March 2001 - Matt Domsch <[email protected]>
- * Feedback received from Stephane Eranian incorporated.
- * efivar_write() checks copy_from_user() return value.
- * efivar_read/write() returns proper errno.
- * v0.02 release to [email protected]
- *
- * 26 February 2001 - Matt Domsch <[email protected]>
- * v0.01 release to [email protected]
- */
-
-#include <linux/config.h>
-#include <linux/types.h>
-#include <linux/errno.h>
-#include <linux/init.h>
-#include <linux/proc_fs.h>
-#include <linux/sched.h> /* for capable() */
-#include <linux/mm.h>
-#include <linux/module.h>
-#include <linux/smp.h>
-#include <linux/efi.h>
-
-#include <asm/fpswa.h>
-#include <asm/uaccess.h>
-
-MODULE_AUTHOR("Matt Domsch <[email protected]>");
-MODULE_DESCRIPTION("/proc interface to EFI Variables");
-MODULE_LICENSE("GPL");
-
-#define EFIVARS_VERSION "0.06 2002-Dec-10"
-
-static int
-efivar_read(char *page, char **start, off_t off,
- int count, int *eof, void *data);
-static int
-efivar_write(struct file *file, const char *buffer,
- unsigned long count, void *data);
-
-
-/*
- * The maximum size of VariableName + Data = 1024
- * Therefore, it's reasonable to save that much
- * space in each part of the structure,
- * and we use a page for reading/writing.
- */
-
-typedef struct _efi_variable_t {
- efi_char16_t VariableName[1024/sizeof(efi_char16_t)];
- efi_guid_t VendorGuid;
- unsigned long DataSize;
- __u8 Data[1024];
- efi_status_t Status;
- __u32 Attributes;
-} __attribute__((packed)) efi_variable_t;
-
-
-typedef struct _efivar_entry_t {
- efi_variable_t var;
- struct proc_dir_entry *entry;
- struct list_head list;
-} efivar_entry_t;
-
-/*
- efivars_lock protects two things:
- 1) efivar_list - adds, removals, reads, writes
- 2) efi.[gs]et_variable() calls.
- It must not be held when creating proc entries or calling kmalloc.
- efi.get_next_variable() is only called from efivars_init(),
- which is protected by the BKL, so that path is safe.
-*/
-static spinlock_t efivars_lock = SPIN_LOCK_UNLOCKED;
-static LIST_HEAD(efivar_list);
-static struct proc_dir_entry *efi_vars_dir;
-
-#define efivar_entry(n) list_entry(n, efivar_entry_t, list)
-
-/* Return the number of unicode characters in data */
-static unsigned long
-utf8_strlen(efi_char16_t *data, unsigned long maxlength)
-{
- unsigned long length = 0;
- while (*data++ != 0 && length < maxlength)
- length++;
- return length;
-}
-
-/* Return the number of bytes is the length of this string */
-/* Note: this is NOT the same as the number of unicode characters */
-static inline unsigned long
-utf8_strsize(efi_char16_t *data, unsigned long maxlength)
-{
- return utf8_strlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
-}
-
-
-static int
-proc_calc_metrics(char *page, char **start, off_t off,
- int count, int *eof, int len)
-{
- if (len <= off+count) *eof = 1;
- *start = page + off;
- len -= off;
- if (len>count) len = count;
- if (len<0) len = 0;
- return len;
-}
-
-/*
- * efivar_create_proc_entry()
- * Requires:
- * variable_name_size = number of bytes required to hold
- * variable_name (not counting the NULL
- * character at the end.
- * efivars_lock is not held on entry or exit.
- * Returns 1 on failure, 0 on success
- */
-static int
-efivar_create_proc_entry(unsigned long variable_name_size,
- efi_char16_t *variable_name,
- efi_guid_t *vendor_guid)
-{
- int i, short_name_size = variable_name_size / sizeof(efi_char16_t) + 38;
- char *short_name;
- efivar_entry_t *new_efivar;
-
- short_name = kmalloc(short_name_size+1, GFP_KERNEL);
- new_efivar = kmalloc(sizeof(efivar_entry_t), GFP_KERNEL);
-
- if (!short_name || !new_efivar) {
- if (short_name) kfree(short_name);
- if (new_efivar) kfree(new_efivar);
- return 1;
- }
- memset(short_name, 0, short_name_size+1);
- memset(new_efivar, 0, sizeof(efivar_entry_t));
-
- memcpy(new_efivar->var.VariableName, variable_name,
- variable_name_size);
- memcpy(&(new_efivar->var.VendorGuid), vendor_guid, sizeof(efi_guid_t));
-
- /* Convert Unicode to normal chars (assume top bits are 0),
- ala UTF-8 */
- for (i=0; i< (int) (variable_name_size / sizeof(efi_char16_t)); i++) {
- short_name[i] = variable_name[i] & 0xFF;
- }
-
- /* This is ugly, but necessary to separate one vendor's
- private variables from another's. */
-
- *(short_name + strlen(short_name)) = '-';
- efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
-
- /* Create the entry in proc */
- new_efivar->entry = create_proc_entry(short_name, 0600, efi_vars_dir);
- kfree(short_name); short_name = NULL;
- if (!new_efivar->entry) return 1;
-
- new_efivar->entry->owner = THIS_MODULE;
- new_efivar->entry->data = new_efivar;
- new_efivar->entry->read_proc = efivar_read;
- new_efivar->entry->write_proc = efivar_write;
-
- spin_lock(&efivars_lock);
- list_add(&new_efivar->list, &efivar_list);
- spin_unlock(&efivars_lock);
-
- return 0;
-}
-
-
-
-/***********************************************************
- * efivar_read()
- * Requires:
- * Modifies: page
- * Returns: number of bytes written, or -EINVAL on failure
- ***********************************************************/
-
-static int
-efivar_read(char *page, char **start, off_t off, int count, int *eof, void *data)
-{
- int len = sizeof(efi_variable_t);
- efivar_entry_t *efi_var = data;
- efi_variable_t *var_data = (efi_variable_t *)page;
-
- if (!page || !data) return -EINVAL;
-
- spin_lock(&efivars_lock);
-
- memcpy(var_data, &efi_var->var, len);
-
- var_data->DataSize = 1024;
- var_data->Status = efi.get_variable(var_data->VariableName,
- &var_data->VendorGuid,
- &var_data->Attributes,
- &var_data->DataSize,
- var_data->Data);
-
- spin_unlock(&efivars_lock);
-
- return proc_calc_metrics(page, start, off, count, eof, len);
-}
-
-/***********************************************************
- * efivar_write()
- * Requires: data is an efi_setvariable_t data type,
- * properly filled in, possibly by a call
- * first to efivar_read().
- * Caller must have CAP_SYS_ADMIN
- * Modifies: NVRAM
- * Returns: var_data->DataSize on success, errno on failure
- *
- ***********************************************************/
-static int
-efivar_write(struct file *file, const char *buffer,
- unsigned long count, void *data)
-{
- unsigned long strsize1, strsize2;
- int found=0;
- struct list_head *pos, *n;
- unsigned long size = sizeof(efi_variable_t);
- efi_status_t status;
- efivar_entry_t *efivar = data, *search_efivar = NULL;
- efi_variable_t *var_data;
- if (!data || count != size) {
- printk(KERN_WARNING "efivars: improper struct of size 0x%lx passed.\n", count);
- return -EINVAL;
- }
- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
-
- var_data = kmalloc(size, GFP_KERNEL);
- if (!var_data)
- return -ENOMEM;
- if (copy_from_user(var_data, buffer, size)) {
- kfree(var_data);
- return -EFAULT;
- }
-
- spin_lock(&efivars_lock);
-
- /* Since the data ptr we've currently got is probably for
- a different variable find the right variable.
- This allows any properly formatted data structure to
- be written to any of the files in /proc/efi/vars and it will work.
- */
- list_for_each_safe(pos, n, &efivar_list) {
- search_efivar = efivar_entry(pos);
- strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
- strsize2 = utf8_strsize(var_data->VariableName, 1024);
- if ( strsize1 == strsize2 &&
- !memcmp(&(search_efivar->var.VariableName),
- var_data->VariableName, strsize1) &&
- !efi_guidcmp(search_efivar->var.VendorGuid,
- var_data->VendorGuid)) {
- found = 1;
- break;
- }
- }
- if (found) efivar = search_efivar;
-
- status = efi.set_variable(var_data->VariableName,
- &var_data->VendorGuid,
- var_data->Attributes,
- var_data->DataSize,
- var_data->Data);
-
- if (status != EFI_SUCCESS) {
- printk(KERN_WARNING "set_variable() failed: status=%lx\n", status);
- kfree(var_data);
- spin_unlock(&efivars_lock);
- return -EIO;
- }
-
-
- if (!var_data->DataSize || !var_data->Attributes) {
- /* We just deleted the NVRAM variable */
- remove_proc_entry(efivar->entry->name, efi_vars_dir);
- list_del(&efivar->list);
- kfree(efivar);
- }
-
- spin_unlock(&efivars_lock);
-
- /* If this is a new variable, set up the proc entry for it. */
- if (!found) {
- efivar_create_proc_entry(utf8_strsize(var_data->VariableName,
- 1024),
- var_data->VariableName,
- &var_data->VendorGuid);
- }
-
- kfree(var_data);
- return size;
-}
-
-/*
- * The EFI system table contains pointers to the SAL system table,
- * HCDP, ACPI, SMBIOS, etc, that may be useful to applications.
- */
-static ssize_t
-efi_systab_read(struct file *file, char *buffer, size_t count, loff_t *ppos)
-{
- void *data;
- u8 *proc_buffer;
- ssize_t size, length;
- int ret;
- const int max_nr_entries = 7; /* num ptrs to tables we could expose */
- const int max_line_len = 80;
-
- if (!efi.systab)
- return 0;
-
- proc_buffer = kmalloc(max_nr_entries * max_line_len, GFP_KERNEL);
- if (!proc_buffer)
- return -ENOMEM;
-
- length = 0;
- if (efi.mps)
- length += sprintf(proc_buffer + length, "MPS=0x%lx\n", __pa(efi.mps));
- if (efi.acpi20)
- length += sprintf(proc_buffer + length, "ACPI20=0x%lx\n", __pa(efi.acpi20));
- if (efi.acpi)
- length += sprintf(proc_buffer + length, "ACPI=0x%lx\n", __pa(efi.acpi));
- if (efi.smbios)
- length += sprintf(proc_buffer + length, "SMBIOS=0x%lx\n", __pa(efi.smbios));
- if (efi.sal_systab)
- length += sprintf(proc_buffer + length, "SAL=0x%lx\n", __pa(efi.sal_systab));
- if (efi.hcdp)
- length += sprintf(proc_buffer + length, "HCDP=0x%lx\n", __pa(efi.hcdp));
- if (efi.boot_info)
- length += sprintf(proc_buffer + length, "BOOTINFO=0x%lx\n", __pa(efi.boot_info));
-
- if (*ppos >= length) {
- ret = 0;
- goto out;
- }
-
- data = proc_buffer + file->f_pos;
- size = length - file->f_pos;
- if (size > count)
- size = count;
- if (copy_to_user(buffer, data, size)) {
- ret = -EFAULT;
- goto out;
- }
-
- *ppos += size;
- ret = size;
-
-out:
- kfree(proc_buffer);
- return ret;
-}
-
-static struct proc_dir_entry *efi_systab_entry;
-static struct file_operations efi_systab_fops = {
- .read = efi_systab_read,
-};
-
-static ssize_t
-efi_fpswa_read (struct file *file, char *buffer, size_t count, loff_t *ppos)
-{
- ssize_t size, length;
- char str[32];
- void *data;
-
- snprintf(str, sizeof(str), "revision=%u.%u\n",
- fpswa_interface->revision >> 16, fpswa_interface->revision & 0xffff);
-
- length = strlen(str);
-
- if (*ppos >= length)
- return 0;
-
- data = str + file->f_pos;
- size = length - file->f_pos;
- if (size > count)
- size = count;
- if (copy_to_user(buffer, data, size))
- return -EFAULT;
-
- *ppos += size;
- return size;
-}
-
-static struct proc_dir_entry *efi_fpswa_entry;
-static struct file_operations efi_fpswa_fops = {
- .read = efi_fpswa_read,
-};
-
-static int __init
-efivars_init(void)
-{
- efi_status_t status;
- efi_guid_t vendor_guid;
- efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
- unsigned long variable_name_size = 1024;
-
- printk(KERN_INFO "EFI Variables Facility v%s\n", EFIVARS_VERSION);
-
- /* Since efi.c happens before procfs is available,
- we create the directory here if it doesn't
- already exist. There's probably a better way
- to do this.
- */
- if (!efi_dir)
- efi_dir = proc_mkdir("efi", NULL);
-
- efi_systab_entry = create_proc_entry("systab", S_IRUSR | S_IRGRP, efi_dir);
- if (efi_systab_entry)
- efi_systab_entry->proc_fops = &efi_systab_fops;
-
- if (fpswa_interface) {
- efi_fpswa_entry = create_proc_entry("fpswa", S_IRUGO, efi_dir);
- if (efi_fpswa_entry)
- efi_fpswa_entry->proc_fops = &efi_fpswa_fops;
- }
-
- efi_vars_dir = proc_mkdir("vars", efi_dir);
-
- /* Per EFI spec, the maximum storage allocated for both
- the variable name and variable data is 1024 bytes.
- */
-
- memset(variable_name, 0, 1024);
-
- do {
- variable_name_size=1024;
-
- status = efi.get_next_variable(&variable_name_size,
- variable_name,
- &vendor_guid);
-
-
- switch (status) {
- case EFI_SUCCESS:
- efivar_create_proc_entry(variable_name_size,
- variable_name,
- &vendor_guid);
- break;
- case EFI_NOT_FOUND:
- break;
- default:
- printk(KERN_WARNING "get_next_variable: status=%lx\n", status);
- status = EFI_NOT_FOUND;
- break;
- }
-
- } while (status != EFI_NOT_FOUND);
-
- kfree(variable_name);
- return 0;
-}
-
-static void __exit
-efivars_exit(void)
-{
- struct list_head *pos, *n;
- efivar_entry_t *efivar;
-
- spin_lock(&efivars_lock);
- if (efi_systab_entry)
- remove_proc_entry(efi_systab_entry->name, efi_dir);
- list_for_each_safe(pos, n, &efivar_list) {
- efivar = efivar_entry(pos);
- remove_proc_entry(efivar->entry->name, efi_vars_dir);
- list_del(&efivar->list);
- kfree(efivar);
- }
- spin_unlock(&efivars_lock);
-
- remove_proc_entry(efi_vars_dir->name, efi_dir);
-}
-
-module_init(efivars_init);
-module_exit(efivars_exit);
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only. This must remain at the end
- * of the file.
- * ---------------------------------------------------------------------------
- * Local variables:
- * c-indent-level: 4
- * c-brace-imaginary-offset: 0
- * c-brace-offset: -4
- * c-argdecl-indent: 4
- * c-label-offset: -4
- * c-continued-statement-offset: 4
- * c-continued-brace-offset: 0
- * indent-tabs-mode: nil
- * tab-width: 8
- * End:
- */
diff -urN linux-2.6.6-rc2/arch/ia64/kernel/Makefile linux-2.6.6-rc2-mod/arch/ia64/kernel/Makefile
--- linux-2.6.6-rc2/arch/ia64/kernel/Makefile 2004-04-20 14:30:29.000000000 -0700
+++ linux-2.6.6-rc2-mod/arch/ia64/kernel/Makefile 2004-04-20 23:25:33.000000000 -0700
@@ -8,7 +8,6 @@
irq_lsapic.o ivt.o machvec.o pal.o patch.o process.o perfmon.o ptrace.o sal.o \
salinfo.o semaphore.o setup.o signal.o sys_ia64.o time.o traps.o unaligned.o unwind.o mca.o mca_asm.o
-obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_IA64_BRL_EMU) += brl_emu.o
obj-$(CONFIG_IA64_GENERIC) += acpi-ext.o
obj-$(CONFIG_IA64_HP_ZX1) += acpi-ext.o
diff -urN linux-2.6.6-rc2/arch/ia64/mm/init.c linux-2.6.6-rc2-mod/arch/ia64/mm/init.c
--- linux-2.6.6-rc2/arch/ia64/mm/init.c 2004-04-20 14:29:07.000000000 -0700
+++ linux-2.6.6-rc2-mod/arch/ia64/mm/init.c 2004-04-20 23:27:09.000000000 -0700
@@ -18,6 +18,7 @@
#include <linux/reboot.h>
#include <linux/slab.h>
#include <linux/swap.h>
+#include <linux/proc_fs.h>
#include <asm/a.out.h>
#include <asm/bitops.h>
On Thursday 22 April 2004 11:32 am, Matt Tolentino wrote:
> I broke up the efivars driver update patch I had sent out
> quite a while ago into several smaller patches. This
> includes several fixes and suggestions that were pointed
> out. The patches are broken down as follows:
>
> 1 - remove all traces of efivars from arch/ia64/
> 2 - add new sysfs based efivars driver into
> drivers/firmware with accompanying Kconfig/Makefile
> changes to make it fully functional for ia64 again.
> 3 - cleans up x86 references to the /proc version of
> the efivars driver.
I like these changes.
I did notice that the new drivers/.../efivars.c is not identical to
the old arch/ia64/kernel/efivars.c (the hints to emacs were removed).
I like the emacs hint removal, but didn't review patch for any other
differences.
Any plans to consolidate other bits from efi.c? There are a number
of things there that look like they could be shared:
is_available_memory()
efi_init()
(mostly)
efi_initialize_iomem_resources()
(i386 only today, but I think we'd like it for ia64 also)
efi_mem_type()
efi_mem_attributes()
(the i386 versions look slightly buggy in that they
assume a fixed descriptor size, so old kernels won't
work with new firmware that adds stuff to the descriptor)
Bjorn
> I like these changes.
Thanks!
> I did notice that the new drivers/.../efivars.c is not identical to
> the old arch/ia64/kernel/efivars.c (the hints to emacs were removed).
> I like the emacs hint removal, but didn't review patch for any other
> differences.
Right, it's been fully converted to sysfs. Just for reference, the
resultant sysfs tree looks something like:
/sys |
...
|-firmware
|-efi
|-systab
|-vars
|- BootNext-xxxx-xxx-x-x-x-x *
|-attributes
|-data
|-guid
|-raw_var
|-size
|- BootCurrent-xxxxxx-x-x-x-x *
|- ConOut-xxxx-x-x-x-x-x-x-*
...
|- del_var
|- new_var
where xxxx-x-x-x-x-x is the GUID.
> Any plans to consolidate other bits from efi.c? There are a number
> of things there that look like they could be shared:
Yes, I plan to consolidate as much of the common code as possible soon.
matt
On Thursday 22 April 2004 11:14 am, Tolentino, Matthew E wrote:
> Right, it's been fully converted to sysfs.
Ah, of course. You mentioned that; I just can't read :-)
On Thu, Apr 22, 2004 at 10:32:19AM -0700, Matt Tolentino wrote:
> Andrew,
>
> I broke up the efivars driver update patch I had sent out
> quite a while ago into several smaller patches. This
> includes several fixes and suggestions that were pointed
> out. The patches are broken down as follows:
>
> 2 - add new sysfs based efivars driver into
> drivers/firmware with accompanying Kconfig/Makefile
> changes to make it fully functional for ia64 again.
Trying these against 2.6.5 + ia64 patch, with efibootmgr-0.5.2-test2.
Works: reading, deleting values.
Doesn't work: creating and editing values
I note that efibootmgr prints out a warning when it can't read
nonexistant variables, like BootNext. I'll remove that warning.
It's likely a bug in efibootmgr, as this is the first time I've tried
the sysfs side of things. If you're in a position to help debug, I'd
appreciate it.
Thanks,
Matt
--
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
On Fri, Apr 23, 2004 at 05:29:45PM -0500, Matt Domsch wrote:
> Trying these against 2.6.5 + ia64 patch, with efibootmgr-0.5.2-test2.
>
> Works: reading, deleting values.
> Doesn't work: creating and editing values
>
> I note that efibootmgr prints out a warning when it can't read
> nonexistant variables, like BootNext. I'll remove that warning.
>
> It's likely a bug in efibootmgr, as this is the first time I've tried
> the sysfs side of things. If you're in a position to help debug, I'd
> appreciate it.
In fact, it *was* a bug in efibootmgr. Here's the patch to fix it...
There's also a bug in efivars.c:efivar_delete(), this one-liner should fix it.
--- linux-2.6.5/drivers/firmware/efivars.c 2004-04-24 06:58:08.000000000 -0400
+++ linux-2.6.5.mld/drivers/firmware/efivars.c 2004-04-24 12:09:28.099545422 -0400
@@ -532,7 +532,7 @@ efivar_delete(struct subsystem *sub, con
efivar_unregister(search_efivar);
/* It's dead Jim.... */
- return status;
+ return size;
}
--
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
diff -urNp --exclude='*~' efibootmgr-0.5.0-test2.orig/src/lib/efi.c efibootmgr-0.5.0-test2/src/lib/efi.c
--- efibootmgr-0.5.0-test2.orig/src/lib/efi.c 2003-09-07 00:24:18.000000000 -0400
+++ efibootmgr-0.5.0-test2/src/lib/efi.c 2004-04-24 12:15:25.375908233 -0400
@@ -166,7 +166,7 @@ create_or_edit_variable(efi_variable_t *
memcpy(&testvar, var, sizeof(*var));
variable_to_name(var, name);
- if (read_variable(name, &testvar))
+ if (read_variable(name, &testvar) == EFI_SUCCESS)
return edit_variable(var);
else
return create_variable(var);
diff -urNp --exclude='*~' efibootmgr-0.5.0-test2.orig/src/lib/efivars_sysfs.c efibootmgr-0.5.0-test2/src/lib/efivars_sysfs.c
--- efibootmgr-0.5.0-test2.orig/src/lib/efivars_sysfs.c 2003-09-07 00:21:37.000000000 -0400
+++ efibootmgr-0.5.0-test2/src/lib/efivars_sysfs.c 2004-04-24 12:35:38.207924626 -0400
@@ -49,8 +49,6 @@ sysfs_read_variable(const char *name, ef
snprintf(filename, PATH_MAX-1, "%s/%s/raw_var", SYSFS_DIR_EFI_VARS,name);
fd = open(filename, O_RDONLY);
if (fd == -1) {
- sprintf(buffer, "sysfs_read_variable():open(%s)", filename);
- perror(buffer);
return EFI_NOT_FOUND;
}
readsize = read(fd, var, sizeof(*var));
@@ -74,8 +72,6 @@ sysfs_write_variable(const char *filenam
fd = open(filename, O_WRONLY);
if (fd == -1) {
- sprintf(buffer, "sysfs_write_variable():open(%s)", filename);
- perror(buffer);
return EFI_INVALID_PARAMETER;
}
writesize = write(fd, var, sizeof(*var));
Patch below fixes three small bugs in efivars.c as posted by Matt
Tolentino last week and included in the latest -mm. Aside from this
small patch, I'm quite pleased with Matt T's work, thanks!
* dummy() used for reading write-only sysfs files should return
-ENODEV to indicate failure, not 0.
* efivar_create() should return the number of bytes written on
success, not zero.
* efivar_delete() should return the number of bytes written on
success, not zero.
Compiled, tested with efibootmgr-0.5.0-test3. The anomolies I noted
late Friday night are resolved by this kernel patch - efibootmgr was
actually testing the values returned by writes.
--
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
--- linux-2.6.5.orig/drivers/firmware/efivars.c 2004-04-24 06:58:08.000000000 -0400
+++ linux-2.6.5/drivers/firmware/efivars.c 2004-04-27 03:48:33.037318467 -0400
@@ -1,7 +1,7 @@
/*
* EFI Variables - efivars.c
*
- * Copyright (C) 2001,2003 Dell <[email protected]>
+ * Copyright (C) 2001,2003,2004 Dell <[email protected]>
* Copyright (C) 2004 Intel Corporation <[email protected]>
*
* This code takes all variables accessible from EFI runtime and
@@ -23,6 +23,9 @@
*
* Changelog:
*
+ * 26 Apr 2004 - Matt Domsch <[email protected]>
+ * minor bug fixes
+ *
* 21 Apr 2004 - Matt Tolentino <[email protected])
* converted driver to export variable information via sysfs
* and moved to drivers/firmware directory
@@ -78,7 +81,7 @@ MODULE_AUTHOR("Matt Domsch <Matt_Domsch@
MODULE_DESCRIPTION("sysfs interface to EFI Variables");
MODULE_LICENSE("GPL");
-#define EFIVARS_VERSION "0.07 2003-Aug-29"
+#define EFIVARS_VERSION "0.07 2004-Apr-26"
/*
* efivars_lock protects two things:
@@ -408,7 +411,7 @@ static struct kobj_type ktype_efivar = {
static ssize_t
dummy(struct subsystem *sub, char *buf)
{
- return 0;
+ return -ENODEV;
}
static inline void
@@ -472,7 +475,10 @@ efivar_create(struct subsystem *sub, con
/* Create the entry in sysfs. Locking is not required here */
status = efivar_create_sysfs_entry(utf8_strsize(new_var->VariableName,
1024), new_var->VariableName, &new_var->VendorGuid);
- return status;
+ if (status) {
+ printk(KERN_WARNING "efivars: variable created, but sysfs entry wasn't.\n");
+ }
+ return count;
}
static ssize_t
@@ -532,7 +538,7 @@ efivar_delete(struct subsystem *sub, con
efivar_unregister(search_efivar);
/* It's dead Jim.... */
- return status;
+ return count;
}
static VAR_SUBSYS_ATTR(new_var, 0200, dummy, efivar_create);
>Patch below fixes three small bugs in efivars.c as posted by Matt
>Tolentino last week and included in the latest -mm. Aside from this
>small patch, I'm quite pleased with Matt T's work, thanks!=20
>
>* dummy() used for reading write-only sysfs files should return
> -ENODEV to indicate failure, not 0.
>* efivar_create() should return the number of bytes written on
> success, not zero. =20
>* efivar_delete() should return the number of bytes written on
> success, not zero.
>
>Compiled, tested with efibootmgr-0.5.0-test3. The anomolies I noted
>late Friday night are resolved by this kernel patch - efibootmgr was
>actually testing the values returned by writes.
Thanks for taking the time to look at this Matt...
matt
Good work on the sysfs port, I like it too. I ran into a little
problem trying it with the newest efibootmgr (test3) on an HP rx2600.
I'm not sure what the problem is, so this is a bug report/query.
Efibootmgr complains "show_boot_order(): No such file or directory".
However, it looks ok:
# find /sys/firmware/efi/vars/ -name BootOrder*
/sys/firmware/efi/vars/BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
Looking at the directory a little more closely
# stat BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /
File: `BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c /'
...
What's the purpose of the ' ' at the end of the directory name? The
comment in the code suggests it might have a purpose:
/* This is ugly, but necessary to separate one vendor's
private variables from another's. */
*(short_name + strlen(short_name)) = '-';
efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
*(short_name + strlen(short_name)) = ' ';
But the same comment was in the /proc efivar code, without the
additional space tagged onto the end. All of the EFI variables end with
a space, so there's nothing special about this one. Is the space
intentional? The data in the variable looks good otherwise:
# xxd BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /data
0000000: 0100 0000 0200 0500 0600 0300 ............
# xxd BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /guid
0000000: 3862 6534 6466 3631 2d39 3363 612d 3131 8be4df61-93ca-11
0000010: 6432 2d61 6130 642d 3030 6530 3938 3033 d2-aa0d-00e09803
0000020: 3262 3863 0a 2b8c.
# xxd BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /attributes
0000000: 4546 495f 5641 5249 4142 4c45 5f4e 4f4e EFI_VARIABLE_NON
0000010: 5f56 4f4c 4154 494c 450a 4546 495f 5641 _VOLATILE.EFI_VA
0000020: 5249 4142 4c45 5f42 4f4f 5453 4552 5649 RIABLE_BOOTSERVI
0000030: 4345 5f41 4343 4553 530a 4546 495f 5641 CE_ACCESS.EFI_VA
0000040: 5249 4142 4c45 5f52 554e 5449 4d45 5f41 RIABLE_RUNTIME_A
0000050: 4343 4553 530a CCESS.
# xxd BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /size
0000000: 3078 630a 0xc.
Thanks,
Alex
--
Alex Williamson HP Linux & Open Source Lab
On Thu, Apr 29, 2004 at 01:50:54PM -0600, Alex Williamson wrote:
> # stat BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /
> File: `BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c /'
FWIW, my Intel Tiger2-based system doesn't have the space at the end...
> *(short_name + strlen(short_name)) = '-';
> efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> *(short_name + strlen(short_name)) = ' ';
even though looking at this I would have expected it to...
Can you remove that last line and see what it does? Best as I can
tell, it isn't necessary or desired.
--
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & http://www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
On Thu, 2004-04-29 at 15:19, Matt Domsch wrote:
> On Thu, Apr 29, 2004 at 01:50:54PM -0600, Alex Williamson wrote:
> > # stat BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /
> > File: `BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c /'
>
> FWIW, my Intel Tiger2-based system doesn't have the space at the end...
>
> > *(short_name + strlen(short_name)) = '-';
> > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > *(short_name + strlen(short_name)) = ' ';
>
> even though looking at this I would have expected it to...
Yeah, I'm not sure how you couldn't have a space at the end...
>
> Can you remove that last line and see what it does? Best as I can
> tell, it isn't necessary or desired.
Yes, removing that last line above gets rid of the space, everything
looks right, and efibootmgr doesn't complain. I'd attach a patch but
I'm curious if Matt T. had some reason for adding it.
Alex
--
Alex Williamson HP Linux & Open Source Lab
> On Thu, 2004-04-29 at 15:19, Matt Domsch wrote:
> > On Thu, Apr 29, 2004 at 01:50:54PM -0600, Alex Williamson wrote:
> > > # stat BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c\ /
> > > File: `BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c /'
> >
> > FWIW, my Intel Tiger2-based system doesn't have the space
> at the end...
> >
> > > *(short_name + strlen(short_name)) = '-';
> > > efi_guid_unparse(vendor_guid, short_name +
> strlen(short_name));
> > > *(short_name + strlen(short_name)) = ' ';
> >
> > even though looking at this I would have expected it to...
>
> Yeah, I'm not sure how you couldn't have a space at the end...
That's odd.
> > Can you remove that last line and see what it does? Best as I can
> > tell, it isn't necessary or desired.
>
> Yes, removing that last line above gets rid of the space,
> everything
> looks right, and efibootmgr doesn't complain. I'd attach a patch but
> I'm curious if Matt T. had some reason for adding it.
I had it in there because when I originally rewrote this
driver for sysfs it kept cutting off the last character. I
had to put it aside for a bit and just didn't take it out
last week before I resent it. I just retried it on local
ia64 and x86 machines and it worked fine on both accounts
without the extra space.
Andrew, can you please apply the following patch to remove
space?
thanks,
matt
diff -urN linux-2.6.6-rc3-clean/drivers/firmware/efivars.c linux-2.6.6-rc3/drivers/firmware/efivars.c
--- linux-2.6.6-rc3-clean/drivers/firmware/efivars.c 2004-04-27 18:36:33.000000000 -0700
+++ linux-2.6.6-rc3/drivers/firmware/efivars.c 2004-04-29 16:31:48.052355632 -0700
@@ -633,7 +633,6 @@
*(short_name + strlen(short_name)) = '-';
efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
- *(short_name + strlen(short_name)) = ' ';
kobject_set_name(&new_efivar->kobj, short_name);
kobj_set_kset_s(new_efivar, vars_subsys);