[PATCH, RFC] [1/3] Generic in-kernel AC status

[PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

The included patch adds support for power management methods to register 
callbacks in order to allow drivers to check if the system is on AC or 
not. Following patches add support to ACPI and APM. Feedback welcome.

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5be87ba..59ece0f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -129,6 +129,8 @@ struct pm_ops {
 
 extern void pm_set_ops(struct pm_ops *);
 extern struct pm_ops *pm_ops;
+extern void pm_set_ac_status(int (*ac_status_function)(void));
+extern int pm_get_ac_status(void);
 extern int pm_suspend(suspend_state_t state);
 
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 46110d5..a1daafb 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -25,6 +25,7 @@
 DECLARE_MUTEX(pm_sem);
 
 struct pm_ops *pm_ops = NULL;
+int (*get_ac_status)(void);
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
@@ -39,6 +40,39 @@ void pm_set_ops(struct pm_ops * ops)
 	up(&pm_sem);
 }
 
+/**
+ *	pm_set_ac_status - Set the ac status callback
+ *	@ops:	Pointer to function
+ */
+
+void pm_set_ac_status(int (*ac_status_function)(void))
+{
+	down(&pm_sem);
+	get_ac_status = ac_status_function;
+	up(&pm_sem);
+}
+
+EXPORT_SYMBOL(pm_set_ac_status);
+
+/**
+ *	pm_get_ac_status - return true if the system is on AC
+ */
+
+int pm_get_ac_status(void)
+{
+	int status;
+
+	down (&pm_sem);
+	if (get_ac_status)
+		status = get_ac_status();
+	else
+		status = 0;
+	up (&pm_sem);
+
+	return status;
+}
+
+EXPORT_SYMBOL(pm_get_ac_status);
 
 /**
  *	suspend_prepare - Do prep work before entering low-power state.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH, RFC] [2/3] ACPI support for generic in-kernel AC status

[Matthew Garrett]

The included patch adds support for the generic in-kernel AC status 
code. Each AC adapter device is added to a list - when queried, if at 
least one claims to be online then we assume that the system is on AC.

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 61f95ce..aa2d9b9 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -29,6 +29,7 @@
 #include <linux/types.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/pm.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 
@@ -67,8 +68,11 @@ static struct acpi_driver acpi_ac_driver
 struct acpi_ac {
 	acpi_handle handle;
 	unsigned long state;
+	struct list_head entry;
 };
 
+static struct list_head ac_adapter_list;
+
 static struct file_operations acpi_ac_fops = {
 	.open = acpi_ac_open_fs,
 	.read = seq_read,
@@ -255,6 +259,8 @@ static int acpi_ac_add(struct acpi_devic
 		goto end;
 	}
 
+	list_add(&ac->entry, &ac_adapter_list);
+
 	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
 	       ac->state ? "on-line" : "off-line");
@@ -288,17 +294,34 @@ static int acpi_ac_remove(struct acpi_de
 
 	acpi_ac_remove_fs(device);
 
+	list_del(&ac->entry);
+
 	kfree(ac);
 
 	return_VALUE(0);
 }
 
+static int acpi_ac_online_status(void)
+{
+	struct acpi_ac *adapter;
+	
+	list_for_each_entry(adapter, &ac_adapter_list, entry) {
+		acpi_ac_get_state(adapter);
+		if (adapter->state)
+			return 1;
+	}
+
+	return 0;
+}
+
 static int __init acpi_ac_init(void)
 {
 	int result = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ac_init");
 
+	INIT_LIST_HEAD(&ac_adapter_list);
+
 	acpi_ac_dir = proc_mkdir(ACPI_AC_CLASS, acpi_root_dir);
 	if (!acpi_ac_dir)
 		return_VALUE(-ENODEV);
@@ -310,6 +333,8 @@ static int __init acpi_ac_init(void)
 		return_VALUE(-ENODEV);
 	}
 
+	pm_set_ac_status(acpi_ac_online_status);
+
 	return_VALUE(0);
 }
 
@@ -317,6 +342,8 @@ static void __exit acpi_ac_exit(void)
 {
 	ACPI_FUNCTION_TRACE("acpi_ac_exit");
 
+	pm_set_ac_status(NULL);
+
 	acpi_bus_unregister_driver(&acpi_ac_driver);
 
 	remove_proc_entry(ACPI_AC_CLASS, acpi_root_dir);

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

Whoops - missing <linux/module.h>. Here's the fixed version.

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5be87ba..59ece0f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -129,6 +129,8 @@ struct pm_ops {
 
 extern void pm_set_ops(struct pm_ops *);
 extern struct pm_ops *pm_ops;
+extern void pm_set_ac_status(int (*ac_status_function)(void));
+extern int pm_get_ac_status(void);
 extern int pm_suspend(suspend_state_t state);
 
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 46110d5..453cd0e 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -15,7 +15,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/pm.h>
-
+#include <linux/module.h>
 
 #include "power.h"
 
@@ -25,6 +25,7 @@
 DECLARE_MUTEX(pm_sem);
 
 struct pm_ops *pm_ops = NULL;
+int (*get_ac_status)(void);
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
@@ -39,6 +40,39 @@ void pm_set_ops(struct pm_ops * ops)
 	up(&pm_sem);
 }
 
+/**
+ *	pm_set_ac_status - Set the ac status callback
+ *	@ops:	Pointer to function
+ */
+
+void pm_set_ac_status(int (*ac_status_function)(void))
+{
+	down(&pm_sem);
+	get_ac_status = ac_status_function;
+	up(&pm_sem);
+}
+
+EXPORT_SYMBOL(pm_set_ac_status);
+
+/**
+ *	pm_get_ac_status - return true if the system is on AC
+ */
+
+int pm_get_ac_status(void)
+{
+	int status;
+
+	down (&pm_sem);
+	if (get_ac_status)
+		status = get_ac_status();
+	else
+		status = 0;
+	up (&pm_sem);
+
+	return status;
+}
+
+EXPORT_SYMBOL(pm_get_ac_status);
 
 /**
  *	suspend_prepare - Do prep work before entering low-power state.


-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH, RFC] [3/3] APM support for generic in-kernel AC status

[Matthew Garrett]

This adds APM support for the generic in-kernel AC status code.

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c
index 6c8e483..e63f533 100644
--- a/arch/i386/kernel/apm.c
+++ b/arch/i386/kernel/apm.c
@@ -1627,6 +1627,18 @@ static int do_open(struct inode * inode,
 	return 0;
 }
 
+static int apm_get_online_status(void)
+{
+	unsigned short bx;
+	unsigned short cx;
+	unsigned short dx;
+
+	if (apm_get_power_status(&bx, &cx, &dx))
+		return 0;
+	
+	return ((bx >> 8) & 0xff);
+};
+
 static int apm_get_info(char *buf, char **start, off_t fpos, int length)
 {
 	char *		p;
@@ -2372,6 +2384,8 @@ static int __init apm_init(void)
 
 	misc_register(&apm_device);
 
+	pm_set_ac_status(apm_get_online_status);
+
 	if (HZ != 100)
 		idle_period = (idle_period * HZ) / 100;
 	if (idle_threshold < 100) {
@@ -2396,6 +2410,9 @@ static void __exit apm_exit(void)
 		 */
 		cpu_idle_wait();
 	}
