ACPI ec.c and ec_sys.c fixes

ACPI ec.c and ec_sys.c fixes

[Thomas Renninger]

These apply on top of Matthew's platform driver linux-next branch.

One hunk of the EC port double registration fix, is a revert of
the patch where the problem got introduced:
git commit (platform driver's linux-next branch):
10da7c1b03753ecc0fe1a34608cc327f8c1c3630
ACPI: Register EC io ports in /proc/ioports

These patches are only needed for linux-next and should go in
through the platform driver's tree.

Thanks,

       Thomas

[PATCH 1/2] acpi ec: Fix possible double io port registration

[Thomas Renninger]

which will result in a harmless but ugly WARN message on
some machines.

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg59@srcf.ucam.org
CC: platform-driver-x86@vger.kernel.org
CC: linux-acpi@vger.kernel.org
CC: astarikovskiy@suse.de
CC: akpm@linux-foundation.org
---
 drivers/acpi/ec.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 265a99c..1fa0aaf 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -818,6 +818,12 @@ static int acpi_ec_add(struct acpi_device *device)
 	if (!first_ec)
 		first_ec = ec;
 	device->driver_data = ec;
+
+	WARN(!request_region(ec->data_addr, 1, "EC data"),
+	     "Could not request EC data io port 0x%lx", ec->data_addr);
+	WARN(!request_region(ec->command_addr, 1, "EC cmd"),
+	     "Could not request EC cmd io port 0x%lx", ec->command_addr);
+
 	pr_info(PREFIX "GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n",
 			  ec->gpe, ec->command_addr, ec->data_addr);
 
@@ -844,6 +850,8 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 		kfree(handler);
 	}
 	mutex_unlock(&ec->lock);
+	release_region(ec->data_addr, 1);
+	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
 	if (ec == first_ec)
 		first_ec = NULL;
@@ -864,18 +872,10 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 	 * the second address region returned is the status/command
 	 * port.
 	 */
-	if (ec->data_addr == 0) {
+	if (ec->data_addr == 0)
 		ec->data_addr = resource->data.io.minimum;
-		WARN(!request_region(ec->data_addr, 1, "EC data"),
-		     "Could not request EC data io port %lu",
-		     ec->data_addr);
-	}
-	else if (ec->command_addr == 0) {
+	else if (ec->command_addr == 0)
 		ec->command_addr = resource->data.io.minimum;
-		WARN(!request_region(ec->command_addr, 1, "EC command"),
-		     "Could not request EC command io port %lu",
-		     ec->command_addr);
-	}
 	else
 		return AE_CTRL_TERMINATE;
 
-- 
1.6.3

[PATCH 2/2] acpi ec_sys: Be more cautious about ec write access

[Thomas Renninger]

- Set Kconfig option default n
- Only allow root to read/write io file (sever bug!)
- Introduce write support module param -> default off
- Properly clean up if any debugfs files cannot be created

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg59@srcf.ucam.org
CC: platform-driver-x86@vger.kernel.org
CC: linux-acpi@vger.kernel.org
CC: astarikovskiy@suse.de
---
 drivers/acpi/Kconfig  |   11 ++++++++---
 drivers/acpi/ec_sys.c |   30 +++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index f7226d1..08e0140 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -106,14 +106,19 @@ config ACPI_SYSFS_POWER
 
 config ACPI_EC_DEBUGFS
 	tristate "EC read/write access through /sys/kernel/debug/ec"
-	default y
+	default n
 	help
 	  Say N to disable Embedded Controller /sys/kernel/debug interface
 
+	  Be aware that using this interface can confuse your Embedded
+	  Controller in a way that a normal reboot is not enough. You then
+	  have to power of your system, and remove the laptop battery for
+	  some seconds.
 	  An Embedded Controller typically is available on laptops and reads
 	  sensor values like battery state and temperature.
-	  The kernel access the EC through ACPI parsed code provided by BIOS
-	  tables.
+	  The kernel accesses the EC through ACPI parsed code provided by BIOS
+	  tables. This option allows to access the EC directly without ACPI
+	  code being involved.
 	  Thus this option is a debug option that helps to write ACPI drivers
 	  and can be used to identify ACPI code or EC firmware bugs.
 
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
index 3ef9781..9a319f3 100644
--- a/drivers/acpi/ec_sys.c
+++ b/drivers/acpi/ec_sys.c
@@ -17,6 +17,11 @@ MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
 MODULE_DESCRIPTION("ACPI EC sysfs access driver");
 MODULE_LICENSE("GPL");
 
