acpi dsts loading and populate_rootfs

acpi dsts loading and populate_rootfs

[Christoph Hellwig]

Folks, moving this call around hidden behing in completely unreviewed
acpi junk is not acceptable.

Either populate_rootfs _is_ safe to be called earlier and then we should
do it always or it's not.  Either way such a change should be posted
separately and reviewd on lkml.

Len, can you please revert "ACPI: basic initramfs DSDT override support"
aka commit 71fc47a9adf8ee89e5c96a47222915c5485ac437 until we've sorted
this out properly?  Thanks.

Re: acpi dsts loading and populate_rootfs

[Christoph Hellwig]

On Sun, Feb 10, 2008 at 08:12:26AM +0100, Christoph Hellwig wrote:
> Folks, moving this call around hidden behing in completely unreviewed
> acpi junk is not acceptable.
> 
> Either populate_rootfs _is_ safe to be called earlier and then we should
> do it always or it's not.  Either way such a change should be posted
> separately and reviewd on lkml.
> 
> Len, can you please revert "ACPI: basic initramfs DSDT override support"
> aka commit 71fc47a9adf8ee89e5c96a47222915c5485ac437 until we've sorted
> this out properly?  Thanks.

And while we're at it the file reading thing in there is utter crap
aswell.  You really should be using the firmware loader which works
perfectly fine if you initramfs is set up for it.  So please folks,
back to the drawing board, do it properly and send it out to lkml
for review please.

Re: acpi dsts loading and populate_rootfs

[Eric Piel]