+
+	pm_set_ac_status(NULL);
+
 	if (((apm_info.bios.flags & APM_BIOS_DISENGAGED) == 0)
 	    && (apm_info.connection_version > 0x0100)) {
 		error = apm_engage_power_management(APM_DEVICE_ALL, 0);

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Greg KH]

On Wed, Feb 08, 2006 at 01:04:22PM +0000, Matthew Garrett wrote:
> +/**
> + *	pm_set_ac_status - Set the ac status callback
> + *	@ops:	Pointer to function
> + */
> +
> +void pm_set_ac_status(int (*ac_status_function)(void))

No extra line in there please.
And perhaps a bit more description of what this is used for?

> +{
> +	down(&pm_sem);

Shouldn't this be a mutex?

> +	get_ac_status = ac_status_function;
> +	up(&pm_sem);
> +}
> +
> +EXPORT_SYMBOL(pm_set_ac_status);

No extra line between the function and the EXPORT_SYMBOL please.

Also, how about EXPORT_SYMBOL_GPL()?

And, who will be using this interface, and what for?



> +
> +/**
> + *	pm_get_ac_status - return true if the system is on AC
> + */
> +
> +int pm_get_ac_status(void)
> +{
> +	int status;
> +
> +	down (&pm_sem);
> +	if (get_ac_status)
> +		status = get_ac_status();
> +	else
> +		status = 0;
> +	up (&pm_sem);

You can save 2 lines by setting status = 0 on the first line of this
function :)

