CHROMIUMOS: chromeos_acpi: Introduce ability to handle buffers.

Introduce the ability to handle ACPI buffers.

ChromeOS needs to be able to receive assorted information
from the BIOS. Passing it in an opaque buffer structure
seems an obvious choice. This change adds the ability
to handle ACPI buffers in chromeos_acpi driver.

It turned out that chromeos_acpi can not be converted into a 
removable module anymore, which made debugging difficult
(full kernel recompilation needed, if something goes wrong -
the machine is hosed, etc.)

This change restores the option of building chromeos_acpi as 
a module, maintaining status quo (it is built in by
default). chromeos_acpi_exit() is copied from the original
implementation.

chromeos_acpi driver is architectured such that ACPI object
values are represented as strings. This addition converts
converts the binary buffer contents into a multiline string
(16 bytes representes in %.2.x format in each) line.


Change-Id: I8db12454e48db5dddb56a11836ad2d8fd8b6d7bf
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>;

BUG=chromium-os:13069, chromium-os:13091
TEST=manual

For two cases, when chromeos_acpi is configured as a module
and not:

- build system image, modify it for test
- install it on an target with BIOS supplying a
  chromeos ACPI buffer object
- restart the machine
- observe the contents of the VDAT object:

localhost tmp # cat /sys/devices/platform/chromeos_acpi/VDAT
 11 22 33 44 f7 1f 1c 40 96 57 74 41 cc dd ee ff


In case it is configured as a module:

- observe that chromeos_acpi can be removed/reinstalled:
localhost tmp # rmmod chromeos_acpi
localhost tmp # modprobe chromeos_acpi
localhost tmp # rmmod chromeos_acpi
localhost tmp # modprobe chromeos_acpi

- temporarily modify the module to print 5 bytes per string
- try installing the changed module and examine VDAT again:
localhost tmp # cat /sys/devices/platform/chromeos_acpi/VDAT
 11 22 33 44 f7
 1f 1c 40 96 57
 74 41 cc dd ee
 ff

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=9e2f48b

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

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
File drivers/platform/x86/chromeos_acpi.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:402: /* one newline per line, all rounded
up, plust
plust?

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:427: if (((i + 1) % char_per_line) == 0) {
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)

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:432: *p++ = '\n';
Same paranoia here - check room_left>=2

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

Hi, comments below.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/Kconfig
File drivers/platform/x86/Kconfig (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/Kconfig...
drivers/platform/x86/Kconfig:465: default y
New config options should be n by default (i.e. omit a default value). Our
configs will select them.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeos.c
File drivers/platform/x86/chromeos.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos.c:78: " Can't write to nvram\n");
%s  with __func__ allows this to still be a one-line string.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
File drivers/platform/x86/chromeos_acpi.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:333: #ifdef CONFIG_CHROMEOS
It would be preferred to just change the Kconfig to depend on CONFIG_CHROMEOS,
so ifdefs can be dropped.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:438: }
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.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:530: (u32)ACPI_NAME_SIZE);
Is this cast needed?

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:612: printk(MY_INFO "removed\n");
Shouldn't there be a chromeos_acpi_available=false somewhere here?

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:616: module_exit(chromeos_acpi_exit);
You need a MODULE_LICENSE, MODULE_AUTHOR, etc here.

http://codereview.chromium.org/6690023/diff/4001/include/linux/chromeos_platf...
File include/linux/chromeos_platform.h (right):

http://codereview.chromium.org/6690023/diff/4001/include/linux/chromeos_platf...
include/linux/chromeos_platform.h:26: #include <linux/errno.h>
Please add this to the top instead.

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

Guys, I am about to retest it, please have another look.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/Kconfig
File drivers/platform/x86/Kconfig (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/Kconfig...
drivers/platform/x86/Kconfig:465: default y
On 2011/03/14 21:24:32, Olof Johansson wrote:
> New config options should be n by default (i.e. omit a default value). Our
> configs will select them.

Done.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeos.c
File drivers/platform/x86/chromeos.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos.c:78: " Can't write to nvram\n");
On 2011/03/14 21:24:32, Olof Johansson wrote:
> %s  with __func__ allows this to still be a one-line string.

Done.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
File drivers/platform/x86/chromeos_acpi.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:333: #ifdef CONFIG_CHROMEOS
On 2011/03/14 21:24:32, Olof Johansson wrote:
> It would be preferred to just change the Kconfig to depend on CONFIG_CHROMEOS,
> so ifdefs can be dropped.

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.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:402: /* one newline per line, all rounded
up, plust
On 2011/03/14 21:22:28, Randall Spangler wrote:
> plust?

Done.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:427: if (((i + 1) % char_per_line) == 0) {
On 2011/03/14 21:22:28, Randall Spangler wrote:
> 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)

Done.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:432: *p++ = '\n';
On 2011/03/14 21:22:28, Randall Spangler wrote:
> Same paranoia here - check room_left>=2

Done.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:438: }
On 2011/03/14 21:24:32, Olof Johansson wrote:
> 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.

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?

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:530: (u32)ACPI_NAME_SIZE);
On 2011/03/14 21:24:32, Olof Johansson wrote:
> Is this cast needed?

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.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:612: printk(MY_INFO "removed\n");
On 2011/03/14 21:24:32, Olof Johansson wrote:
> Shouldn't there be a chromeos_acpi_available=false somewhere here?
good point, added.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:616: module_exit(chromeos_acpi_exit);
On 2011/03/14 21:24:32, Olof Johansson wrote:
> You need a MODULE_LICENSE, MODULE_AUTHOR, etc here.

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:46]

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
File drivers/platform/x86/chromeos_acpi.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:333: #ifdef CONFIG_CHROMEOS
On 2011/03/14 22:00:04, vb wrote:
> On 2011/03/14 21:24:32, Olof Johansson wrote:
> > It would be preferred to just change the Kconfig to depend on
CONFIG_CHROMEOS,
> > so ifdefs can be dropped.
> 
> 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.

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.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:438: }
SGTM

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:530: (u32)ACPI_NAME_SIZE);
Ah, yes, it's trying to be type safe. Ok.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:616: module_exit(chromeos_acpi_exit);
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]

PLA

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
File drivers/platform/x86/chromeos_acpi.c (right):

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:333: #ifdef CONFIG_CHROMEOS
On 2011/03/14 22:28:47, Olof Johansson wrote:
> On 2011/03/14 22:00:04, vb wrote:
> > On 2011/03/14 21:24:32, Olof Johansson wrote:
> > > It would be preferred to just change the Kconfig to depend on
> CONFIG_CHROMEOS,
> > > so ifdefs can be dropped.
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:438: }
On 2011/03/14 22:28:47, Olof Johansson wrote:
> SGTM
opened 
http://code.google.com/p/chromium-os/issues/detail?id=13091

http://codereview.chromium.org/6690023/diff/4001/drivers/platform/x86/chromeo...
drivers/platform/x86/chromeos_acpi.c:616: module_exit(chromeos_acpi_exit);
On 2011/03/14 22:28:47, Olof Johansson wrote:
> 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.

Done.

[Randall Spangler, 2011-03-15 01:28:27]

LGTM

[Olof Johansson, 2011-03-15 17:05:09]

One nit left, please fix that before pushing but otherwise LGTM.

http://codereview.chromium.org/6690023/diff/9006/include/linux/chromeos_platf...
File include/linux/chromeos_platform.h (right):

http://codereview.chromium.org/6690023/diff/9006/include/linux/chromeos_platf...
include/linux/chromeos_platform.h:26: #include <linux/errno.h>
All includes should be done at the top of the file, please move this up.