Merge lp:~sforshee/apport/iwlwifi-fw-error into lp:~apport-hackers/apport/trunk

[Seth Forshee]

Add data/iwlwifi_error_dump for reporting bugs when iwlwifi generates
a firmware dump. Also make assoicated changes to support this new
problem type and add an upstart job to trigger the script from the
associated uevent.

[Seth Forshee (sforshee)]

I'm not sure that the upstart job really belongs here. If not, let me know and I'll remove it.

[Martin Pitt (pitti)]

Thanks for this!

- To avoid an inflation of new problem types, would it be ok to subsume that under "KernelCrash"? ProblemType is being checked in quite a number of places, and new ones should be documented in doc/data-format.tex. But this isn't meant to be a precise description, just a broad category (crash / bug / package install).

- It would be really good for avoiding duplicate bugs if iwlwifi_error_dump could come up with a significant report['DuplicateSignature']; this should be identical for the same problem (including the model name, firmware version, and a meaningful bit of the stack trace), but not over-identify.

- In the same vein it is prudent to craft some report['Title'] there which will be used as a default bug title. Reporters will have a hard time to come up with a title which is meaningful for the kernel team bug triagers.

- Do you expect the dump script to ever take more than 30 s? If not, I'd really appreciate if we could replace the upstart job with an udev rule (don't worry, I'll worry about packaging this in setup.py and so on). That'll avoid a dependency on a particular init system (apport is being used in Debian and other distros, and we'll switch to systemd in the next cycles).

Thanks!

review: Needs Fixing

[Martin Pitt (pitti)]

Oh, and if iwlwifi_error_dump does the Title and DuplicateSignature fields, there is no need any more to hardcode this driver specific knowledge into the rather generic report.py.

[Seth Forshee (sforshee)]

On Tue, Mar 25, 2014 at 05:58:53PM -0000, Martin Pitt wrote:
> Oh, and if iwlwifi_error_dump does the Title and DuplicateSignature fields, there is no need any more to hardcode this driver specific knowledge into the rather generic report.py.

Right. So I had only noticed the methods before and not the preferential
use of the fields. Those were the only reason I needed to add the new
ProblemType, so I think "KernelCrash" will probably work fine.

A udev rule is fine too, there's no reason it should take longer than 30
seconds. I'll make these updates.

[lp:~sforshee/apport/iwlwifi-fw-error]

2778. By Seth Forshee on 2014-03-25

Remove "IwlErrorDump" ProblemType and use "KernelCrash" instead.
Along with this, remove special handling from report.py and set
Title and DuplicateSignature in iwlwifi_error_dump.

2779. By Seth Forshee on 2014-03-25

Remove iwlwifi-error-dump Upstart job.

[Seth Forshee (sforshee)]

I've also got the files for udev, not sure what to do with them so for now I just uploaded them to http://people.canonical.com/~sforshee/iwl-crashdump-files/.

[Martin Pitt (pitti)]

| phy = sys.argv[1]

It's better to use = os.path.basename(sys.argv[1]) here and call the script with the full ${DEVPATH} argument. That eliminiates the need for the iwlwifi-dump-sram.sh wrapper.

I can do that, and the packaging of the udev rule, so don't worry about that.

| sysfs_path = '/sys/kernel/debug/ieee80211/' + phy + '/iwlwifi/iwlmvm/fw_error_dump'

Is the "iwlmvm" fixed everywhere? On my system I only have /sys/kernel/debug/ieee80211/phy0/iwlwifi/iwldvm/.

Can this situation be triggered synthetically for testing? How did you test the script? I'd like to write at least a cursory automatic test for that.

Thanks!

review: Needs Information

[Martin Pitt (pitti)]

+pr['Title'] = 'iwlwifi firmware error'

I suppose we shoudl at least include the error code and fw version here?

[Seth Forshee (sforshee)]

On Wed, Mar 26, 2014 at 10:04:25AM -0000, Martin Pitt wrote:
> Is the "iwlmvm" fixed everywhere? On my system I only have /sys/kernel/debug/ieee80211/phy0/iwlwifi/iwldvm/.

iwldvm and iwlmvm are kind of different sub-drivers of iwlwifi,
supporting different generations of Intel wireless hardware. iwlmvm is
for the more recent hardware, the 3160/7160 models and later iirc.

Intel only added support for the firmware crash dumps to iwlmvm, I
suspect because the iwldvm firmware is receiving minimal maintenance at
this point.

> Can this situation be triggered synthetically for testing? How did you test the script? I'd like to write at least a cursory automatic test for that.

Yes, 'echo 1 > /sys/kernel/debug/ieee80211/<phy>/iwlwifi/iwlmvm/fw_restart'
as root. Of course that only works if you have an iwlmvm card.

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1297491 is an
example of a report which I collected this way.

[Seth Forshee (sforshee)]

On Wed, Mar 26, 2014 at 10:05:09AM -0000, Martin Pitt wrote:
> +pr['Title'] = 'iwlwifi firmware error'
>
> I suppose we shoudl at least include the error code and fw version here?

It adds the error code to the title later if it's able to find it from dmesg:

# Get iwl firmware version and error code from dmesg
dmesg = command_output(['dmesg'])
regex = re.compile('^.*iwlwifi [0-9a-fA-F:]{10}\.[0-9a-fA-F]: Loaded firmware version: ([0-9\.]+).*\\n.*iwlwifi [0-9a-fA-F:]{10}\.[0-9a-fA-F]: (0x[0-9A-F]{8} \| [A-Z_]+)', re.MULTILINE)
m = regex.findall(dmesg)
if m:
    l = m[len(m) - 1]
    fw_version = l[0]
    error_code = l[1]

    pr['IwlFwVersion'] = fw_version
    pr['IwlErrorCode'] = error_code
    pr['DuplicateSignature'] = 'iwlwifi:' + fw_version + ':' + error_code
    pr['Title'] += ': ' + error_code

The fw version could be added. But there are code paths in the driver
which could result in getting the dump without that data being printed
in dmesg, in which case we'll at least get that minimal title. I don't
know if that ever happens in practice or not.

[Martin Pitt (pitti)]

Thanks for the additional explanation! I remove the intermediate shell script, package the udev rule, and merge this.

review: Approve

=== added file 'data/iwlwifi_error_dump'
--- data/iwlwifi_error_dump	1970-01-01 00:00:00 +0000
+++ data/iwlwifi_error_dump	2014-03-25 20:21:31 +0000
@@ -0,0 +1,56 @@
+#!/usr/bin/python
+#
+# Collect information about an iwlwifi firmware error dump.
+#
+# Copyright (c) 2014 Canonical Ltd.
+# Author: Seth Forshee <[e-mail]>
+#
+# 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.  See http://www.gnu.org/copyleft/gpl.html for
+# the full text of the license.
+
+import os, sys, re
+import apport, apport.fileutils
+from apport.hookutils import command_output
+
+if len(sys.argv) != 2:
+    sys.exit(1)
+
+phy = sys.argv[1]
+sysfs_path = '/sys/kernel/debug/ieee80211/' + phy + '/iwlwifi/iwlmvm/fw_error_dump'
+if not os.path.exists(sysfs_path):
+    sys.exit(1)
+
+pr = apport.Report('KernelCrash')
+pr['Package'] = apport.packaging.get_kernel_package()
+pr['Title'] = 'iwlwifi firmware error'
+pr.add_os_info()
+
+# Get iwl firmware version and error code from dmesg
+dmesg = command_output(['dmesg'])
+regex = re.compile('^.*iwlwifi [0-9a-fA-F:]{10}\.[0-9a-fA-F]: Loaded firmware version: ([0-9\.]+).*\\n.*iwlwifi [0-9a-fA-F:]{10}\.[0-9a-fA-F]: (0x[0-9A-F]{8} \| [A-Z_]+)', re.MULTILINE)
+m = regex.findall(dmesg)
+if m:
+    l = m[len(m) - 1]
+    fw_version = l[0]
+    error_code = l[1]
+
+    pr['IwlFwVersion'] = fw_version
+    pr['IwlErrorCode'] = error_code
+    pr['DuplicateSignature'] = 'iwlwifi:' + fw_version + ':' + error_code
+    pr['Title'] += ': ' + error_code
+
+# Get iwl firmware dump file from debugfs
+try:
+    with open(sysfs_path, 'rb') as f:
+        pr['IwlFwDump'] = f.read()
+except IOError:
+    pass
+
+try:
+    with open(apport.fileutils.make_report_path(pr), 'wb') as f:
+        pr.write(f)
+except IOError as e:
+    apport.fatal('Cannot create report: ' + str(e))