thanks,

greg k-h

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

On Wed, Feb 08, 2006 at 08:58:03AM -0800, Greg KH wrote:
> > +void pm_set_ac_status(int (*ac_status_function)(void))
> 
> No extra line in there please.

That matches the style used in the rest of the file.

> And perhaps a bit more description of what this is used for?

Ok.

> > +{
> > +	down(&pm_sem);
> 
> Shouldn't this be a mutex?

It is, isn't it? The name's somewhat misleading, but I was just using 
what already existed...

> > +	get_ac_status = ac_status_function;
> > +	up(&pm_sem);
> > +}
> > +
> > +EXPORT_SYMBOL(pm_set_ac_status);
> 
> No extra line between the function and the EXPORT_SYMBOL please.

Sure.

> Also, how about EXPORT_SYMBOL_GPL()?

If that's considered reasonable, sure.

> And, who will be using this interface, and what for?

The backlight interface only supports exporting and setting the current 
brightness. For various bits of hardware, the AC and DC brightnesses are 
stored separately. Drivers would need to know which brightness value to 
export to userspace. I have an HP backlight driver here which would 
benefit from this, and I'm looking at the same issue for a Panasonic 
one.

Here's an updated version, anyway.

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5be87ba..59ece0f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -129,6 +129,8 @@ struct pm_ops {
 
 extern void pm_set_ops(struct pm_ops *);
 extern struct pm_ops *pm_ops;
+extern void pm_set_ac_status(int (*ac_status_function)(void));
+extern int pm_get_ac_status(void);
 extern int pm_suspend(suspend_state_t state);
 
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 46110d5..929a3c0 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -15,7 +15,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/pm.h>
-
+#include <linux/module.h>
 
 #include "power.h"
 
@@ -25,6 +25,7 @@
 DECLARE_MUTEX(pm_sem);
 
 struct pm_ops *pm_ops = NULL;
+int (*get_ac_status)(void);
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
@@ -39,6 +40,36 @@ void pm_set_ops(struct pm_ops * ops)
 	up(&pm_sem);
 }
 
