CHROMIUMOS: chromeos_acpi: Introduce ability to handle buffers.

drivers/platform/x86/chromeos_acpi.c

 /*
  *  chromeos_acpi.c - ChromeOS specific ACPI support
  *
  *
  * Copyright (C) 2011 The Chromium OS Authors
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
  *  the Free Software Foundation; either version 2 of the License, or
  *  (at your option) any later version.
@@ skipping 18 matching lines @@
  * All values are presented in the string form (numbers as decimal values) and
  * can be accessed as the contents of the appropriate read only files in the
  * sysfs directory tree originating in /sys/devices/platform/chromeos_acpi.
  */
 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #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");
 
 #define MY_LOGPREFIX "chromeos_acpi: "
 #define MY_ERR KERN_ERR MY_LOGPREFIX
 #define MY_NOTICE KERN_NOTICE MY_LOGPREFIX
 #define MY_INFO KERN_INFO MY_LOGPREFIX
 
 /* ACPI method name for MLST; the response for this method is a
@@ skipping 57 matching lines @@
  */
 struct chromeos_acpi_dev {
         struct platform_device *p_dev;
         struct acpi_attribute *attributes;
         struct acpi_attribute_group *groups;
 };
 
 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.
  */
 static ssize_t show_acpi_attribute(struct device *dev,
                                    struct device_attribute *attr, char *buf)
 {
         struct acpi_attribute *paa;
 
         paa = container_of(attr, struct acpi_attribute, dev_attr);
@@ skipping 194 matching lines @@
                                element->type);
                         continue;
                 }
                 aag->ag.attrs[j++] = &paa->dev_attr.attr;
         }
 
         if (sysfs_create_group(&chromeos_acpi.p_dev->dev.kobj, &aag->ag))
                 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
  *
  * @po: package contents as returned by ACPI
  * @found:      integer pointer to store the value in
  *
  */
 static void handle_single_int(union acpi_object *po, int *found)
 {
         union acpi_object *element = po->package.elements;
 
         if (!element) {
                 WARN_ON(1);
                 return;
         }
 
         if (element->type == ACPI_TYPE_INTEGER)
                 *found = (int) element->integer.value;
         else
                 printk(MY_ERR "acpi_object unexpected type %d, expected int\n",
                        element->type);
 }
-
+#endif
 
 /*
  * handle_acpi_package() create sysfs group including attributes
  *                       representing an ACPI package.
  *
  * @po: package contents as returned by ACPI
  * @pm: name of the group
  *
  * Scalar objects included in the package get sys fs attributes created for
  * them. Nested packages are passed to a function creating a sys fs group per
@@ skipping 16 matching lines @@
                         break;
 
                 case ACPI_TYPE_STRING:
                         copy_size = min(element->string.length,
                                         sizeof(attr_value) - 1);
                         memcpy(attr_value, element->string.pointer, copy_size);
                         attr_value[copy_size] = '\0';
                         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;
                 }
         }
 }
 
 
 /*
  * add_acpi_method() evaluate an ACPI method and create sysfs attributes.
  *
  * @device: ACPI device
@@ skipping 14 matching lines @@
                 printk(MY_ERR "failed to retrieve %s (%d)\n", pm, status);
                 return;
         }
 
         po = output.pointer;
 
         if (po->type != ACPI_TYPE_PACKAGE)
                 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);
 }
 
 /*
  * chromeos_process_mlst() Evaluate the MLST method and add methods listed
  *                         in the response.
  *
  * @device: ACPI device
  *
  * Returns: 0 if successful, non-zero if error.
@@ skipping 22 matching lines @@
                 kfree(output.pointer);
                 return -EINVAL;
         }
 
         for (j = 0; j < po->package.count; j++) {
                 union acpi_object *element = po->package.elements + j;
                 int copy_size = 0;
                 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);
                 } else {
                         pr_debug(MY_LOGPREFIX "ignoring type %d\n",
                                  element->type);
                 }
         }
 
         kfree(output.pointer);
@@ skipping 35 matching lines @@
         }
 
         ret = acpi_bus_register_driver(&chromeos_acpi_driver);
         if (ret < 0) {
                 printk(MY_ERR "failed to register driver (%d)\n", ret);
                 platform_device_unregister(chromeos_acpi.p_dev);
                 chromeos_acpi.p_dev = NULL;
                 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.

+