autotest: Add USB autosuspend whitelist smarts to power_x86S... [chromiumos/third_party/autotest : master]

[Todd Broch (Code Review)]

Todd Broch has uploaded a new change for review.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

autotest: Add USB autosuspend whitelist smarts to power_x86Settings.

CrOS only enables USB autosuspend on built-in USB devices.
power_x86Settings presently checks all USB devices for this being
enabled. This CL adds smarts to evaluate the whitelist file and only
check the built-in USB devices.

It also pork barrels a slight increase in the RAPL PP0 number from 1W
to 1.2W as I was able to get this to fail sporadically when it was
only 1W

BUG=chromium-os:34876
TEST=run test on several x86 platforms

Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
---
M client/site_tests/power_x86Settings/power_x86Settings.py
1 file changed, 42 insertions(+), 4 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/autotest refs/changes/97/37797/1
--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

[Sameer Nanda (Code Review)]

Sameer Nanda has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 1: (4 inline comments)

....................................................
File client/site_tests/power_x86Settings/power_x86Settings.py
Line 198: out = utils.system_output(cmd, ignore_status=True)
probably better to use python's file read functions instead of shell commands?

Line 199: logging.debug("USB whitelist = %s", out)
nit: single quotes instead of double for consistency with rest of file.

Line 204: if re.search(re_str, match_str):
this seems like an exact match search. It is possible to specify wildcard matches (for example 1076:7e*) in the usb-autosuspend.conf file.

Line 230: whitelisted = self._usb_device_is_whitelisted(vid, pid)
while time is not an issue with these tests, it might be cleaner to read & parse the board-specific usb-autosuspend.conf file once outside of the for loop.

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

Gerrit-Reviewer: Benson Leung <ble...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Sameer Nanda <sna...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 1: (4 inline comments)

....................................................
File client/site_tests/power_x86Settings/power_x86Settings.py
Line 198: out = utils.system_output(cmd, ignore_status=True)

Given the complication of parsing bash ... I thought replicating what laptop-mode-tools must do with these files (sourcing) would be the least error prone. For example if one was to add conditionals to the assignment parsing would get more complicated.

Looked into python attempts to _parse_ other scripts and only came up w/ ConfigParser module which didn't fit.

Line 199: logging.debug("USB whitelist = %s", out)

Done

Line 204: if re.search(re_str, match_str):

Done

Line 230: whitelisted = self._usb_device_is_whitelisted(vid, pid)

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

Gerrit-Reviewer: Benson Leung <ble...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Sameer Nanda <sna...@chromium.org>

Gerrit-Reviewer: Todd Broch <tbr...@chromium.org>

[Sameer Nanda (Code Review)]

Sameer Nanda has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File client/site_tests/power_x86Settings/power_x86Settings.py
Line 109: May be regular expressions. See LMTfor details.
nit: missing space between LMT and for.

Line 253: dirpath)
should this be info instead of warn? Warnings would raise concern and will always happen during test runs (USB network dongles).

Line 257: if out != expected_state and whitelisted:
Hmm, this checks seems a bit loose. Should "and out == 'on'" be added here for safety?

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File client/site_tests/power_x86Settings/power_x86Settings.py
Line 109: May be regular expressions. See LMTfor details.

Done

Line 253: dirpath)
Done

Line 257: if out != expected_state and whitelisted:

Agreed ... made it not self._on_ac as thats what LMT is using to decide currently.

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

[Sameer Nanda (Code Review)]

Sameer Nanda has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 3: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: autotest: Add USB autosuspend whitelist smarts to power_x86Settings.
......................................................................

Patch Set 3: Verified; Ready

--
To view, visit https://gerrit.chromium.org/gerrit/37797
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50c56c0082a0651e9ab02300c537a69510b3f100
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Todd Broch <tbr...@chromium.org>