+/**
+ *	pm_set_ac_status - Set the ac status callback. Returns true if the
+ *                         system is on AC and has a registered callback.
+ *	@ops:	Pointer to function
+ */
+
+void pm_set_ac_status(int (*ac_status_function)(void))
+{
+	down(&pm_sem);
+	get_ac_status = ac_status_function;
+	up(&pm_sem);
+}
+EXPORT_SYMBOL_GPL(pm_set_ac_status);
+
+/**
+ *	pm_get_ac_status - return true if the system is on AC
+ */
+
+int pm_get_ac_status(void)
+{
+	int status=0;
+
+	down (&pm_sem);
+	if (get_ac_status)
+		status = get_ac_status();
+	up (&pm_sem);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(pm_get_ac_status);
 
 /**
  *	suspend_prepare - Do prep work before entering low-power state.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Greg KH]

On Wed, Feb 08, 2006 at 05:08:58PM +0000, Matthew Garrett wrote:
> On Wed, Feb 08, 2006 at 08:58:03AM -0800, Greg KH wrote:
> > > +{
> > > +	down(&pm_sem);
> > 
> > Shouldn't this be a mutex?
> 
> It is, isn't it? The name's somewhat misleading, but I was just using 
> what already existed...

Ah, ok, nevermind, I thought this was new.

> +/**
> + *	pm_set_ac_status - Set the ac status callback. Returns true if the
> + *                         system is on AC and has a registered callback.

kerneldoc will not work with this.  It needs to be a one line, short,
description.  Put the full description below the function paramaters, it
can be as many lines as you want there.

thanks,

greg k-h

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

On Wed, Feb 08, 2006 at 09:16:49AM -0800, Greg KH wrote:

> kerneldoc will not work with this.  It needs to be a one line, short,
> description.  Put the full description below the function paramaters, it
> can be as many lines as you want there.

Ok, how's this one?

Signed-Off-By: Matthew Garrett <mjg59@srcf.ucam.org>

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5be87ba..59ece0f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -129,6 +129,8 @@ struct pm_ops {
 
 extern void pm_set_ops(struct pm_ops *);
 extern struct pm_ops *pm_ops;
+extern void pm_set_ac_status(int (*ac_status_function)(void));
+extern int pm_get_ac_status(void);
 extern int pm_suspend(suspend_state_t state);
 
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 46110d5..31745fb 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -15,7 +15,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/pm.h>
-
+#include <linux/module.h>
 
 #include "power.h"
 
@@ -25,6 +25,7 @@
 DECLARE_MUTEX(pm_sem);
 
 struct pm_ops *pm_ops = NULL;
+int (*get_ac_status)(void);
 suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
@@ -39,6 +40,41 @@ void pm_set_ops(struct pm_ops * ops)
 	up(&pm_sem);
 }
 
+/**
+ *	pm_set_ac_status - Set the ac status callback.
+ *	@ops:	Pointer to function
+ *
+ *      Provide a callback that returns true if the system is currently on
+ *      AC power. This should be called by power management subsystems.
+ */
+
+void pm_set_ac_status(int (*ac_status_function)(void))
+{
+	down(&pm_sem);
+	get_ac_status = ac_status_function;
+	up(&pm_sem);
+}
+EXPORT_SYMBOL_GPL(pm_set_ac_status);
+
+/**
+ *	pm_get_ac_status - return true if the system is on AC
+ *
+ *      Returns true if the system has a registered callback for determining 
+ *      AC state and is currently on AC power, false otherwise.
+ */
+
+int pm_get_ac_status(void)
+{
+	int status=0;
+
+	down (&pm_sem);
+	if (get_ac_status)
+		status = get_ac_status();
+	up (&pm_sem);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(pm_get_ac_status);
 
 /**
  *	suspend_prepare - Do prep work before entering low-power state.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Greg KH]

On Wed, Feb 08, 2006 at 05:49:33PM +0000, Matthew Garrett wrote:
> +/**
> + *	pm_set_ac_status - Set the ac status callback.
> + *	@ops:	Pointer to function
> + *
> + *      Provide a callback that returns true if the system is currently on
> + *      AC power. This should be called by power management subsystems.
> + */

Mix of tabs and spaces here.  Just use 1 space after the "*" character.

thanks,

greg k-h

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Gabor Gombas]

On Wed, Feb 08, 2006 at 05:08:58PM +0000, Matthew Garrett wrote:

> The backlight interface only supports exporting and setting the current 
> brightness. For various bits of hardware, the AC and DC brightnesses are 
> stored separately. Drivers would need to know which brightness value to 
> export to userspace. I have an HP backlight driver here which would 
> benefit from this, and I'm looking at the same issue for a Panasonic 
> one.

I don't know the backlight interface but extending it to export all
available brightness values would seem more logical to me.

If I'd had a laptop I'd hate if I could only set the DC brightness if I
plug out the AC power.

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences
     ---------------------------------------------------------

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

On Čt 09-02-06 09:53:44, Gabor Gombas wrote:
> On Wed, Feb 08, 2006 at 05:08:58PM +0000, Matthew Garrett wrote:
> 
> > The backlight interface only supports exporting and setting the current 
> > brightness. For various bits of hardware, the AC and DC brightnesses are 
> > stored separately. Drivers would need to know which brightness value to 
> > export to userspace. I have an HP backlight driver here which would 
> > benefit from this, and I'm looking at the same issue for a Panasonic 
> > one.
> 
> I don't know the backlight interface but extending it to export all
> available brightness values would seem more logical to me.
> 
> If I'd had a laptop I'd hate if I could only set the DC brightness if I
> plug out the AC power.

Still "set current brightness" operation makes a lot of sense.
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Gabor Gombas]

On Fri, Feb 10, 2006 at 01:21:31PM +0100, Pavel Machek wrote:

> Still "set current brightness" operation makes a lot of sense.

Yes, but userspace already knows if you are on AC power or not,
therefore it can decide what "current" means. If this is the only reason
then adding a new kernel infrastructure is overkill.

Also, doing things differently when on AC power smells like a policy
decision, and AFAIK policy handling is not wanted in the kernel.

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences,
     Laboratory of Parallel and Distributed Systems
     Address   : H-1132 Budapest Victor Hugo u. 18-22. Hungary
     Phone/Fax : +36 1 329-78-64 (secretary)
     W3        : http://www.lpds.sztaki.hu
     ---------------------------------------------------------

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

