Re: [PATCH] Use userland-like functions for reading the ACPI table

[Ãric Piel]

24/02/08 02:31, Christoph Hellwig wrote/a Ãcrit:
> On Sat, Feb 23, 2008 at 12:45:38PM -0800, Linus Torvalds wrote:
>>> As recommended by Christoph Hellwig, even if we can't rely on the userspace
>>> firmware loader so early at boot, at least use normal syscall (as in
>>> init/do_mounts_*.c). Similarly, use kfree() instead of ACPI_FREE().
>> So I'm missing a lot of the background here.
>>
>> I don't think "sys_open()" is in any way preferable to the alternatives, 
>> especially since it depends on thread-global state (the file descriptor 
>> table) rather than much more local state ("struct file" that you've 
>> opened).
>>
>> I think the calls to sys_open() in init do_dounts etc are very different: 
>> they really are more about a real kernel-level almost-user-mode thread 
>> than a core driver. 
> 
> Well, that's what this code is like aswell.  That's why I recommended
> to Eric to move it to init/ and make it look like that code.  I haven't
> quite caught up with the discussion yet, but Eric think moving it
> there might not be a that good idea.  Having this code in drivers/
> even if it's just called in init time is a bad idea, as people will
> copy it.  Then again using the functions the code was using before
> isn't any better.
Ok, I see the problem with leaving the code in the drivers/ directory.
Here is a new version of the patch that also moves the code to
init/initramfs.c, the file which seems to have the closer relationship
to the code. It also keeps the address space swapping which I had
mistakenly "cleaned up". 

Eric

--
Use userland-like functions for reading the ACPI table

As recommended by Christoph Hellwig, even if we can't rely on the userspace
firmware loader so early at boot, at least use normal syscall (as in
init/do_mounts_*.c). Move the function to init/ so that it's obvious that this
code is not ran as a normal core driver.  Moreover, use kfree() instead of
ACPI_FREE().

Also, it's recommended to open the file before stating it, to avoid surprises.

Signed-off-by: Eric Piel <eric.piel@xxxxxxxxxxxxxxxx>
---
 drivers/acpi/osl.c |   62 +--------------------------------------------------
 init/initramfs.c   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 8edba7b..b075781 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -93,6 +93,7 @@ static char osi_additional_string[OSI_STRING_LENGTH_MAX];
 
 #ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 static int acpi_no_initrd_override;
+extern struct acpi_table_header *acpi_find_dsdt_initrd(void);
 #endif
 
 /*
@@ -324,67 +325,6 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
 	return AE_OK;
 }
 
-#ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
-static struct acpi_table_header *acpi_find_dsdt_initrd(void)
-{
-	struct file *firmware_file;
-	mm_segment_t oldfs;
-	unsigned long len, len2;
-	struct acpi_table_header *dsdt_buffer, *ret = NULL;
-	struct kstat stat;
-	char *ramfs_dsdt_name = "/DSDT.aml";
-
-	printk(KERN_INFO PREFIX "Checking initramfs for custom DSDT\n");
-
-	/*
-	 * Never do this at home, only the user-space is allowed to open a file.
-	 * The clean way would be to use the firmware loader.
-	 * But this code must be run before there is any userspace available.
-	 * A static/init firmware infrastructure doesn't exist yet...
-	 */
-	if (vfs_stat(ramfs_dsdt_name, &stat) < 0)
-		return ret;
-
-	len = stat.size;
-	/* check especially against empty files */
-	if (len <= 4) {
-		printk(KERN_ERR PREFIX "Failed: DSDT only %lu bytes.\n", len);
-		return ret;
-	}
-
-	firmware_file = filp_open(ramfs_dsdt_name, O_RDONLY, 0);
-	if (IS_ERR(firmware_file)) {
-		printk(KERN_ERR PREFIX "Failed to open %s.\n", ramfs_dsdt_name);
-		return ret;
-	}
-
-	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
-	if (!dsdt_buffer) {
-		printk(KERN_ERR PREFIX "Failed to allocate %lu bytes.\n", len);
-		goto err;
-	}
-
-	oldfs = get_fs();
-	set_fs(KERNEL_DS);
-	len2 = vfs_read(firmware_file, (char __user *)dsdt_buffer, len,
-		&firmware_file->f_pos);
-	set_fs(oldfs);
-	if (len2 < len) {
-		printk(KERN_ERR PREFIX "Failed to read %lu bytes from %s.\n",
-			len, ramfs_dsdt_name);
-		ACPI_FREE(dsdt_buffer);
-		goto err;
-	}
-
-	printk(KERN_INFO PREFIX "Found %lu byte DSDT in %s.\n",
-			len, ramfs_dsdt_name);
-	ret = dsdt_buffer;
-err:
-	filp_close(firmware_file, NULL);
-	return ret;
-}
-#endif
-
 acpi_status
 acpi_os_table_override(struct acpi_table_header * existing_table,
 		       struct acpi_table_header ** new_table)
diff --git a/init/initramfs.c b/init/initramfs.c
index b74e845..9a0563a 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -1,3 +1,4 @@
+#include <asm/uaccess.h>
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
@@ -6,6 +7,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/syscalls.h>
+#include <acpi/acpi.h>
 
 static __initdata char *message;
 static void __init error(char *x)
@@ -577,3 +579,64 @@ int __init populate_rootfs(void)
 	}
 	return 0;
 }
+
+#ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
+struct acpi_table_header *acpi_find_dsdt_initrd(void)
+{
+	int fd;
+	mm_segment_t oldfs;
+	unsigned long len, len2;
+	struct acpi_table_header *dsdt_buffer, *ret = NULL;
+	struct kstat stat;
+	char *ramfs_dsdt_name = "/DSDT.aml";
+
+	printk(KERN_INFO "ACPI: Checking initramfs for custom DSDT\n");
+
+	/*
+	 * Never do this at home, only the user-space is allowed to open a file.
+	 * The clean way would be to use the firmware loader.
+	 * But this code must be run before there is any userspace available.
+	 * A static/init firmware infrastructure doesn't exist yet...
+	 */
+	fd = sys_open(ramfs_dsdt_name, O_RDONLY, 0);
+	if (fd < 0)
+		return ret; /* No need for warning, no DSDT override is normal */
+
+	/* There exists 3 different sys_fstat's, all are wrapper to vfs_fstat */
+	if (vfs_fstat(fd, &stat) < 0) {
+		printk(KERN_ERR "ACPI: Failed to stat %s.\n", ramfs_dsdt_name);
+		goto err;
+	}
+
+	len = stat.size;
+	/* check especially against empty files */
+	if (len <= 4) {
+		printk(KERN_ERR "ACPI: Failed: DSDT only %lu bytes.\n", len);
+		goto err;
+	}
+
+	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
+	if (!dsdt_buffer) {
+		printk(KERN_ERR "ACPI: Failed to allocate %lu bytes.\n", len);
+		goto err;
+	}
+
+	oldfs = get_fs();
+	set_fs(KERNEL_DS);
+	len2 = sys_read(fd, (char __user *)dsdt_buffer, len);
+	set_fs(oldfs);
+	if (len2 < len) {
+		printk(KERN_ERR "ACPI: Failed to read %lu bytes from %s.\n",
+			len, ramfs_dsdt_name);
+		kfree(dsdt_buffer);
+		goto err;
+	}
+
+	printk(KERN_INFO "ACPI: Found %lu byte DSDT in %s.\n",
+			len, ramfs_dsdt_name);
+	ret = dsdt_buffer;
+err:
+	sys_close(fd);
+	return ret;
+}
+#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/