(adding some CC's)
Christoph Hellwig wrote:
> On Sun, Feb 10, 2008 at 08:12:26AM +0100, Christoph Hellwig wrote:
>> Folks, moving this call around hidden behing in completely unreviewed
>> acpi junk is not acceptable.
>>
>> Either populate_rootfs _is_ safe to be called earlier and then we should
>> do it always or it's not.  Either way such a change should be posted
>> separately and reviewd on lkml.
Hi,

I agree with you, the order of the initialization should not be changed 
by some config options. Either it works, or it doesn't! So the question 
now is : "is fine to move populate_rootfs() earlier?"

Some data points:
* It used to be called earlier. It was moved to be called later about a 
year ago by Linus (8d610dd52dd1da696e199e4b4545f33a2a5de5c6) with this log:
>     Make sure we populate the initroot filesystem late enough
>     
>     We should not initialize rootfs before all the core initializers have
>     run.  So do it as a separate stage just before starting the regular
>     driver initializers.
So there must be some good reasons to keep it late enough...

* This patch is used in SuSE, and it seems noone has ever had a problem 
with the initramfs early init.

I guess the problem that Linus solved by moving populate_rootfs() 
happens only rarely or on only few configurations. Linus, do you 
remember what kind of problem it was? How can I reproduce it?

One different solution that I implemented [1] was to have an 
"early_populate_rootfs()" before the acpi_early_init(), leaving 
populate_rootfs() at the normal place. If the early version fails, it's 
ok: we can't override the DSDT, but the later version will work as usual 
anyway. This solution also seems to work quite often (it's used in 
Ubuntu, Mandriva and PCLinuxOS)

Would that seem an acceptable solution? Or what other way exists?


>> Len, can you please revert "ACPI: basic initramfs DSDT override support"
>> aka commit 71fc47a9adf8ee89e5c96a47222915c5485ac437 until we've sorted
>> this out properly?  Thanks.
> 
> And while we're at it the file reading thing in there is utter crap
> aswell.  You really should be using the firmware loader which works
> perfectly fine if you initramfs is set up for it.  So please folks,
> back to the drawing board, do it properly and send it out to lkml
> for review please.
Christoph, if you have seen this part of the code, you have probably 
also read the big fat warning explaining why this cannot be done by 
firmware loader (ie: userspace cannot be run at this early time, 
corresponding to acpi_early_init()). However, you probably know the 
kernel ten times better than me. Could you explain what I misunderstood 
when writing this warning, and give me some hints about how to use the 
firmware loader in this case?

See you,
Eric


[1] : 
http://gaugusch.at/acpi-dsdt-initrd-patches/acpi-dsdt-initrd-v0.8.4-2.6.21.patch

Re: acpi dsts loading and populate_rootfs

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On Sun, 10 Feb 2008 12:58:09 +0100 Eric Piel wrote:

> (adding some CC's)
> Christoph Hellwig wrote:
> > On Sun, Feb 10, 2008 at 08:12:26AM +0100, Christoph Hellwig wrote:
> >> Folks, moving this call around hidden behing in completely unreviewed
> >> acpi junk is not acceptable.
> >>
> >> Either populate_rootfs _is_ safe to be called earlier and then we should
> >> do it always or it's not.  Either way such a change should be posted
> >> separately and reviewd on lkml.
> Hi,
>
> I agree with you, the order of the initialization should not be changed
> by some config options. Either it works, or it doesn't! So the question
> now is : "is fine to move populate_rootfs() earlier?"
>
> Some data points:
> * It used to be called earlier. It was moved to be called later about a
> year ago by Linus (8d610dd52dd1da696e199e4b4545f33a2a5de5c6) with this log:
> >     Make sure we populate the initroot filesystem late enough
> >
> >     We should not initialize rootfs before all the core initializers have
> >     run.  So do it as a separate stage just before starting the regular
> >     driver initializers.
> So there must be some good reasons to keep it late enough...
>
> * This patch is used in SuSE, and it seems noone has ever had a problem
> with the initramfs early init.
>
> I guess the problem that Linus solved by moving populate_rootfs()
> happens only rarely or on only few configurations. Linus, do you
> remember what kind of problem it was? How can I reproduce it?

AFAIR the problem was that the kernel was trying to exec /sbin/hotplug
before some kernel subsystems (e.g., pipe support) were completely
initialized, which caused oopses during boot.

Initramfs images generated by distro tools usually do not contain
/sbin/hotplug, and therefore avoid the problem.  (The kernel might
also call /sbin/modprobe by itself, but it either does not do it
during early boot, or /sbin/modprobe does not use kernel features
which had not been initialized yet).

> One different solution that I implemented [1] was to have an
> "early_populate_rootfs()" before the acpi_early_init(), leaving
> populate_rootfs() at the normal place. If the early version fails, it's
> ok: we can't override the DSDT, but the later version will work as usual
> anyway. This solution also seems to work quite often (it's used in
> Ubuntu, Mandriva and PCLinuxOS)

This would not solve the problem, because /sbin/hotplug (if present in
the initramfs image) will exist too early.

> Would that seem an acceptable solution? Or what other way exists?

Disabling call_usermodehelper() until all core initializers had
completed would fix the problem too; will such change be acceptable?

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: acpi dsts loading and populate_rootfs

[Éric Piel]

11/02/08 14:47, Sergey Vlasov wrote/a écrit:
> On Sun, 10 Feb 2008 12:58:09 +0100 Eric Piel wrote:
> 
>> I guess the problem that Linus solved by moving populate_rootfs()
>> happens only rarely or on only few configurations. Linus, do you
>> remember what kind of problem it was? How can I reproduce it?
> 
> AFAIR the problem was that the kernel was trying to exec /sbin/hotplug
> before some kernel subsystems (e.g., pipe support) were completely
> initialized, which caused oopses during boot.
> 
> Initramfs images generated by distro tools usually do not contain
> /sbin/hotplug, and therefore avoid the problem.  (The kernel might
> also call /sbin/modprobe by itself, but it either does not do it
> during early boot, or /sbin/modprobe does not use kernel features
> which had not been initialized yet).
Thanks a lot for the pointer. I was able to find the original bug
report. After reading the various fix proposals and the current code I
think I understand what is going on.

Basically, the problem is that one should not run a user land program
too early because not everything is initialized yet (for instance
pipefs). This might happen because at initialization some drivers can
trigger automatically /sbin/hotplug (via call_usermodehelper()). So the
fix which was applied consists in populating the rootfs only once all
the kernel subsystem is initialized. In this way, the hotplugging is
still triggered but as the user space program doesn't exist it can't be
called! (hehe)

>> One different solution that I implemented [1] was to have an
>> "early_populate_rootfs()" before the acpi_early_init(), leaving
>> populate_rootfs() at the normal place. If the early version fails, it's
>> ok: we can't override the DSDT, but the later version will work as usual
>> anyway. This solution also seems to work quite often (it's used in
>> Ubuntu, Mandriva and PCLinuxOS)
> 
> This would not solve the problem, because /sbin/hotplug (if present in
> the initramfs image) will exist too early.
Yes, now that I understand the problem, reading the initramfs twice
would in no way help!

>> Would that seem an acceptable solution? Or what other way exists?
> 
> Disabling call_usermodehelper() until all core initializers had
> completed would fix the problem too; will such change be acceptable?
Yes, that looks like a nice way. Actually, I discovered that for suspend
and resume, there is exactly the same need, and a special flag is
already available: usermodehelper_disabled.

So here is a proposal patch leveraging this flag. The rootfs is
populated early but user land execution is forbidden until we reach a
sufficient initialization state.  I think it even has the bonus of
doing _explicitly_ what we want (avoid user land execution).

Does this look good? (if so, I can probably make it even cleaner by
renaming/removing the rootfs_initcall level)

Eric
--

diff --git a/init/initramfs.c b/init/initramfs.c
index c0b1e05..b74e845 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -577,10 +577,3 @@ int __init populate_rootfs(void)
 	}
 	return 0;
 }
-#ifndef CONFIG_ACPI_CUSTOM_DSDT_INITRD
-/*
- * if this option is enabled, populate_rootfs() is called _earlier_ in the
- * boot sequence. This insures that the ACPI initialisation can find the file.
- */
-rootfs_initcall(populate_rootfs);
-#endif
diff --git a/init/main.c b/init/main.c
index 8b19820..703ded0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -102,11 +102,7 @@ static inline void mark_rodata_ro(void) { }
 extern void tc_init(void);
 #endif
 
-#ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 extern int populate_rootfs(void);
-#else
-static inline void populate_rootfs(void) {}
-#endif
 
 enum system_states system_state;
 EXPORT_SYMBOL(system_state);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bb7df2a..d6a2b40 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -267,14 +267,23 @@ static void __call_usermodehelper(struct work_struct *work)
 	}
 }
 
-#ifdef CONFIG_PM
 /*
  * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
  * (used for preventing user land processes from being created after the user
  * land has been frozen during a system-wide hibernation or suspend operation).
+ * It is also used at boot up to avoid calling a user land process before all
+ * the kernel subsystems are initialised (such as pipefs).
  */
-static int usermodehelper_disabled;
+static int usermodehelper_disabled = 1;
 