On Fri, Feb 10, 2006 at 02:13:38PM +0100, Gabor Gombas wrote:

> Also, doing things differently when on AC power smells like a policy
> decision, and AFAIK policy handling is not wanted in the kernel.

Backlight drivers are supposed to return the current brightness. With 
some hardware, the only way to do that requires knowing whether the 
system is on AC or not.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

On Pá 10-02-06 14:13:38, Gabor Gombas wrote:
> On Fri, Feb 10, 2006 at 01:21:31PM +0100, Pavel Machek wrote:
> 
> > Still "set current brightness" operation makes a lot of sense.
> 
> Yes, but userspace already knows if you are on AC power or not,
> therefore it can decide what "current" means. If this is the only reason
> then adding a new kernel infrastructure is overkill.

It is not.

powernow-k8 needs to know if ac is plugged, else it can overload
battery.

backlight code needs it for get current brightness.

on Zauruses, battery charging is done in kernel (not in
hardware). Battery charging obviously needs to know if ac is
connected.

on Zauruses, IIRC backlight control code needs to know ac/dc, because
voltage differs on some internal buses and backlight power needs to be
programmed in different way.

> Also, doing things differently when on AC power smells like a policy
> decision, and AFAIK policy handling is not wanted in the kernel.

It is not policy decision, it is protect-hardware-from-damage on
embedded platorms/pn-k8.

-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [linux-pm] Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Gabor Gombas]

On Fri, Feb 10, 2006 at 02:54:15PM +0100, Pavel Machek wrote:

> > Yes, but userspace already knows if you are on AC power or not,
> > therefore it can decide what "current" means. If this is the only reason
> > then adding a new kernel infrastructure is overkill.
> 
> It is not.

Ok, fair enough.

Gabor

-- 
     ---------------------------------------------------------
     MTA SZTAKI Computer and Automation Research Institute
                Hungarian Academy of Sciences,
     Laboratory of Parallel and Distributed Systems
     Address   : H-1132 Budapest Victor Hugo u. 18-22. Hungary
     Phone/Fax : +36 1 329-78-64 (secretary)
     W3        : http://www.lpds.sztaki.hu
     ---------------------------------------------------------

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Philipp Matthias Hahn]

Hi!

On Wed, Feb 08, 2006 at 01:04:22PM +0000, Matthew Garrett wrote:
> diff --git a/include/linux/pm.h b/include/linux/pm.h
...
> +void pm_set_ac_status(int (*ac_status_function)(void))
> +{
> +	down(&pm_sem);
> +	get_ac_status = ac_status_function;
> +	up(&pm_sem);
> +}

Why do you need a semaphore/mutex, when you only do one assignment,
which is atomic by itself?

BYtE
Philipp
-- 
  / /  (_)__  __ ____  __ Philipp Hahn
 / /__/ / _ \/ // /\ \/ /
/____/_/_//_/\_,_/ /_/\_\ pmhahn@titan.lahn.de

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

On St 08-02-06 23:25:32, Philipp Matthias Hahn wrote:
> Hi!
> 
> On Wed, Feb 08, 2006 at 01:04:22PM +0000, Matthew Garrett wrote:
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> ...
> > +void pm_set_ac_status(int (*ac_status_function)(void))
> > +{
> > +	down(&pm_sem);
> > +	get_ac_status = ac_status_function;
> > +	up(&pm_sem);
> > +}
> 
> Why do you need a semaphore/mutex, when you only do one assignment,
> which is atomic by itself?

It is not.
								Pavel

-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Stefan Seyfried]

On Wed, Feb 08, 2006 at 12:57:53PM +0000, Matthew Garrett wrote:
> The included patch adds support for power management methods to register 
> callbacks in order to allow drivers to check if the system is on AC or 
> not. Following patches add support to ACPI and APM. Feedback welcome.

Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
Can't we handles this easily in userspace?
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:
> On Wed, Feb 08, 2006 at 12:57:53PM +0000, Matthew Garrett wrote:
> > The included patch adds support for power management methods to register 
> > callbacks in order to allow drivers to check if the system is on AC or 
> > not. Following patches add support to ACPI and APM. Feedback welcome.
> 
> Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
> Can't we handles this easily in userspace?

Some kernel parts need to now: for example powernow-k8: some
frequencies are not allowed when you are running off battery. [Just
now it is solved by not allowing those frequencies at all unless ACPI
is available; quite an ugly solution.]

								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Stefan Seyfried]