+static bool write_support;
+module_param(write_support, bool, 0644);
+MODULE_PARM_DESC(write_support, "Dangerous, reboot and removal of battery may "
+		 "be needed.");
+
 #define EC_SPACE_SIZE 256
 
 struct sysdev_class acpi_ec_sysdev_class = {
@@ -102,6 +107,7 @@ int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
 {
 	struct dentry *dev_dir;
 	char name[64];
+
 	if (ec_device_count == 0) {
 		acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
 		if (!acpi_ec_debugfs_dir)
@@ -111,17 +117,27 @@ int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
 	sprintf(name, "ec%u", ec_device_count);
 	dev_dir = debugfs_create_dir(name, acpi_ec_debugfs_dir);
 	if (!dev_dir) {
-		if (ec_device_count == 0)
-			debugfs_remove_recursive(acpi_ec_debugfs_dir);
-		/* TBD: Proper cleanup for multiple ECs */
+		if (ec_device_count != 0)
+			goto error;
 		return -ENOMEM;
 	}
 
-	debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe);
-	debugfs_create_bool("use_global_lock", 0444, dev_dir,
-			    (u32 *)&first_ec->global_lock);
-	debugfs_create_file("io", 0666, dev_dir, ec, &acpi_ec_io_ops);
+	if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
+		goto error;
+	if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
+				 (u32 *)&first_ec->global_lock))
+		goto error;
+
+	if (!write_support)
+		acpi_ec_io_ops.write = NULL;
+	if (!debugfs_create_file("io", 0600, dev_dir, ec, &acpi_ec_io_ops))
+		goto error;
+
 	return 0;
+
+error:
+	debugfs_remove_recursive(acpi_ec_debugfs_dir);
+	return -ENOMEM;
 }
 
 static int __init acpi_ec_sys_init(void)
-- 
1.6.3

Re: [PATCH 2/2] acpi ec_sys: Be more cautious about ec write access

[Thomas Renninger]

On Thursday 29 July 2010 10:08:45 pm Thomas Renninger wrote:
> +
> +	if (!write_support)
> +		acpi_ec_io_ops.write = NULL;
> +	if (!debugfs_create_file("io", 0600, dev_dir, ec, &acpi_ec_io_ops))
Wait, this is wrong. I send an updated patch in a second...

       Thomas

[PATCH] acpi ec_sys: Be more cautious about ec write access

[Thomas Renninger]

- Set Kconfig option default n
- Only allow root to read/write io file (sever bug!)
- Introduce write support module param -> default off
- Properly clean up if any debugfs files cannot be created

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: mjg59@srcf.ucam.org
CC: platform-driver-x86@vger.kernel.org
CC: linux-acpi@vger.kernel.org
CC: astarikovskiy@suse.de
---
 drivers/acpi/Kconfig  |   11 ++++++++---
 drivers/acpi/ec_sys.c |   31 ++++++++++++++++++++++++-------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index f7226d1..08e0140 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -106,14 +106,19 @@ config ACPI_SYSFS_POWER
 
 config ACPI_EC_DEBUGFS
 	tristate "EC read/write access through /sys/kernel/debug/ec"
-	default y
+	default n
 	help
 	  Say N to disable Embedded Controller /sys/kernel/debug interface
 
+	  Be aware that using this interface can confuse your Embedded
+	  Controller in a way that a normal reboot is not enough. You then
+	  have to power of your system, and remove the laptop battery for
+	  some seconds.
 	  An Embedded Controller typically is available on laptops and reads
 	  sensor values like battery state and temperature.
-	  The kernel access the EC through ACPI parsed code provided by BIOS
-	  tables.
+	  The kernel accesses the EC through ACPI parsed code provided by BIOS
+	  tables. This option allows to access the EC directly without ACPI
+	  code being involved.
 	  Thus this option is a debug option that helps to write ACPI drivers
 	  and can be used to identify ACPI code or EC firmware bugs.
 
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
index 3ef9781..0e869b3 100644
--- a/drivers/acpi/ec_sys.c
+++ b/drivers/acpi/ec_sys.c
@@ -17,6 +17,11 @@ MODULE_AUTHOR("Thomas Renninger <trenn@suse.de>");
 MODULE_DESCRIPTION("ACPI EC sysfs access driver");
 MODULE_LICENSE("GPL");
 
