CHROMIUM: thermal: Add debugfs metrics for fan state changes [chromiumos/third_party/kernel : chromeos-3.2]

0 views
Skip to first unread message

Jon Kliegman (Code Review)

unread,
May 8, 2012, 7:20:42 PM5/8/12
to Olof Johansson, Mandeep Singh Baines
Jon Kliegman has uploaded a new change for review.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................

CHROMIUM: thermal: Add debugfs metrics for fan state changes

Gather stats for:
# times a fan failed to turn on due to temperature drop
# times multiple fans triggered at once
# times a fan turned on

BUG=chromium-os:30526
TEST=in dev

Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Signed-off-by: Jonathan Kliegman <kli...@chromium.org>
---
M drivers/thermal/thermal_sys.c
M include/linux/thermal.h
2 files changed, 52 insertions(+), 1 deletion(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/76/22176/1
--
To view, visit https://gerrit.chromium.org/gerrit/22176
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 8, 2012, 7:25:54 PM5/8/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 2:

Did some brief testing:

55000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=0
55000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=0
55000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=1
52000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=1
52000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=1
52000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=1
52000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=0 on=3 mult=1 del=1
51000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=1 on=3 mult=1 del=0
51000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=1 on=3 mult=1 del=0
51000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=1 on=3 mult=1 del=0
51000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=1 on=3 mult=1 del=0
51000: 0=0 1=0 2=0 3=1 4=1 5=0 aborted=1 on=3 mult=1 del=0

Left number is temperature, 0-5 are the fans. For testing I set the delay period to 5 seconds and you can see the aborted count increasing when leaving delay.

For the multiple turnons, there's always 1 at startup it seems - at boot we always turn one fan on (the 0% fan) and we tend to boot up warm enough for the second fan to always trigger so we'll see the early mult=1. One boot cycle I saw aborted=1 at the start too which corresponds to me seeing us occasionally boot up hot.

I'm not sure how to report this all into UMA which is the other related bug so will look into how it does that. If its smart enough to always understand that '1' is the expected value for a machine then we don't need funky logic to ignore the boot case.

I've also chosen to ignore almost all errors with creating debugfs. If it doesn't properly create the files then we just don't have them - system should still run so no point errorring out. I can add log warnings but not sure that gains much and if we're reporting "unknown" to UMA (we should be able to do this?) that will give us information on how often we see errors with this.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>
Gerrit-Reviewer: Sameer Nanda <sna...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 8, 2012, 7:26:21 PM5/8/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 2:

Apologies for the log dump - it looks like gerrit forced it into html and stripped line breaks.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 9, 2012, 11:44:53 AM5/9/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 2: (1 inline comment)

....................................................
File drivers/thermal/thermal_sys.c
Line 1164: if (num_tripped > 1)
Another option is to use cdevs_in_delay > 1 here, but this would include fans that got triggered in a previous poll period so would be a slightly different condition. This one seems slightly more accurate to me, but does add a bit more code.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 9, 2012, 1:52:13 PM5/9/12
to Jon Kliegman, Sameer Nanda, Olof Johansson
Sameer Nanda has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 2: (4 inline comments)

The number of times a boot happens should be vanishingly small compared to number of times the fan on/abort/multi-on events happen. I would, therefore, not worry about special case handling for the fan multi-on boot case.

....................................................
File drivers/thermal/thermal_sys.c
Line 1305: }
nit: add a newline here for clarity.

Line 1309: tz->debugfs_dir = NULL;
nit: add a newline here for clarity.

Line 1325: &tz->multiple_trips);
Can you make the debugfs names more consistent -- maybe prepend cdevs to the names?

Line 1327: }
nit: add a newline here for clarity.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 9, 2012, 2:08:16 PM5/9/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 2: (4 inline comments)

....................................................
File drivers/thermal/thermal_sys.c
Line 1305: }
Done. I'd actually like to move the debugfs_root initialization out of this but couldn't find a more "global" spot to put it.

Line 1309: tz->debugfs_dir = NULL;
Done

Line 1325: &tz->multiple_trips);
Done