On Fri, Feb 10, 2006 at 01:19:13PM +0100, Pavel Machek wrote:
> On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:

> > Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
> > Can't we handles this easily in userspace?
> 
> Some kernel parts need to now: for example powernow-k8: some

we can tell them from userspace.

> frequencies are not allowed when you are running off battery. [Just
> now it is solved by not allowing those frequencies at all unless ACPI
> is available; quite an ugly solution.]

And this is a reason to include it in the kernel?
Pavel? Is it you? What have they done to you? ;-)

I mean - we are moving suspend and everything to userspace, so i wonder
why this has to be in kernel.
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

On Pá 10-02-06 13:32:59, Stefan Seyfried wrote:
> On Fri, Feb 10, 2006 at 01:19:13PM +0100, Pavel Machek wrote:
> > On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:
> 
> > > Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
> > > Can't we handles this easily in userspace?
> > 
> > Some kernel parts need to now: for example powernow-k8: some
> 
> we can tell them from userspace.

No, because battery will explode if you do not slow cpu down fast
enough. And it would be extremely ugly.

Right now, interfaces we have are:

/proc/apm -- to get AC/DC info on APM

/proc/acpi/ac_adapter/*/state -- to get in on ACPI

...then we have...

in-kernel interface to get AC/DC info on APM, and in-kernel interface
to get AC/DC info on ACPI (powernow-k8 needs it). Brightness needs to
use those interfaces, too.

With your proposed solution, we'd have to add
/proc/powernow_k8/you_are_on_ac and /proc/backlight/you_are_on_ac and
..., or something, along with daemon to poll /proc/apm and
/proc/acpi/*/*/state (and whatever else embedded machines do), and
feed it back to kernel.

That's clearly wrong thing to do. So Matthew patch is actually
cleanup. It should allow us to kill special interfaces and have
something like /sys/power/ac ... and not having to introduce special
interfaces for pn-k8, backlight and more.

(More responses in separate thread.)

> > frequencies are not allowed when you are running off battery. [Just
> > now it is solved by not allowing those frequencies at all unless ACPI
> > is available; quite an ugly solution.]
> 
> And this is a reason to include it in the kernel?
> Pavel? Is it you? What have they done to you? ;-)

Frequency scaling is currently done in kernel, and this is "battery
blows up" issue.

> I mean - we are moving suspend and everything to userspace, so i wonder
> why this has to be in kernel.

Unless you want LZF, encryption, and graphical progress bar, suspend
goes to userspace. But AC/DC knowledge is actually neccessary for
kernel. You can live without it on most PCs (except powernow-k8
machines and notebooks that have braindamaged), but embedded platforms
definitely need it.
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Jan Engelhardt]

>> > > Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
>> > > Can't we handles this easily in userspace?
>> > 
>> > Some kernel parts need to now: for example powernow-k8: some
>> 
>> we can tell them from userspace.
>
>No, because battery will explode if you do not slow cpu down fast
>enough.

How is that? APC UPS don't explode either if they are switching to battery.


Jan Engelhardt
-- 

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Kyle Moffett]

On Feb 12, 2006, at 05:17, Jan Engelhardt wrote:
>>>>> Ok. Maybe i am not seeing the point. But why do we need this in  
>>>>> the kernel?  Can't we handles this easily in userspace?
>>>>
>>>> Some kernel parts need to now: for example powernow-k8: some
>>>
>>> we can tell them from userspace.
>>
>> No, because battery will explode if you do not slow cpu down fast  
>> enough.
>
> How is that? APC UPS don't explode either if they are switching to  
> battery.

Yes, but APC UPS have big 3-inch fans blowing air over the batteries  
to keep them from overheating; laptops dont. Besides which, have you  
ever stuck your hand on a UPS while it's running a server on  
battery?  Those things get damn hot even with the fans, not even  
considering the fact that a lead-acid battery is much more efficient  
about storing and using energy than a Lithium-ion battery.

Incidentally, my UPS thermometer tells me that the peak load on- 
battery temp is more than 120 F; would you _really_ want that sitting  
on your lap in addition to all the heat generated by CPU, ram, GPU, etc?

There are real applications out there that have these problems, in  
fact a lot of manufacturers have issued recalls because of exploding  
batteries, power bricks, etc, indicating that this is not a simple  
problem.