+static int __init enable_usermodehelper(void)
+{
+	usermodehelper_disabled = 0;
+	return 0;
+}
+rootfs_initcall(enable_usermodehelper);
+
+#ifdef CONFIG_PM
 /* Number of helpers running */
 static atomic_t running_helpers = ATOMIC_INIT(0);
 
@@ -342,8 +351,6 @@ static void register_pm_notifier_callback(void)
 	pm_notifier(usermodehelper_pm_callback, 0);
 }
 #else /* CONFIG_PM */
-#define usermodehelper_disabled	0
-
 static inline void helper_lock(void) {}
 static inline void helper_unlock(void) {}
 static inline void register_pm_notifier_callback(void) {}

[PATCH] Allow populate_rootfs() to be called early (was: acpi dsts loading and populate_rootfs)

[Éric Piel]

12/02/08 00:41, Éric Piel wrote/a écrit:
> 11/02/08 14:47, Sergey Vlasov wrote/a écrit:
>>> Would that seem an acceptable solution? Or what other way exists?
>> Disabling call_usermodehelper() until all core initializers had
>> completed would fix the problem too; will such change be acceptable?
> Yes, that looks like a nice way. Actually, I discovered that for suspend
> and resume, there is exactly the same need, and a special flag is
> already available: usermodehelper_disabled.
> 
> So here is a proposal patch leveraging this flag. The rootfs is
> populated early but user land execution is forbidden until we reach a
> sufficient initialization state.  I think it even has the bonus of
> doing _explicitly_ what we want (avoid user land execution).
> 
> Does this look good? (if so, I can probably make it even cleaner by
> renaming/removing the rootfs_initcall level)

It's been a week and no one has screamed, so I guess the idea looks fine
to everyone :-) 

Here is a boot tested patch for integration. In addition to the previous
version, it removes also rootfs_initcall(), and uses fs_initcall_sync().
There should be absolutely no difference in behaviour as there was no
user of fs_initcall_sync() in the kernel.

I'm not sure what should be the best tree to go through. Maybe the ACPI
tree, as all this is needed for CONFIG_ACPI_CUSTOM_DSDT_INITRD...

See you,
Eric

---
For ACPI table override, we need the rootfs available early. So this patch
* removes the kludge calling populate_rootfs early _sometimes_
* leverage the usermodehelper_disabled variable to prevent too early userspace
  execution would could lead to oops if the kernel subsystem are not yet initialised

This solves a theoretical bug where someone has both a DSDT table and a
hotplug helper in his or her initramfs.

---
 include/asm-generic/vmlinux.lds.h |    1 -
 include/linux/init.h              |    1 -
 init/initramfs.c                  |    7 -------
 init/main.c                       |    4 ----
 kernel/kmod.c                     |   15 +++++++++++----
 5 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f784d2f..19ddbae 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -335,7 +335,6 @@
   	*(.initcall4s.init)						\
   	*(.initcall5.init)						\
   	*(.initcall5s.init)						\
-	*(.initcallrootfs.init)						\
   	*(.initcall6.init)						\
   	*(.initcall6s.init)						\
   	*(.initcall7.init)						\
diff --git a/include/linux/init.h b/include/linux/init.h
index a404a00..ecd05cd 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -183,7 +183,6 @@ void prepare_namespace(void);
 #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
 #define fs_initcall(fn)			__define_initcall("5",fn,5)
 #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
-#define rootfs_initcall(fn)		__define_initcall("rootfs",fn,rootfs)
 #define device_initcall(fn)		__define_initcall("6",fn,6)
 #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
 #define late_initcall(fn)		__define_initcall("7",fn,7)
diff --git a/init/initramfs.c b/init/initramfs.c
index c0b1e05..b74e845 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -577,10 +577,3 @@ int __init populate_rootfs(void)
 	}
 	return 0;
 }
-#ifndef CONFIG_ACPI_CUSTOM_DSDT_INITRD
-/*
- * if this option is enabled, populate_rootfs() is called _earlier_ in the
- * boot sequence. This insures that the ACPI initialisation can find the file.
- */
-rootfs_initcall(populate_rootfs);
-#endif
diff --git a/init/main.c b/init/main.c
index 8b19820..703ded0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -102,11 +102,7 @@ static inline void mark_rodata_ro(void) { }
 extern void tc_init(void);
 #endif
 
-#ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 extern int populate_rootfs(void);
-#else
-static inline void populate_rootfs(void) {}
-#endif
 
 enum system_states system_state;
 EXPORT_SYMBOL(system_state);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bb7df2a..112b261 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -267,14 +267,23 @@ static void __call_usermodehelper(struct work_struct *work)
 	}
 }
 
-#ifdef CONFIG_PM
 /*
  * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
  * (used for preventing user land processes from being created after the user
  * land has been frozen during a system-wide hibernation or suspend operation).
+ * It is also used at boot up to avoid calling a user land process before all
+ * the kernel subsystems are initialised (such as pipefs).
  */
-static int usermodehelper_disabled;
+static int usermodehelper_disabled = 1;
 
+static int __init enable_usermodehelper(void)
+{
+	usermodehelper_disabled = 0;
+	return 0;
+}
+fs_initcall_sync(enable_usermodehelper);
+
+#ifdef CONFIG_PM
 /* Number of helpers running */
 static atomic_t running_helpers = ATOMIC_INIT(0);
 
@@ -342,8 +351,6 @@ static void register_pm_notifier_callback(void)
 	pm_notifier(usermodehelper_pm_callback, 0);
 }
 #else /* CONFIG_PM */