Line 1327: }
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 9, 2012, 2:22:23 PM5/9/12
to Jon Kliegman, Sameer Nanda, Olof Johansson
Sameer Nanda has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 3: (1 inline comment)

LGTM, but lets hold off till we agree on what the UMA stats would look like in case we need to rework these.

....................................................
File drivers/thermal/thermal_sys.c
Line 1319: debugfs_create_u32("cdevs_in_delay", 0444,
delayed instead of in_delay?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 9, 2012, 2:27:07 PM5/9/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 3: (1 inline comment)

Sounds good. I saw your comments on the bug (crosbug.com/30527 for easy linking) so lets make sure we're gathering the right information.

....................................................
File drivers/thermal/thermal_sys.c
Line 1319: debugfs_create_u32("cdevs_in_delay", 0444,
This one is actually different - its not a counter, just exposing an internal state variable to help with debugging.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 11, 2012, 4:36:20 PM5/11/12
to Jon Kliegman, Sameer Nanda, Olof Johansson
Sameer Nanda has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 3: Looks good to me, approved

Discussed UMA metrics in the other bug. What you have here should suffice. LGTMing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Olof Johansson (Code Review)

unread,
May 11, 2012, 6:03:42 PM5/11/12
to Jon Kliegman, Sameer Nanda
Olof Johansson has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File drivers/thermal/thermal_sys.c
Line 1305: }
Do the above in thermal_init instead, and remove in thermal_exit.

Line 1310: tz->debugfs_dir = NULL;
No need to initialize all these to 0. kzalloc does it for you.

Line 1314: debugfs_root);
If you setup debugfs_root in thermal_init, then you can drop the if above, since the only time it should fail is when CONFIG_DEBUGFS is off. If it is indeed off, the above create_dir will ignore the arguments and just return an error. So you can save a level of indentation by not checking debugfs_root, just checking the return from create_dir.

Line 1315: if (tz->debugfs_dir && !IS_ERR(tz->debugfs_dir)) {
NULL should never be returned here, so no need to check for it. IS_ERR() is all you need.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 14, 2012, 5:24:38 PM5/14/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 3: (4 inline comments)

....................................................
File drivers/thermal/thermal_sys.c
Line 1305: }
Done

Line 1310: tz->debugfs_dir = NULL;
Done

Line 1314: debugfs_root);
Done

Line 1315: if (tz->debugfs_dir && !IS_ERR(tz->debugfs_dir)) {
Documentation/filesystems/debugfs.txt and the function comments in fs/debugfs/inode.c both say that a NULL can be returned. Looking in the code there is a path for a NULL return

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 16, 2012, 3:35:45 PM5/16/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 6:

Patch set 5 was a (failed) rebase against TOT. Patch set 6 fixed the rebase.

PTAL. Running through trybots now

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 16, 2012, 5:37:58 PM5/16/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 6: Verified

Trybots are green

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 16, 2012, 7:52:23 PM5/16/12
to Jon Kliegman, Sameer Nanda, Olof Johansson
Sameer Nanda has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 6: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 10:43:36 AM5/17/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 6:

Olof, please speak up if you still have objections.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Olof Johansson (Code Review)

unread,
May 17, 2012, 7:12:45 PM5/17/12
to Jon Kliegman, Sameer Nanda
Olof Johansson has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 6: Looks good to me, approved

(1 inline comment)

LGTM. Comment below, no need to fix.

....................................................
File drivers/thermal/thermal_sys.c
Line 1298: if (tz->debugfs_dir && !IS_ERR(tz->debugfs_dir)) {
Technically, you could use !IS_ERR_OR_NULL() here then, but it's no big deal. LGTM as is.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 18, 2012, 10:37:55 AM5/18/12
to Sameer Nanda, Olof Johansson
Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: thermal: Add debugfs metrics for fan state changes
......................................................................


Patch Set 7: Verified; Looks good to me, approved; Ready

NIT fix, pulling LGTM's forward.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07368e9260f69482e16674ddab826314bde7e22b
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>
Reply all
Reply to author
Forward
0 new messages