Cheers,
Kyle Moffett

--
Diplomacy involves walking softly and _carrying_ a big stick.   
Actually using the big stick means the diplomacy part failed.
   -- Rob Landley

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Thomas Renninger]

Pavel Machek wrote:
> On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:
>> On Wed, Feb 08, 2006 at 12:57:53PM +0000, Matthew Garrett wrote:
>>> The included patch adds support for power management methods to register 
>>> callbacks in order to allow drivers to check if the system is on AC or 
>>> not. Following patches add support to ACPI and APM. Feedback welcome.
>> Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
>> Can't we handles this easily in userspace?
> 
> Some kernel parts need to now: for example powernow-k8: some
> frequencies are not allowed when you are running off battery. [Just
> now it is solved by not allowing those frequencies at all unless ACPI
> is available; quite an ugly solution.]
> 
Allowed CPUfreqs are exported via _PPC.
This is why a lot hardware sends an ac_adapter and a processor event
when (un)plugging ac adapter.
Limiting cpufreq already works nice that way.

AMD64 laptops are booting with lower freqs per default until they are
pushed up, so there shouldn't be anything critical?

For the brightness part, I don't see any "laptop is going to explode"
issue.
I always hated the brightness going down when I unplugged ac on M$
and had to push ten times the brightness "up button" before I could
go on working...

Shouldn't it be:

               
ac Event   ---> userspace <--- user config
                   |
                   | 
brightness <-------|

Whether the ac brightness will be set when going to ac, or
whatever brightness, should be configurable in userspace IMO.
This is a one liner in the acpid config?

However, I also like the general /sys/../brightness file very much!


     Thomas

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Rafael J. Wysocki]

On Tuesday 14 February 2006 18:44, Thomas Renninger wrote:
> Pavel Machek wrote:
> > On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:
> >> On Wed, Feb 08, 2006 at 12:57:53PM +0000, Matthew Garrett wrote:
> >>> The included patch adds support for power management methods to register 
> >>> callbacks in order to allow drivers to check if the system is on AC or 
> >>> not. Following patches add support to ACPI and APM. Feedback welcome.
> >> Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
> >> Can't we handles this easily in userspace?
> > 
> > Some kernel parts need to now: for example powernow-k8: some
> > frequencies are not allowed when you are running off battery. [Just
> > now it is solved by not allowing those frequencies at all unless ACPI
> > is available; quite an ugly solution.]
> > 
> Allowed CPUfreqs are exported via _PPC.
> This is why a lot hardware sends an ac_adapter and a processor event
> when (un)plugging ac adapter.
> Limiting cpufreq already works nice that way.
> 
> AMD64 laptops are booting with lower freqs per default until they are
> pushed up, so there shouldn't be anything critical?

This is not true as far as my box is concerned (Asus L5D).  It starts with
the _highest_ clock available.

> For the brightness part, I don't see any "laptop is going to explode"
> issue.
> I always hated the brightness going down when I unplugged ac on M$

Currently I have the same problem on Linux, but I don't know the solution
(yet).  Any hints? :-)

Rafael

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Thomas Renninger]

Rafael J. Wysocki wrote:
> On Tuesday 14 February 2006 18:44, Thomas Renninger wrote:
>> Pavel Machek wrote:
>>> On Pá 10-02-06 09:06:43, Stefan Seyfried wrote:
>>>> On Wed, Feb 08, 2006 at 12:57:53PM +0000, Matthew Garrett wrote:
>>>>> The included patch adds support for power management methods to register 
>>>>> callbacks in order to allow drivers to check if the system is on AC or 
>>>>> not. Following patches add support to ACPI and APM. Feedback welcome.
>>>> Ok. Maybe i am not seeing the point. But why do we need this in the kernel?
>>>> Can't we handles this easily in userspace?
>>> Some kernel parts need to now: for example powernow-k8: some
>>> frequencies are not allowed when you are running off battery. [Just
>>> now it is solved by not allowing those frequencies at all unless ACPI
>>> is available; quite an ugly solution.]
>>>
>> Allowed CPUfreqs are exported via _PPC.
>> This is why a lot hardware sends an ac_adapter and a processor event
>> when (un)plugging ac adapter.
>> Limiting cpufreq already works nice that way.
>>
>> AMD64 laptops are booting with lower freqs per default until they are
>> pushed up, so there shouldn't be anything critical?
> 
> This is not true as far as my box is concerned (Asus L5D).  It starts with
> the _highest_ clock available.
Hmm, but then there shouldn't be any critical overheat problems and if,
the hardware has to switch off the machine hard. OS always could freeze,
but the battery must not start to burn...
> 
>> For the brightness part, I don't see any "laptop is going to explode"
>> issue.
>> I always hated the brightness going down when I unplugged ac on M$
> 
> Currently I have the same problem on Linux, but I don't know the solution
> (yet).  Any hints? :-)
Hmm, this is probably done by ACPI in some ac connected function?
Seems as some machines already adjust brightness in ACPI context to the
ac/battery brightness value and some leave it to the OS to adjust.
Override it in /sys/../brightness (as soon as it exists) or the current
vendor specific solution file.
Connect the acpid to a battery ACPI event rule and let override it there.