+static bool write_support;
+module_param(write_support, bool, 0644);
+MODULE_PARM_DESC(write_support, "Dangerous, reboot and removal of battery may "
+		 "be needed.");
+
 #define EC_SPACE_SIZE 256
 
 struct sysdev_class acpi_ec_sysdev_class = {
@@ -102,6 +107,8 @@ int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
 {
 	struct dentry *dev_dir;
 	char name[64];
+	mode_t mode = 0400;
+
 	if (ec_device_count == 0) {
 		acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
 		if (!acpi_ec_debugfs_dir)
@@ -111,17 +118,27 @@ int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
 	sprintf(name, "ec%u", ec_device_count);
 	dev_dir = debugfs_create_dir(name, acpi_ec_debugfs_dir);
 	if (!dev_dir) {
-		if (ec_device_count == 0)
-			debugfs_remove_recursive(acpi_ec_debugfs_dir);
-		/* TBD: Proper cleanup for multiple ECs */
+		if (ec_device_count != 0)
+			goto error;
 		return -ENOMEM;
 	}
 
-	debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe);
-	debugfs_create_bool("use_global_lock", 0444, dev_dir,
-			    (u32 *)&first_ec->global_lock);
-	debugfs_create_file("io", 0666, dev_dir, ec, &acpi_ec_io_ops);
+	if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
+		goto error;
+	if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
+				 (u32 *)&first_ec->global_lock))
+		goto error;
+
+	if (write_support)
+		mode = 0600;
+	if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
+		goto error;
+
 	return 0;
+
+error:
+	debugfs_remove_recursive(acpi_ec_debugfs_dir);
+	return -ENOMEM;
 }
 
 static int __init acpi_ec_sys_init(void)
-- 
1.6.3

Re: [PATCH] acpi ec_sys: Be more cautious about ec write access

[Henrique de Moraes Holschuh]

On Thu, 29 Jul 2010, Thomas Renninger wrote:
> - Only allow root to read/write io file (sever bug!)

I'd go further, and only allow CAP_SYS_RAWIO.

> +	  The kernel accesses the EC through ACPI parsed code provided by BIOS
> +	  tables. This option allows to access the EC directly without ACPI
> +	  code being involved.

This is not really true.  Kernel drivers can, and do access the EC without
help from the AML firmware (DSDT, SSDT...).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] acpi ec_sys: Be more cautious about ec write access

[Thomas Renninger]

On Friday 30 July 2010 06:37:17 pm Henrique de Moraes Holschuh wrote:
> On Thu, 29 Jul 2010, Thomas Renninger wrote:
> > - Only allow root to read/write io file (sever bug!)
>
> I'd go further, and only allow CAP_SYS_RAWIO.
I'll have a look and eventually come up with something on-top.

> > +	  The kernel accesses the EC through ACPI parsed code provided by BIOS
> > +	  tables. This option allows to access the EC directly without ACPI
> > +	  code being involved.
>
> This is not really true.  Kernel drivers can, and do access the EC without
> help from the AML firmware (DSDT, SSDT...).
Yes, the native laptop driver hacks which should not exist...
Generally the EC should only be accessed via ACPI interpreted code, is it 
really worth to mention these exceptions at this point?

   Thomas

Re: [PATCH] acpi ec_sys: Be more cautious about ec write access

[Henrique de Moraes Holschuh]

On Sun, 01 Aug 2010, Thomas Renninger wrote:
> On Friday 30 July 2010 06:37:17 pm Henrique de Moraes Holschuh wrote:
> > On Thu, 29 Jul 2010, Thomas Renninger wrote:
> > > - Only allow root to read/write io file (sever bug!)
> >
> > I'd go further, and only allow CAP_SYS_RAWIO.
> I'll have a look and eventually come up with something on-top.
> 
> > > +	  The kernel accesses the EC through ACPI parsed code provided by BIOS
> > > +	  tables. This option allows to access the EC directly without ACPI
> > > +	  code being involved.
> >
> > This is not really true.  Kernel drivers can, and do access the EC without
> > help from the AML firmware (DSDT, SSDT...).
> Yes, the native laptop driver hacks which should not exist...

But which DO exist.  Let's not go there.

> Generally the EC should only be accessed via ACPI interpreted code, is it 
> really worth to mention these exceptions at this point?

Why state incorrect information?  No kernel driver needs this new Kconfig
option to work, it is required only for *userspace* access to the EC
register space.  IMHO, it would better to say this:

"This option allows userspace direct access to the EC registers, for
debugging and kernel driver development purposes".

Which is entirely correct and gets the proper idea of its intended usage
across (it is not intended to be used for userspace drivers, AFAIK).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh