A script for accessing reboot info in CMOS.

This is a script which allows accessing a certain byte in
CMOS. See script Doc string for details of operation.

This script is being added in a new directory which will
host  various userland utilities used to communicate with
the firmware.

Tested on a development machine using mock ACPI and NVRAM
files and on ST8 equipped with the BIOS implementing the
ChromeOS ACPI device. Below is the development machine
run:

vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
(utils $) for de in 0 1; do
> for tr in 0 1; do
> for re in 0 1; do
>  echo;
>  echo "re=$re de=$de tr=$tr"
>  ./reboot_mode.py --nvram_file=/tmp/nvram  --acpi_file=/tmp/acpi_file  --de=$de --tr=$tr --re=$re
> ./reboot_mode.py  --acpi_file=/tmp/acpi_file  --nvram_file=/tmp/nvram  --de=$de --tr=$tr --re=$re
> /usr/bin/od -Ax -t x1 -v /tmp/nvram | grep '^000060' | awk '{print $14}'
> done
> done
> done

re=0 de=0 tr=0
reboot_mode.py: no update required
00

re=1 de=0 tr=0
reboot_mode.py: no update required
80

re=0 de=0 tr=1
reboot_mode.py: no update required
20

re=1 de=0 tr=1
reboot_mode.py: no update required
a0

re=0 de=1 tr=0
reboot_mode.py: no update required
40

re=1 de=1 tr=0
reboot_mode.py: no update required
c0

re=0 de=1 tr=1
reboot_mode.py: no update required
60

re=1 de=1 tr=1
reboot_mode.py: no update required
e0

(utils $) ./reboot_mode.py --acpi_file=/tmp/acpi_file   --nvram_file=/tmp/nvram
Current reboot mode settings:
try_firmware_b : 1
recovery       : 1
debug_reset    : 1
(utils $)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[dhendrix, 2010-05-28 02:56:46]

My python-fu is weak, but AFAICT the code looks okay. Let's see if nsanders can
spot any obvious issues.

[dhendrix, 2010-05-28 03:21:40]

http://codereview.chromium.org/2320003/diff/1/2
File src/platform/utils/reboot_mode.py (right):

http://codereview.chromium.org/2320003/diff/1/2#newcode50
src/platform/utils/reboot_mode.py:50: }
It seems to my like these should be mutually exclusive, ie, an enumeration
rather than a bitfield. Perhaps I don't understand the larger picture, but I
don't see how we'd end up with more than one mode set in a given running state.

http://codereview.chromium.org/2320003/diff/1/2#newcode113
src/platform/utils/reboot_mode.py:113: nvram_contents = f.read()
I am not sure off-hand if this will fail gracefully for if you read the RTC
bytes (0-13) -- you may need to seek past those bytes.

Also, I'm not entirely sure what the behavior will be if you read beyond the
size of the NVRAM. This may vary on a per-machine basis, so I do not know if the
kernel driver is smart enough to handle it like an EOF.

In general, I'd recommend only accessing the byte(s) you need rather than
buffering the entire file and indexing into it later.

http://codereview.chromium.org/2320003/diff/1/2#newcode126
src/platform/utils/reboot_mode.py:126: curr_mode_byte =
ord(nvram_contents[rdb_offset])
If the current ReadNvram function proves problematic due to the factors
described above, I'd recommend passing an offset parameter into it and only
reading the rdb_offset byte.

http://codereview.chromium.org/2320003/diff/1/2#newcode132
src/platform/utils/reboot_mode.py:132: old_mode =
ord(nvram_contents[rdb_offset])
same as above.

[vb, 2010-05-28 16:10:16]

Dave, thank you for your comments.

Frankly, I don't think there is a problem reading the entire NVRAM
contents. The f.read() invocation without parameter causes the entire
file to be read. We might want to limit the amount of read data by the
number of bytes up to the byte of interest, or we could seek to it and
read just one byte - I don't think it matters at the low level and
makes any difference. Let's see if Nick has an opinion.

I take it you are content with introducing another portage package for
the userland to firmware interaction utilities, I will add an ebuild
for this package after we are through with the review,

cheers,
/v


On Thu, May 27, 2010 at 8:21 PM,  <dhendrix@chromium.org> wrote:
>
> http://codereview.chromium.org/2320003/diff/1/2
> File src/platform/utils/reboot_mode.py (right):
>
> http://codereview.chromium.org/2320003/diff/1/2#newcode50
> src/platform/utils/reboot_mode.py:50: }
> It seems to my like these should be mutually exclusive, ie, an
> enumeration rather than a bitfield. Perhaps I don't understand the
> larger picture, but I don't see how we'd end up with more than one mode
> set in a given running state.
>
> http://codereview.chromium.org/2320003/diff/1/2#newcode113
> src/platform/utils/reboot_mode.py:113: nvram_contents = f.read()
> I am not sure off-hand if this will fail gracefully for if you read the
> RTC bytes (0-13) -- you may need to seek past those bytes.
>
> Also, I'm not entirely sure what the behavior will be if you read beyond
> the size of the NVRAM. This may vary on a per-machine basis, so I do not
> know if the kernel driver is smart enough to handle it like an EOF.
>
> In general, I'd recommend only accessing the byte(s) you need rather
> than buffering the entire file and indexing into it later.
>
> http://codereview.chromium.org/2320003/diff/1/2#newcode126
> src/platform/utils/reboot_mode.py:126: curr_mode_byte =
> ord(nvram_contents[rdb_offset])
> If the current ReadNvram function proves problematic due to the factors
> described above, I'd recommend passing an offset parameter into it and
> only reading the rdb_offset byte.
>
> http://codereview.chromium.org/2320003/diff/1/2#newcode132
> src/platform/utils/reboot_mode.py:132: old_mode =
> ord(nvram_contents[rdb_offset])
> same as above.
>
> http://codereview.chromium.org/2320003/show
>



-- 
Vadim Bendebury (mailto:vbendeb@google.com)
Google ChromeOS - (650) 253-7486

[Randall Spangler, 2010-05-28 16:45:59]

http://codereview.chromium.org/2320003/diff/1/2
File src/platform/utils/reboot_mode.py (right):

http://codereview.chromium.org/2320003/diff/1/2#newcode50
src/platform/utils/reboot_mode.py:50: }
On 2010/05/28 03:21:40, dhendrix wrote:
> It seems to my like these should be mutually exclusive, ie, an enumeration
> rather than a bitfield. Perhaps I don't understand the larger picture, but I
> don't see how we'd end up with more than one mode set in a given running
state.

We may want to select whether a debug reset uses the normal firmware, firmware
b, or recovery mode.  So debug reset is orthogonal to the other modes.  (i.e., a
good candidate for a separate bit)

Debug reset and try B are mutually exclusive, but changing those to an enum
(00=normal, 01=recovery, 10=try_b, 11=undefined-treat-as-recovery) doesn't save
any bits, and has the same functionality as the two bitfields.

http://codereview.chromium.org/2320003/diff/1/2#newcode112
src/platform/utils/reboot_mode.py:112: f = open(nvram_file, 'r')
Use mode 'rb' for binary.

http://codereview.chromium.org/2320003/diff/1/2#newcode113
src/platform/utils/reboot_mode.py:113: nvram_contents = f.read()
On 2010/05/28 03:21:40, dhendrix wrote:
> I am not sure off-hand if this will fail gracefully for if you read the RTC
> bytes (0-13) -- you may need to seek past those bytes.
> 
> Also, I'm not entirely sure what the behavior will be if you read beyond the
> size of the NVRAM. This may vary on a per-machine basis, so I do not know if
the
> kernel driver is smart enough to handle it like an EOF.
> 
> In general, I'd recommend only accessing the byte(s) you need rather than
> buffering the entire file and indexing into it later.

+1

f.seek(rdb_offset)
f.read(1)

Then nvram_contents is just the one field we care about.

http://codereview.chromium.org/2320003/diff/1/2#newcode143
src/platform/utils/reboot_mode.py:143: raise RebootModeError('no update
required')
This means running the utility twice to set a flag works the first time, then
fails the second time because the flag is already set.  Just return if no update
necessary.

http://codereview.chromium.org/2320003/diff/1/2#newcode145
src/platform/utils/reboot_mode.py:145: f = open(nvram_file, 'r+')
rb+

http://codereview.chromium.org/2320003/diff/1/2#newcode159
src/platform/utils/reboot_mode.py:159: if len(fields_to_set):
Don't need len().  fields_to_set will eval as false if the dict is empty, true
if it contains something.

[vb, 2010-05-28 18:24:11]

guys, I addressed your comments and retested. Verified that in case byte offset
exceeds the /dev/nvram file size, the appropriate error message is printed
(snippet below taken on an ST8):

vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
localhost tmp # cat /tmp/x
1000
localhost tmp #  ./reboot_mode.py --acpi_file=/tmp/x
reboot_mode.py: Failed reading mode byte from /dev/nvram
localhost tmp #  ./reboot_mode.py 
Current reboot mode settings:
try_firmware_b : 0
recovery       : 0
debug_reset    : 0
localhost tmp # 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Please look again.

http://codereview.chromium.org/2320003/diff/1/2
File src/platform/utils/reboot_mode.py (right):

http://codereview.chromium.org/2320003/diff/1/2#newcode50
src/platform/utils/reboot_mode.py:50: }
On 2010/05/28 16:46:00, Randall Spangler wrote:
> On 2010/05/28 03:21:40, dhendrix wrote:
> > It seems to my like these should be mutually exclusive, ie, an enumeration
> > rather than a bitfield. Perhaps I don't understand the larger picture, but I
> > don't see how we'd end up with more than one mode set in a given running
> state.
> 
> We may want to select whether a debug reset uses the normal firmware, firmware
> b, or recovery mode.  So debug reset is orthogonal to the other modes.  (i.e.,
a
> good candidate for a separate bit)
> 
> Debug reset and try B are mutually exclusive, but changing those to an enum
> (00=normal, 01=recovery, 10=try_b, 11=undefined-treat-as-recovery) doesn't
save
> any bits, and has the same functionality as the two bitfields.

Yes, we probably could save some bits by introducing a mix of enums and
dedicated fields, but IMO it's not worth it.

http://codereview.chromium.org/2320003/diff/1/2#newcode112
src/platform/utils/reboot_mode.py:112: f = open(nvram_file, 'r')
On 2010/05/28 16:46:00, Randall Spangler wrote:
> Use mode 'rb' for binary.

Done.

http://codereview.chromium.org/2320003/diff/1/2#newcode113
src/platform/utils/reboot_mode.py:113: nvram_contents = f.read()
On 2010/05/28 16:46:00, Randall Spangler wrote:
> On 2010/05/28 03:21:40, dhendrix wrote:
> > I am not sure off-hand if this will fail gracefully for if you read the RTC
> > bytes (0-13) -- you may need to seek past those bytes.
> > 
> > Also, I'm not entirely sure what the behavior will be if you read beyond the
> > size of the NVRAM. This may vary on a per-machine basis, so I do not know if
> the
> > kernel driver is smart enough to handle it like an EOF.
> > 
> > In general, I'd recommend only accessing the byte(s) you need rather than
> > buffering the entire file and indexing into it later.
> 
> +1
> 
> f.seek(rdb_offset)
> f.read(1)
> 
> Then nvram_contents is just the one field we care about.

Well, I did it the way you guys wanted, but it is a bit of a pain, because
f.seek(X) silently returns if X exceeds the file size  and file is open strictly
for reading.

os.path.getsize(/dev/nvram) returns zero, so there is no easy way to check this
file's size (especially if we want the code work the same for a /dev file and
the mock.

The good thing is that if seek goes beyond the file size, f.read() returns an
empty string, which can be used as an indication that the seek operation failed.

http://codereview.chromium.org/2320003/diff/1/2#newcode126
src/platform/utils/reboot_mode.py:126: curr_mode_byte =
ord(nvram_contents[rdb_offset])
On 2010/05/28 03:21:40, dhendrix wrote:
> If the current ReadNvram function proves problematic due to the factors
> described above, I'd recommend passing an offset parameter into it and only
> reading the rdb_offset byte.

yes, we now deal just with that one byte.

http://codereview.chromium.org/2320003/diff/1/2#newcode132
src/platform/utils/reboot_mode.py:132: old_mode =
ord(nvram_contents[rdb_offset])
On 2010/05/28 03:21:40, dhendrix wrote:
> same as above.

Done.

http://codereview.chromium.org/2320003/diff/1/2#newcode143
src/platform/utils/reboot_mode.py:143: raise RebootModeError('no update
required')
On 2010/05/28 16:46:00, Randall Spangler wrote:
> This means running the utility twice to set a flag works the first time, then
> fails the second time because the flag is already set.  Just return if no
update
> necessary.
> 

changed to exit without error.

http://codereview.chromium.org/2320003/diff/1/2#newcode159
src/platform/utils/reboot_mode.py:159: if len(fields_to_set):
On 2010/05/28 16:46:00, Randall Spangler wrote:
> Don't need len().  fields_to_set will eval as false if the dict is empty, true
> if it contains something.
> 

Done.

[Randall Spangler, 2010-05-28 19:46:40]

lgtm