IMO, the /sys/.../brightness patch should go in as soon as possible, I think
all everybody agrees here?

Maybe I oversaw an issue, but I really don't see a reason for connecting
the brightness to ac in kernel space.
Some weeks later someone likes to have a /sys/../brightness_ignore_ac switch ...

    Thomas

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Matthew Garrett]

On Wed, Feb 15, 2006 at 01:36:59PM +0100, Thomas Renninger wrote:

> Maybe I oversaw an issue, but I really don't see a reason for connecting
> the brightness to ac in kernel space.

The backlight class maintainer specified that /sys/../brightness should 
return the *current* brightness. In some cases that's impossible without 
knowing the ac status.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Thomas Renninger]

Matthew Garrett wrote:
> On Wed, Feb 15, 2006 at 01:36:59PM +0100, Thomas Renninger wrote:
> 
>> Maybe I oversaw an issue, but I really don't see a reason for connecting
>> the brightness to ac in kernel space.
> 
> The backlight class maintainer specified that /sys/../brightness should 
Who's that? Is there more info/spec available?
> return the *current* brightness. In some cases that's impossible without 
> knowing the ac status.
> 
Ok.
The ac/battery status is used to guess the current brightness? Please
do not set suggested values automatically, IMO this is not a nice feature...
I am really looking forward to see a general /sys/../brightness.
Thanks a lot for working on this!
Just an idea for later:
Beside min/max there probably should be a /sys/../brightness_bat 
and /sys/../brightness_ac.
- If ac/battery brightness will not be set by kernel, userspace can write the
  right value to /sys/../brightness and the above two can be readonly.
- If ac/battery brightness will be set by kernel the above two should even be
  writeable so that people can avoid the need of pressing "brightness up"
  button without hacking the kernel.

I vote for letting userspace doing the stuff..., e.g. changing brightness behind
the back of some userspace tool will result in userspace tools starting to
poll.

Comments?

    Thomas

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

Hi!

> >This is not true as far as my box is concerned 
> >(Asus L5D).  It starts with
> >the _highest_ clock available.
> Hmm, but then there shouldn't be any critical 
> overheat problems and if,
> the hardware has to switch off the machine 
> hard. OS always could freeze,
> but the battery must not start to burn...

I told that to hw designers... too late. Fortunately batteries usually
only crash machine if you overload them.

> IMO, the /sys/.../brightness patch should go in 
> as soon as possible, I think
> all everybody agrees here?

Yep.

> Maybe I oversaw an issue, but I really don't 
> see a reason for connecting
> the brightness to ac in kernel space.

We are not going to connect it. But to implement .../brightness, you need to
know ac/battery on several "broken" notebooks.

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH, RFC] [1/3] Generic in-kernel AC status

[Pavel Machek]

Hi!

> > AMD64 laptops are booting with lower freqs per default until they are
> > pushed up, so there shouldn't be anything critical?
> 
> This is not true as far as my box is concerned (Asus L5D).  It starts with
> the _highest_ clock available.

Not all laptops have underpowered batteries. And I have
seen machine that booted fast with underpowered battery...
not nice. Would crash in POST in 30% cases.
> 
> > For the brightness part, I don't see any "laptop is going to explode"
> > issue.
> > I always hated the brightness going down when I unplugged ac on M_
> 
> Currently I have the same problem on Linux, but I don't know the solution
> (yet).  Any hints? :-)

Work around acpi bios... not going to be nice.

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms