CHROMIUMOS: chromeos_acpi: Introduce ability to handle buffers.

drivers/platform/x86/chromeos_acpi.c

Index: drivers/platform/x86/chromeos_acpi.c
diff --git a/drivers/platform/x86/chromeos_acpi.c b/drivers/platform/x86/chromeos_acpi.c
index 49a7633cdbef420abfb9932237518854c21662a5..aa3589f683d778fc11b07ed0a451c888a73be831 100644
--- a/drivers/platform/x86/chromeos_acpi.c
+++ b/drivers/platform/x86/chromeos_acpi.c
@@ -36,6 +36,8 @@
 #include <linux/platform_device.h>
 #include <linux/acpi.h>
 
+#include "chromeos_acpi.h"
+
 MODULE_AUTHOR("Google Inc.");
 MODULE_DESCRIPTION("Chrome OS Extras Driver");
 MODULE_LICENSE("GPL");
@@ -113,13 +115,6 @@ struct chromeos_acpi_dev {
 static struct chromeos_acpi_dev chromeos_acpi = { };
 
 
-/* Values set at probe time */
-int chromeos_acpi_chnv = -1;
-int chromeos_acpi_chsw = -1;
-
-bool chromeos_acpi_available;
-
-
 /*
  * To show attribute value just access the container structure's `value'
  * field.
@@ -334,6 +329,8 @@ static void handle_nested_acpi_package(union acpi_object *po, char *pm,
 		printk(MY_ERR "failed to create group %s.%d\n", pm, instance);
 }
 
+
+#ifdef CONFIG_CHROMEOS

[Olof Johansson, 2011/03/14 21:24:32]

It would be preferred to just change the Kconfig to depend on CONFIG_CHROMEOS,
so ifdefs can be dropped.

[vb, 2011/03/14 22:00:04]

I am not sure I follow: this code is not needed unless CONFIG_CHROMEOS is
enabled. If I do not wrap this function in these #if/#endif there is a warning
of an unused function when compiled without CONFIG_CHROMEOS.

[Olof Johansson, 2011/03/14 22:28:47]

There's not much use in using CONFIG_CHROMEOS_ACPI without having
CONFIG_CHROMEOS enabled. So if you describe that in the Kconfig by making them
appropriately dependent, you can know that you'll always build this with
CONFIG_CHROMEOS on.

[vb, 2011/03/14 22:55:25]

Ah, that's exactly the thing: the case like now, when I am debugging the driver,
I want to decouple it from the rest of the system, so I disable CONFIG_CHROMEOS
and make the driver compile as module.

This way I am not worried about interactions of the half baked driver with the
rest of the system.

 /*
  * handle_single_int() extract a single int value
  *
@@ -356,7 +353,7 @@ static void handle_single_int(union acpi_object *po, int *found)
 		printk(MY_ERR "acpi_object unexpected type %d, expected int\n",
 		       element->type);
 }
-
+#endif
 
 /*
  * handle_acpi_package() create sysfs group including attributes
@@ -393,12 +390,59 @@ static void handle_acpi_package(union acpi_object *po, char *pm)
 			add_sysfs_attribute(attr_value, pm, count, j);
 			break;
 
+		case ACPI_TYPE_BUFFER: {
+			char *base, *p;
+			int i;
+			unsigned room_left;
+			/* Include this many characters per line */
+			unsigned char_per_line = 16;
+			unsigned string_buffer_size =
+				/* three characters to display one byte */
+				element->buffer.length * 3 +
+				/* one newline per line, all rounded up, plust

[Randall Spangler, 2011/03/14 21:22:28]

plust?

[vb, 2011/03/14 22:00:04]

Done.

+				 * extra newline in the end, plus terminating
+				 * zero, hence + 4
+				 */
+				element->buffer.length/char_per_line + 4;
+
+			if (string_buffer_size > sizeof(attr_value)) {
+				p = kzalloc(string_buffer_size, GFP_KERNEL);
+				if (!p) {
+					printk(MY_ERR "out of memory in %s!\n",
+					       __func__);
+					break;
+				}
+			} else {
+				p = attr_value;
+			}
+
+			base = p;
+			room_left = string_buffer_size;
+			for (i = 0; i < element->buffer.length; i++) {
+				int printed;
+				printed = snprintf(p, room_left, " %2.2x",
+						   element->buffer.pointer[i]);
+				room_left -= printed;
+				p += printed;
+				if (((i + 1) % char_per_line) == 0) {

[Randall Spangler, 2011/03/14 21:22:28]

Check for room_left != 0

If somehow printed==room_left, you'd then decrement room_left in here and end up
with wraparound (because it's unsigned)

[vb, 2011/03/14 22:00:04]

Done.

+					room_left--;
+					*p++ = '\n';
+				}
+			}
+			*p++ = '\n';

[Randall Spangler, 2011/03/14 21:22:28]

Same paranoia here - check room_left>=2

[vb, 2011/03/14 22:00:04]

Done.

+			*p++ = '\0';
+			add_sysfs_attribute(base, pm, count, j);
+			if (string_buffer_size > sizeof(attr_value))
+				kfree(p);
+			break;
+		}

[Olof Johansson, 2011/03/14 21:24:32]

It is common practice to pass the binary data over to sysfs registration, and do
the formatting in the sysfs 'show' routine. This way there's no need to keep a
pre-formatted string allocated, etc. Please change this to the same model.

[vb, 2011/03/14 22:00:04]

This would be a much larger change, because as of now the driver does not
communicate the sizes of ACPI method values throughout its code, everything but
the lowest layer operates with zero terminated strings (note the
ACPI_TYPE_INTEGER case above also converts integer to a string at source).

This also makes the show() driver method very simple - it just copies the value
to the user, otherwise it would have to know the structure of the element being
displayed. But this way of doing things does consume some memory, as you point
out

I looked into rearchitecting the module before I made this CL, it would be not a
simple change. I would much rather avoid reworking the driver at this point when
the functionality is needed so urgently.

If you feel strongly about this, I suggest opening a lower priority bug for
reworking this module to meet this requirement - can we do this?

[Olof Johansson, 2011/03/14 22:28:47]

SGTM

[vb, 2011/03/14 22:55:25]

opened 
http://code.google.com/p/chromium-os/issues/detail?id=13091

 		case ACPI_TYPE_PACKAGE:
 			handle_nested_acpi_package(element, pm, count, j);
 			break;
 
 		default:
-			printk(MY_ERR "ignoring type %d\n", element->type);
+			printk(MY_ERR "ignoring type %d (%s)\n",
+			       element->type, pm);
 			break;
 		}
 	}
@@ -433,13 +477,13 @@ static void add_acpi_method(struct acpi_device *device, char *pm)
 		printk(MY_ERR "%s is not a package, ignored\n", pm);
 	else
 		handle_acpi_package(po, pm);
-
+#ifdef CONFIG_CHROMEOS
 	/* Need to export a couple of variables to chromeos.c */
 	if (!strncmp(pm, "CHNV", 4))
 		handle_single_int(po, &chromeos_acpi_chnv);
 	else if (!strncmp(pm, "CHSW", 4))
 		handle_single_int(po, &chromeos_acpi_chsw);
-
+#endif
 	kfree(output.pointer);
 }
 
@@ -482,7 +526,8 @@ static int chromeos_process_mlst(struct acpi_device *device)
 		char method[ACPI_NAME_SIZE + 1];
 
 		if (element->type == ACPI_TYPE_STRING) {
-			copy_size = min(element->string.length, ACPI_NAME_SIZE);
+			copy_size = min(element->string.length,
+					(u32)ACPI_NAME_SIZE);

[Olof Johansson, 2011/03/14 21:24:32]

Is this cast needed?

[vb, 2011/03/14 22:00:04]

yes, for some weird reason min() operates with pointers to its arguments, which
causes a compilation warning as they have different signedness in this case.

[Olof Johansson, 2011/03/14 22:28:47]

Ah, yes, it's trying to be type safe. Ok.

 			memcpy(method, element->string.pointer, copy_size);
 			method[copy_size] = '\0';
 			add_acpi_method(device, method);
@@ -538,8 +583,35 @@ static int __init chromeos_acpi_init(void)
 		return ret;
 	}
 
+#ifdef CONFIG_CHROMEOS
 	chromeos_acpi_available = true;
-
+#endif
 	return 0;
 }
-subsys_initcall(chromeos_acpi_init);
+
+static void chromeos_acpi_exit(void)
+{
+	acpi_bus_unregister_driver(&chromeos_acpi_driver);
+
+	while (chromeos_acpi.groups) {
+		struct acpi_attribute_group *aag;
+		aag = chromeos_acpi.groups;
+		chromeos_acpi.groups = aag->next_acpi_attr_group;
+		sysfs_remove_group(&chromeos_acpi.p_dev->dev.kobj, &aag->ag);
+		kfree(aag);
+	}
+
+	while (chromeos_acpi.attributes) {
+		struct acpi_attribute *aa = chromeos_acpi.attributes;
+		chromeos_acpi.attributes = aa->next_acpi_attr;
+		device_remove_file(&chromeos_acpi.p_dev->dev, &aa->dev_attr);
+		kfree(aa);
+	}
+
+	platform_device_unregister(chromeos_acpi.p_dev);
+	printk(MY_INFO "removed\n");

[Olof Johansson, 2011/03/14 21:24:32]

Shouldn't there be a chromeos_acpi_available=false somewhere here?

[vb, 2011/03/14 22:00:04]

good point, added.

+}
+
+module_init(chromeos_acpi_init);
+module_exit(chromeos_acpi_exit);

[Olof Johansson, 2011/03/14 21:24:32]

You need a MODULE_LICENSE, MODULE_AUTHOR, etc here.

[vb, 2011/03/14 22:00:04]

They are there in the top of the file. You mean that text must be here before or
after these lines?

[Olof Johansson, 2011/03/14 22:28:47]

Oops, missed them. Please move them to the bottom then, it's usually where
they're at on modules. Not sure why we added them at top to start with.

[vb, 2011/03/14 22:55:25]

Done.

+