-#define usermodehelper_disabled	0
-
 static inline void helper_lock(void) {}
 static inline void helper_unlock(void) {}
 static inline void register_pm_notifier_callback(void) {}

Re: [PATCH] Allow populate_rootfs() to be called early (was: acpi dsts loading and populate_rootfs)

[Christoph Hellwig]

On Thu, Feb 21, 2008 at 08:02:36PM +0100, ??ric Piel wrote:
> It's been a week and no one has screamed, so I guess the idea looks fine
> to everyone :-) 
> 
> Here is a boot tested patch for integration. In addition to the previous
> version, it removes also rootfs_initcall(), and uses fs_initcall_sync().
> There should be absolutely no difference in behaviour as there was no
> user of fs_initcall_sync() in the kernel.
> 
> I'm not sure what should be the best tree to go through. Maybe the ACPI
> tree, as all this is needed for CONFIG_ACPI_CUSTOM_DSDT_INITRD...

Actually I've just got a report today where we get an uninitialized
spinlock due to this in -mm.  It's probably possible to work around it,
though.

Re: [PATCH] Allow populate_rootfs() to be called early

[Éric Piel]

21/02/08 20:04, Christoph Hellwig wrote/a écrit:
> On Thu, Feb 21, 2008 at 08:02:36PM +0100, Eric Piel wrote:
>> It's been a week and no one has screamed, so I guess the idea looks fine
>> to everyone :-) 
>>
>> Here is a boot tested patch for integration. In addition to the previous
>> version, it removes also rootfs_initcall(), and uses fs_initcall_sync().
>> There should be absolutely no difference in behaviour as there was no
>> user of fs_initcall_sync() in the kernel.
>>
>> I'm not sure what should be the best tree to go through. Maybe the ACPI
>> tree, as all this is needed for CONFIG_ACPI_CUSTOM_DSDT_INITRD...
> 
> Actually I've just got a report today where we get an uninitialized
> spinlock due to this in -mm.  It's probably possible to work around it,
> though.
What do you mean by "due to this"? Due to userspace helper being called
too early? Then this patch should fix the bug.

Eric

[PATCH] Allow populate_rootfs() to be called early (resent, with sob)

[Éric Piel]

21/02/08 20:02, Éric Piel wrote/a écrit:
> 12/02/08 00:41, Éric Piel wrote/a écrit:
>> 11/02/08 14:47, Sergey Vlasov wrote/a écrit:
>>>> Would that seem an acceptable solution? Or what other way exists?
>>> Disabling call_usermodehelper() until all core initializers had
>>> completed would fix the problem too; will such change be acceptable?
>> Yes, that looks like a nice way. Actually, I discovered that for suspend
>> and resume, there is exactly the same need, and a special flag is
>> already available: usermodehelper_disabled.
>>
>> So here is a proposal patch leveraging this flag. The rootfs is
>> populated early but user land execution is forbidden until we reach a
>> sufficient initialization state.  I think it even has the bonus of
>> doing _explicitly_ what we want (avoid user land execution).
>>
>> Does this look good? (if so, I can probably make it even cleaner by
>> renaming/removing the rootfs_initcall level)
> 
> It's been a week and no one has screamed, so I guess the idea looks fine
> to everyone :-) 
> 
> Here is a boot tested patch for integration. In addition to the previous
> version, it removes also rootfs_initcall(), and uses fs_initcall_sync().
> There should be absolutely no difference in behaviour as there was no
> user of fs_initcall_sync() in the kernel.
> 
> I'm not sure what should be the best tree to go through. Maybe the ACPI
> tree, as all this is needed for CONFIG_ACPI_CUSTOM_DSDT_INITRD...
> 
Len has reminded me that I have to add a s-o-b line to my patches :-)
Here is a corrected version (against 2.6.25-rc2).

Eric 
--

For ACPI table override, we need the rootfs available early. So this patch
* removes the kludge calling populate_rootfs early _sometimes_
* leverage the usermodehelper_disabled variable to prevent too early userspace
  execution would could lead to oops if the kernel subsystem are not yet initialised

Signed-off-by: Eric Piel <eric.piel@tremplin-utc.net>
---
 include/asm-generic/vmlinux.lds.h |    1 -
 include/linux/init.h              |    1 -
 init/initramfs.c                  |    7 -------
 init/main.c                       |    4 ----
 kernel/kmod.c                     |   15 +++++++++++----
 5 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f784d2f..19ddbae 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -335,7 +335,6 @@
   	*(.initcall4s.init)						\
   	*(.initcall5.init)						\
   	*(.initcall5s.init)						\
-	*(.initcallrootfs.init)						\
   	*(.initcall6.init)						\
   	*(.initcall6s.init)						\
   	*(.initcall7.init)						\
diff --git a/include/linux/init.h b/include/linux/init.h
index a404a00..ecd05cd 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -183,7 +183,6 @@ void prepare_namespace(void);
 #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
 #define fs_initcall(fn)			__define_initcall("5",fn,5)
 #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
-#define rootfs_initcall(fn)		__define_initcall("rootfs",fn,rootfs)
 #define device_initcall(fn)		__define_initcall("6",fn,6)
 #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
 #define late_initcall(fn)		__define_initcall("7",fn,7)
diff --git a/init/initramfs.c b/init/initramfs.c
index c0b1e05..b74e845 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -577,10 +577,3 @@ int __init populate_rootfs(void)
 	}
 	return 0;
 }
-#ifndef CONFIG_ACPI_CUSTOM_DSDT_INITRD
-/*
- * if this option is enabled, populate_rootfs() is called _earlier_ in the
- * boot sequence. This insures that the ACPI initialisation can find the file.
- */
-rootfs_initcall(populate_rootfs);
-#endif
diff --git a/init/main.c b/init/main.c
index 8b19820..703ded0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -102,11 +102,7 @@ static inline void mark_rodata_ro(void) { }
 extern void tc_init(void);
 #endif
 
-#ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 extern int populate_rootfs(void);
-#else
-static inline void populate_rootfs(void) {}
-#endif
 
 enum system_states system_state;
 EXPORT_SYMBOL(system_state);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bb7df2a..112b261 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -267,14 +267,23 @@ static void __call_usermodehelper(struct work_struct *work)
 	}
 }
 
-#ifdef CONFIG_PM
 /*
  * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
  * (used for preventing user land processes from being created after the user
  * land has been frozen during a system-wide hibernation or suspend operation).
+ * It is also used at boot up to avoid calling a user land process before all
+ * the kernel subsystems are initialised (such as pipefs).
  */
-static int usermodehelper_disabled;
+static int usermodehelper_disabled = 1;
 
+static int __init enable_usermodehelper(void)
+{
+	usermodehelper_disabled = 0;
+	return 0;
+}
+fs_initcall_sync(enable_usermodehelper);
+
+#ifdef CONFIG_PM
 /* Number of helpers running */
 static atomic_t running_helpers = ATOMIC_INIT(0);
 
@@ -342,8 +351,6 @@ static void register_pm_notifier_callback(void)
 	pm_notifier(usermodehelper_pm_callback, 0);
 }
 #else /* CONFIG_PM */
-#define usermodehelper_disabled	0
-
 static inline void helper_lock(void) {}
 static inline void helper_unlock(void) {}
 static inline void register_pm_notifier_callback(void) {}

Re: acpi dsts loading and populate_rootfs

[Christoph Hellwig]

[skipping the populate_rootfs discussion as it seems you have a better
 handle on that than me]

On Sun, Feb 10, 2008 at 12:58:09PM +0100, Eric Piel wrote:
> >And while we're at it the file reading thing in there is utter crap
> >aswell.  You really should be using the firmware loader which works
> >perfectly fine if you initramfs is set up for it.  So please folks,
> >back to the drawing board, do it properly and send it out to lkml
> >for review please.
> Christoph, if you have seen this part of the code, you have probably 
> also read the big fat warning explaining why this cannot be done by 
> firmware loader (ie: userspace cannot be run at this early time, 
> corresponding to acpi_early_init()). However, you probably know the 
> kernel ten times better than me. Could you explain what I misunderstood 
> when writing this warning, and give me some hints about how to use the 
> firmware loader in this case?

Sorry, I misparsed the comment.  I took it for the usual I'm too lazy
to put something that could load firmware into initramfs excuse.

But thinking about it is there a reason acpi initialization needs to
happen so early that we can't even have userspace in initramfs running?

But if we can't use real userspace this could should at least be written
like the pseudo-userspace in init/do_mounts*.c, using the sys_ syscall
implementations.

As an additional comment the stat + open approach is racy and not a good
idea.  Please just open the file using sys_open, it will tell you
if the file doesn't exist and then use fstat on it to find the
length.  It would also be useful if this kind of code is not hidden
inside acpi but rather done somewhere close to the early init code
because that's where people would expect this kind of nastiness._
syscall
implementations.

As an additional comment the stat + open approach is racy and not a good
idea.  Please just open the file using sys_open, it will tell you
if the file doesn't exist and then use fstat on it to find the
length.  It would also be useful if this kind of code is not hidden
inside acpi but rather done somewhere close to the early init code
because that's where people would expect this kind of nastiness.

Re: acpi dsts loading and populate_rootfs

[Éric Piel]

12/02/08 06:37, Christoph Hellwig wrote/a écrit:
> [skipping the populate_rootfs discussion as it seems you have a better
>  handle on that than me]
> 
> On Sun, Feb 10, 2008 at 12:58:09PM +0100, Eric Piel wrote:
>>> And while we're at it the file reading thing in there is utter crap
>>> aswell.  You really should be using the firmware loader which works
>>> perfectly fine if you initramfs is set up for it.  So please folks,
>>> back to the drawing board, do it properly and send it out to lkml
>>> for review please.
>> Christoph, if you have seen this part of the code, you have probably 
>> also read the big fat warning explaining why this cannot be done by 
>> firmware loader (ie: userspace cannot be run at this early time, 
>> corresponding to acpi_early_init()). However, you probably know the 
>> kernel ten times better than me. Could you explain what I misunderstood 
>> when writing this warning, and give me some hints about how to use the 
>> firmware loader in this case?
> 
> Sorry, I misparsed the comment.  I took it for the usual I'm too lazy
> to put something that could load firmware into initramfs excuse.
> 
> But thinking about it is there a reason acpi initialization needs to
> happen so early that we can't even have userspace in initramfs running?

Hi,
I guess in the complete absolute point of view it's possible to run
userspace without ACPI, after all that's what happens if you don't
activate ACPI in your kernel. However, so far I've taken the init order
as a constant. I'd really prefer not to have to mess with a complete
init order reorganization ;-)

> 
> But if we can't use real userspace this could should at least be written
> like the pseudo-userspace in init/do_mounts*.c, using the sys_ syscall
> implementations.
Yes, thank you very much for the links. Attached is a patch that does
this.

> 
> As an additional comment the stat + open approach is racy and not a good
> idea.  Please just open the file using sys_open, it will tell you
> if the file doesn't exist and then use fstat on it to find the
> length.  It would also be useful if this kind of code is not hidden
> inside acpi but rather done somewhere close to the early init code
> because that's where people would expect this kind of nastiness.
The attached patch also fixes the stat + open order.

Concerning the place of the code, I've tried to find a better place.
However, as acpi_early_init(), from which this function is called, is
placed in driver/acpi/ and the acpi_find_dsdt_initrd() function contains
quite a few references to acpi code it really looked strange to move it
out from the current file. If you still think it make much more sense to
move it somewhere else, could you hint me about which you would think it
fit better in?

In the mean time, here is a patch which should get the situation already
much cleaner. It has been tested on various configs (with and without
DSDT). Let me know if you think it is acceptable.

See you,
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). Similarly, use kfree() instead of ACPI_FREE().

Also, it's recommended to open the file before stating it, to avoid surprises.
---
 drivers/acpi/osl.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 34b3386..b836305 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -42,6 +42,7 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/processor.h>
 #include <asm/uaccess.h>
+#include <linux/syscalls.h>
 
 #include <linux/efi.h>
 #include <linux/ioport.h>
@@ -327,8 +328,7 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
 #ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 static struct acpi_table_header *acpi_find_dsdt_initrd(void)
 {
-	struct file *firmware_file;
-	mm_segment_t oldfs;
+	int fd;
 	unsigned long len, len2;
 	struct acpi_table_header *dsdt_buffer, *ret = NULL;
 	struct kstat stat;
@@ -342,20 +342,21 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 	 * 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;
+	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 PREFIX "Failed to stat %s.\n", ramfs_dsdt_name);
+		goto err;
+	}
 
 	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;
+		goto err;
 	}
 
 	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
@@ -364,15 +365,11 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 		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);
+	len2 = sys_read(fd, (char __user *)dsdt_buffer, len);
 	if (len2 < len) {
 		printk(KERN_ERR PREFIX "Failed to read %lu bytes from %s.\n",
 			len, ramfs_dsdt_name);
-		ACPI_FREE(dsdt_buffer);
+		kfree(dsdt_buffer);
 		goto err;
 	}
 
@@ -380,7 +377,7 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 			len, ramfs_dsdt_name);
 	ret = dsdt_buffer;
 err:
-	filp_close(firmware_file, NULL);
+	sys_close(fd);
 	return ret;
 }
 #endif

Re: acpi dsts loading and populate_rootfs

[Thomas Renninger]

On Thu, 2008-02-21 at 19:46 +0100, Éric Piel wrote:
> 12/02/08 06:37, Christoph Hellwig wrote/a écrit:
> > [skipping the populate_rootfs discussion as it seems you have a better
> >  handle on that than me]
> > 
> > On Sun, Feb 10, 2008 at 12:58:09PM +0100, Eric Piel wrote:
> >>> And while we're at it the file reading thing in there is utter crap
> >>> aswell.  You really should be using the firmware loader which works
> >>> perfectly fine if you initramfs is set up for it.  So please folks,
> >>> back to the drawing board, do it properly and send it out to lkml
> >>> for review please.
> >> Christoph, if you have seen this part of the code, you have probably 
> >> also read the big fat warning explaining why this cannot be done by 
> >> firmware loader (ie: userspace cannot be run at this early time, 
> >> corresponding to acpi_early_init()). However, you probably know the 
> >> kernel ten times better than me. Could you explain what I misunderstood 
> >> when writing this warning, and give me some hints about how to use the 
> >> firmware loader in this case?
> > 
> > Sorry, I misparsed the comment.  I took it for the usual I'm too lazy
> > to put something that could load firmware into initramfs excuse.
> > 
> > But thinking about it is there a reason acpi initialization needs to
> > happen so early that we can't even have userspace in initramfs running?

Maybe you do not need to activate ACPI mode yet, but you need to load
the tables early.

> Hi,
> I guess in the complete absolute point of view it's possible to run
> userspace without ACPI, after all that's what happens if you don't
> activate ACPI in your kernel. However, so far I've taken the init order
> as a constant. I'd really prefer not to have to mess with a complete
> init order reorganization ;-)

Which is probably a good idea.
AFAIK Numa, possibly Apic tables must be available quite early.

   Thomas

> > 
> > But if we can't use real userspace this could should at least be written
> > like the pseudo-userspace in init/do_mounts*.c, using the sys_ syscall
> > implementations.
> Yes, thank you very much for the links. Attached is a patch that does
> this.
> 
> > 
> > As an additional comment the stat + open approach is racy and not a good
> > idea.  Please just open the file using sys_open, it will tell you
> > if the file doesn't exist and then use fstat on it to find the
> > length.  It would also be useful if this kind of code is not hidden
> > inside acpi but rather done somewhere close to the early init code
> > because that's where people would expect this kind of nastiness.
> The attached patch also fixes the stat + open order.
> 
> Concerning the place of the code, I've tried to find a better place.
> However, as acpi_early_init(), from which this function is called, is
> placed in driver/acpi/ and the acpi_find_dsdt_initrd() function contains
> quite a few references to acpi code it really looked strange to move it
> out from the current file. If you still think it make much more sense to
> move it somewhere else, could you hint me about which you would think it
> fit better in?
> 
> In the mean time, here is a patch which should get the situation already
> much cleaner. It has been tested on various configs (with and without
> DSDT). Let me know if you think it is acceptable.
> 
> See you,
> 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). Similarly, use kfree() instead of ACPI_FREE().
> 
> Also, it's recommended to open the file before stating it, to avoid surprises.
> ---
>  drivers/acpi/osl.c |   33 +++++++++++++++------------------
>  1 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 34b3386..b836305 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -42,6 +42,7 @@
>  #include <acpi/acpi_bus.h>
>  #include <acpi/processor.h>
>  #include <asm/uaccess.h>
> +#include <linux/syscalls.h>
>  
>  #include <linux/efi.h>
>  #include <linux/ioport.h>
> @@ -327,8 +328,7 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
>  #ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
>  static struct acpi_table_header *acpi_find_dsdt_initrd(void)
>  {
> -	struct file *firmware_file;
> -	mm_segment_t oldfs;
> +	int fd;
>  	unsigned long len, len2;
>  	struct acpi_table_header *dsdt_buffer, *ret = NULL;
>  	struct kstat stat;
> @@ -342,20 +342,21 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
>  	 * 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;
> +	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 PREFIX "Failed to stat %s.\n", ramfs_dsdt_name);
> +		goto err;
> +	}
>  
>  	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;
> +		goto err;
>  	}
>  
>  	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
> @@ -364,15 +365,11 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
>  		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);
> +	len2 = sys_read(fd, (char __user *)dsdt_buffer, len);
>  	if (len2 < len) {
>  		printk(KERN_ERR PREFIX "Failed to read %lu bytes from %s.\n",
>  			len, ramfs_dsdt_name);
> -		ACPI_FREE(dsdt_buffer);
> +		kfree(dsdt_buffer);
>  		goto err;
>  	}
>  
> @@ -380,7 +377,7 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
>  			len, ramfs_dsdt_name);
>  	ret = dsdt_buffer;
>  err:
> -	filp_close(firmware_file, NULL);
> +	sys_close(fd);
>  	return ret;
>  }
>  #endif
> 

Re: acpi dsts loading and populate_rootfs

[Thomas Renninger]

On Fri, 2008-02-22 at 09:51 +0100, Thomas Renninger wrote:
> On Thu, 2008-02-21 at 19:46 +0100, Éric Piel wrote:
> > 12/02/08 06:37, Christoph Hellwig wrote/a écrit:
> > > [skipping the populate_rootfs discussion as it seems you have a better
> > >  handle on that than me]
> > > 
> > > On Sun, Feb 10, 2008 at 12:58:09PM +0100, Eric Piel wrote:
> > >>> And while we're at it the file reading thing in there is utter crap
> > >>> aswell.  You really should be using the firmware loader which works
> > >>> perfectly fine if you initramfs is set up for it.  So please folks,
> > >>> back to the drawing board, do it properly and send it out to lkml
> > >>> for review please.
> > >> Christoph, if you have seen this part of the code, you have probably 
> > >> also read the big fat warning explaining why this cannot be done by 
> > >> firmware loader (ie: userspace cannot be run at this early time, 
> > >> corresponding to acpi_early_init()). However, you probably know the 
> > >> kernel ten times better than me. Could you explain what I misunderstood 
> > >> when writing this warning, and give me some hints about how to use the 
> > >> firmware loader in this case?
> > > 
> > > Sorry, I misparsed the comment.  I took it for the usual I'm too lazy
> > > to put something that could load firmware into initramfs excuse.
> > > 
> > > But thinking about it is there a reason acpi initialization needs to
> > > happen so early that we can't even have userspace in initramfs running?
> 
> Maybe you do not need to activate ACPI mode yet, but you need to load
> the tables early.
Forget that one, the tables are fetched much earlier...

    Thomas
> 
> > Hi,
> > I guess in the complete absolute point of view it's possible to run
> > userspace without ACPI, after all that's what happens if you don't
> > activate ACPI in your kernel. However, so far I've taken the init order
> > as a constant. I'd really prefer not to have to mess with a complete
> > init order reorganization ;-)
> 
> Which is probably a good idea.
> AFAIK Numa, possibly Apic tables must be available quite early.
> 
>    Thomas
> 
> > > 
> > > But if we can't use real userspace this could should at least be written
> > > like the pseudo-userspace in init/do_mounts*.c, using the sys_ syscall
> > > implementations.
> > Yes, thank you very much for the links. Attached is a patch that does
> > this.
> > 
> > > 
> > > As an additional comment the stat + open approach is racy and not a good
> > > idea.  Please just open the file using sys_open, it will tell you
> > > if the file doesn't exist and then use fstat on it to find the
> > > length.  It would also be useful if this kind of code is not hidden
> > > inside acpi but rather done somewhere close to the early init code
> > > because that's where people would expect this kind of nastiness.
> > The attached patch also fixes the stat + open order.
> > 
> > Concerning the place of the code, I've tried to find a better place.
> > However, as acpi_early_init(), from which this function is called, is
> > placed in driver/acpi/ and the acpi_find_dsdt_initrd() function contains
> > quite a few references to acpi code it really looked strange to move it
> > out from the current file. If you still think it make much more sense to
> > move it somewhere else, could you hint me about which you would think it
> > fit better in?
> > 
> > In the mean time, here is a patch which should get the situation already
> > much cleaner. It has been tested on various configs (with and without
> > DSDT). Let me know if you think it is acceptable.
> > 
> > See you,
> > 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). Similarly, use kfree() instead of ACPI_FREE().
> > 
> > Also, it's recommended to open the file before stating it, to avoid surprises.
> > ---
> >  drivers/acpi/osl.c |   33 +++++++++++++++------------------
> >  1 files changed, 15 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 34b3386..b836305 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -42,6 +42,7 @@
> >  #include <acpi/acpi_bus.h>
> >  #include <acpi/processor.h>
> >  #include <asm/uaccess.h>
> > +#include <linux/syscalls.h>
> >  
> >  #include <linux/efi.h>
> >  #include <linux/ioport.h>
> > @@ -327,8 +328,7 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
> >  #ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
> >  static struct acpi_table_header *acpi_find_dsdt_initrd(void)
> >  {
> > -	struct file *firmware_file;
> > -	mm_segment_t oldfs;
> > +	int fd;
> >  	unsigned long len, len2;
> >  	struct acpi_table_header *dsdt_buffer, *ret = NULL;
> >  	struct kstat stat;
> > @@ -342,20 +342,21 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
> >  	 * 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;
> > +	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 PREFIX "Failed to stat %s.\n", ramfs_dsdt_name);
> > +		goto err;
> > +	}
> >  
> >  	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;
> > +		goto err;
> >  	}
> >  
> >  	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
> > @@ -364,15 +365,11 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
> >  		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);
> > +	len2 = sys_read(fd, (char __user *)dsdt_buffer, len);
> >  	if (len2 < len) {
> >  		printk(KERN_ERR PREFIX "Failed to read %lu bytes from %s.\n",
> >  			len, ramfs_dsdt_name);
> > -		ACPI_FREE(dsdt_buffer);
> > +		kfree(dsdt_buffer);
> >  		goto err;
> >  	}
> >  
> > @@ -380,7 +377,7 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
> >  			len, ramfs_dsdt_name);
> >  	ret = dsdt_buffer;
> >  err:
> > -	filp_close(firmware_file, NULL);
> > +	sys_close(fd);
> >  	return ret;
> >  }
> >  #endif
> > 

Re: acpi dsts loading and populate_rootfs

[Andi Kleen]

> Which is probably a good idea.
> AFAIK Numa, possibly Apic tables must be available quite early.

NUMA setup does not use the DSDT, but separate special tables
(SRAT/SLIT). It also doesn't require the ACPI interpreter, these
are all simple static tables.

-Andi

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

[Éric Piel]

21/02/08 19:46, Éric Piel wrote/a écrit:
> In the mean time, here is a patch which should get the situation already
> much cleaner. It has been tested on various configs (with and without
> DSDT). Let me know if you think it is acceptable.
> 
It seems the patch looks ok for people around so here is a s-o-b version 
for Len. It's against 2.6.25-rc2.

Eric

--

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().

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

Signed-off-by: Eric Piel <eric.piel@tremplin-utc.net>
---
 drivers/acpi/osl.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 34b3386..b836305 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -42,6 +42,7 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/processor.h>
 #include <asm/uaccess.h>
+#include <linux/syscalls.h>
 
 #include <linux/efi.h>
 #include <linux/ioport.h>
@@ -327,8 +328,7 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
 #ifdef CONFIG_ACPI_CUSTOM_DSDT_INITRD
 static struct acpi_table_header *acpi_find_dsdt_initrd(void)
 {
-	struct file *firmware_file;
-	mm_segment_t oldfs;
+	int fd;
 	unsigned long len, len2;
 	struct acpi_table_header *dsdt_buffer, *ret = NULL;
 	struct kstat stat;
@@ -342,20 +342,21 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 	 * 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;
+	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 PREFIX "Failed to stat %s.\n", ramfs_dsdt_name);
+		goto err;
+	}
 
 	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;
+		goto err;
 	}
 
 	dsdt_buffer = kmalloc(len, GFP_ATOMIC);
@@ -364,15 +365,11 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 		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);
+	len2 = sys_read(fd, (char __user *)dsdt_buffer, len);
 	if (len2 < len) {
 		printk(KERN_ERR PREFIX "Failed to read %lu bytes from %s.\n",
 			len, ramfs_dsdt_name);
-		ACPI_FREE(dsdt_buffer);
+		kfree(dsdt_buffer);
 		goto err;
 	}
 
@@ -380,7 +377,7 @@ struct acpi_table_header *acpi_find_dsdt_initrd(void)
 			len, ramfs_dsdt_name);
 	ret = dsdt_buffer;
 err:
-	filp_close(firmware_file, NULL);
+	sys_close(fd);
 	return ret;
 }
 #endif

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

[Linus Torvalds]

On Sat, 23 Feb 2008, Éric Piel 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. 

I *also* think this patch is just fundamentally broken regardless. You 
can't just cast a kernel pointer to (char __user *). The two can be in 
totally different address spaces. There's a reason why it used to do that 
whole dance with oldfs.

[ Of course, on platforms where ACPI actually runs this is not the case, 
  but that is not an excuse for doing things wrong ]

So I would NAK this patch - it looks buggy, and the reason for it is not 
sufficiently explained in the first place.

		Linus

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

[Christoph Hellwig]

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.

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@tremplin-utc.net>
---
 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