Merge lp:~tai271828/checkbox/fix-1313581 into lp:checkbox

[Taihsiang Ho]

Change the way we test usb 3.0 superspeed (LP: #1313581)

fixing LP: #1313581 Change the way we test usb 3.0 superspeed

in this case, we are going to confirm two things:
1. the usb storage is detected as Superspeed USB device
2. xhci_hcd module is used to control the usb device

The idea is,
for the 1st item,
the value of supported_speed is used to confirm the device is recognized as superspeed device because of the spec. 5GB.

For the 2nd item,
compare the pci slot name of the devices using xhci and
the pci slot name where the usb device is on,
to make sure the usb device does be on the bus using xhci.

checkbox-gui was run and
the testing items related to media cards and usb storages
which using the modified script removable_storage_test
were run to confirm there is no regression.

[Zygmunt Krynicki (zyga)]

Hey.

A few comments, can you make the code a tiny bit more pythonic?

81 + if (args.driver == 'xhci_hcd'):
82 + if(5000000000 == test.rem_disks_speed[disk] and
83 + 'xhci' == test.rem_disks_xhci[disk]):
84 + print("\t\tDevice Detected: SuperSpeed USB")
85 + print("\t\tDriver Detected: xhci_hcd")

Fix PEP-8 issues, run flake8 on that file and see what shows up. Fix issues affecting the things you've changed if more are reported.

And squash fixes/comments that belong in older patches.

If you need help with any of that just ask. Thanks

review: Needs Fixing

[Taihsiang Ho (tai271828)]

thanks for the comment, I am working on that.

flake8 scanning as the following:

flake8 checkbox-support/checkbox_support/udev.py
udev.py:95:1: E302 expected 2 blank lines, found 1

flake8 providers/plainbox-provider-checkbox/bin/removable_storage_test
removable_storage_test:25:80: E501 line too long (81 > 79 characters)
removable_storage_test:103:33: E261 at least two spaces before inline comment
removable_storage_test:103:80: E501 line too long (84 > 79 characters)
removable_storage_test:180:21: E125 continuation line does not distinguish itself from next logical line
removable_storage_test:254:80: E501 line too long (82 > 79 characters)
removable_storage_test:262:80: E501 line too long (92 > 79 characters)
removable_storage_test:263:80: E501 line too long (89 > 79 characters)
removable_storage_test:296:29: E125 continuation line does not distinguish itself from next logical line
removable_storage_test:301:29: E125 continuation line does not distinguish itself from next logical line
removable_storage_test:415:78: W291 trailing whitespace
removable_storage_test:467:28: E127 continuation line over-indented for visual indent
removable_storage_test:475:24: E127 continuation line over-indented for visual indent
removable_storage_test:480:29: E127 continuation line over-indented for visual indent
removable_storage_test:481:29: E127 continuation line over-indented for visual indent
removable_storage_test:481:33: E901 SyntaxError: invalid syntax
removable_storage_test:483:25: E125 continuation line does not distinguish itself from next logical line
removable_storage_test:485:33: E127 continuation line over-indented for visual indent
removable_storage_test:501:41: E126 continuation line over-indented for hanging indent
removable_storage_test:570:47: E128 continuation line under-indented for visual indent
removable_storage_test:599:29: E125 continuation line does not distinguish itself from next logical line
removable_storage_test:606:25: E125 continuation line does not distinguish itself from next logical line

[lp:~tai271828/checkbox/fix-1313581]

2962. By Taihsiang Ho on 2014-05-02

fix pep8 coding style scanned by flake8

2963. By Taihsiang Ho on 2014-05-02

pep8 coding style, line too long (81 > 79 characters)

2964. By Taihsiang Ho on 2014-05-02

pep8 coding style, at least two spaces before inline comment, line too long

2965. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line does not distinguish itself from next logical line

2966. By Taihsiang Ho on 2014-05-02

pep8 coding style, line too long

2967. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line does not distinguish itself from next logical line

2968. By Taihsiang Ho on 2014-05-02

trailing whitespace

2969. By Taihsiang Ho on 2014-05-02

line over-indented for visual indent

2970. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line does not distinguish itself from next logical line

2971. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line over-indented for visual indent

2972. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line over-indented for hanging indent

2973. By Taihsiang Ho on 2014-05-02

pep8 coding style, E128 continuation line under-indented for visual indent

2974. By Taihsiang Ho on 2014-05-02

pep8 coding style, E125 continuation line does not distinguish itself from next logical line

2975. By Taihsiang Ho on 2014-05-02

pep8 coding style, E125 continuation line does not distinguish itself from next logical line

2976. By Taihsiang Ho on 2014-05-02

F841 local variable 'avg_write_time' is assigned to but never used

2977. By Taihsiang Ho on 2014-05-02

E265 block comment should start with '# '

[Taihsiang Ho (tai271828)]

comment 2 used flake8 under python 2 (this is incorrect)

flake8 for python3 shows there is no syntax error and two more highlights:

removable_storage_test:554:29: F841 local variable 'avg_write_time' is assigned to but never used
removable_storage_test:610:21: E265 block comment should start with '# '

the rest of scanning result is the same as those in comment 2

----------------------------------
scanned result by flake8 (python3)
----------------------------------

udev.py:95:1: E302 expected 2 blank lines, found 1

removable_storage_test:25:80: E501 line too long (81 > 79 characters)
removable_storage_test:103:33: E261 at least two spaces before inline comment
removable_storage_test:103:80: E501 line too long (84 > 79 characters)
removable_storage_test:180:21: E129 visually indented line with same indent as next logical line
removable_storage_test:254:80: E501 line too long (82 > 79 characters)
removable_storage_test:262:80: E501 line too long (92 > 79 characters)
removable_storage_test:263:80: E501 line too long (89 > 79 characters)
removable_storage_test:296:29: E129 visually indented line with same indent as next logical line
removable_storage_test:301:29: E129 visually indented line with same indent as next logical line
removable_storage_test:415:78: W291 trailing whitespace
removable_storage_test:467:28: E127 continuation line over-indented for visual indent
removable_storage_test:475:24: E127 continuation line over-indented for visual indent
removable_storage_test:480:29: E127 continuation line over-indented for visual indent
removable_storage_test:481:29: E127 continuation line over-indented for visual indent
removable_storage_test:483:25: E129 visually indented line with same indent as next logical line
removable_storage_test:485:33: E127 continuation line over-indented for visual indent
removable_storage_test:501:41: E126 continuation line over-indented for hanging indent
removable_storage_test:548:29: F841 local variable 'avg_write_time' is assigned to but never used
removable_storage_test:570:47: E128 continuation line under-indented for visual indent
removable_storage_test:599:29: E125 continuation line with same indent as next logical line
removable_storage_test:604:21: E265 block comment should start with '# '
removable_storage_test:606:25: E129 visually indented line with same indent as next logical line

[Taihsiang Ho (tai271828)]

test media card and usb storage items on 201304-13404 with 14.04.
Looks good.

[Taihsiang Ho (tai271828)]

revno 2961 is code for the usb3 testing logic.

modification after revno 2961 just for the pep8 coding style.

[Taihsiang Ho (tai271828)]

rebase the commits in
https://code.launchpad.net/~zkrynicki/checkbox/fix-1313581/+merge/218028

Unmerged revisions

2977. By Taihsiang Ho on 2014-05-02

E265 block comment should start with '# '

2976. By Taihsiang Ho on 2014-05-02

F841 local variable 'avg_write_time' is assigned to but never used

2975. By Taihsiang Ho on 2014-05-02

pep8 coding style, E125 continuation line does not distinguish itself from next logical line

2974. By Taihsiang Ho on 2014-05-02

pep8 coding style, E125 continuation line does not distinguish itself from next logical line

2973. By Taihsiang Ho on 2014-05-02

pep8 coding style, E128 continuation line under-indented for visual indent

2972. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line over-indented for hanging indent

2971. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line over-indented for visual indent

2970. By Taihsiang Ho on 2014-05-02

pep8 coding style, continuation line does not distinguish itself from next logical line

2969. By Taihsiang Ho on 2014-05-02

line over-indented for visual indent

2968. By Taihsiang Ho on 2014-05-02

trailing whitespace

=== modified file 'checkbox-support/checkbox_support/udev.py'
--- checkbox-support/checkbox_support/udev.py	2014-01-07 13:39:44 +0000
+++ checkbox-support/checkbox_support/udev.py	2014-05-02 07:49:00 +0000
@@ -91,3 +91,20 @@
     # look better.
     devices.sort(key=lambda device: device.get_device_file())
     return devices
+
+
+def get_udev_xhci_devices(udev_client):
+    """
+    Get a list of all devices on pci slots using xhci drivers
+    """
+    # setup an enumerator so that we can list devices
+    enumerator = GUdev.Enumerator(client=udev_client)
+    # Iterate over pci devices only
+    enumerator.add_match_subsystem('pci')
+    devices = [
+        device for device in enumerator.execute()
+        if (device.get_driver() == 'xhci_hcd')]
+    # Sort the list, this is not needed but makes various debugging dumps
+    # look better.
+    devices.sort(key=lambda device: device.get_property('PCI_SLOT_NAME'))
+    return devices

=== modified file 'providers/plainbox-provider-checkbox/bin/removable_storage_test'
--- providers/plainbox-provider-checkbox/bin/removable_storage_test	2014-01-07 13:43:22 +0000
+++ providers/plainbox-provider-checkbox/bin/removable_storage_test	2014-05-02 07:49:00 +0000
@@ -22,9 +22,12 @@
 from checkbox_support.dbus.udisks2 import lookup_udev_device
 from checkbox_support.dbus.udisks2 import map_udisks1_connection_bus
 from checkbox_support.heuristics.udisks2 import is_memory_card
-from checkbox_support.parsers.udevadm import CARD_READER_RE, GENERIC_RE, FLASH_RE
+from checkbox_support.parsers.udevadm import CARD_READER_RE
+from checkbox_support.parsers.udevadm import GENERIC_RE
+from checkbox_support.parsers.udevadm import FLASH_RE
 from checkbox_support.udev import get_interconnect_speed
 from checkbox_support.udev import get_udev_block_devices
+from checkbox_support.udev import get_udev_xhci_devices
 
 
 class ActionTimer():
@@ -99,6 +102,8 @@
         self.rem_disks_memory_cards = {}
         self.rem_disks_memory_cards_nm = {}
         self.rem_disks_speed = {}
+        # LP: #1313581, TODO: extend to be rem_disks_driver
+        self.rem_disks_xhci = {}
         self.data = ''
         self.device = device
         self.memorycard = memorycard
@@ -174,8 +179,8 @@
             have both the filesystem and block device interfaces
             """
             for udisks2_object_path, interfaces in udisks2_objects.items():
-                if (UDISKS2_FILESYSTEM_INTERFACE in interfaces
-                    and UDISKS2_BLOCK_INTERFACE in interfaces):
+                if (UDISKS2_FILESYSTEM_INTERFACE in interfaces and
+                        UDISKS2_BLOCK_INTERFACE in interfaces):
                     yield udisks2_object_path
         # We need to know about all IO candidates,
         # let's iterate over all the block devices reported by udisks2
@@ -247,6 +252,23 @@
             else:
                 self.rem_disks_memory_cards_nm[dev_file] = None
                 self.rem_disks_nm[dev_file] = None
+        # LP: #1313581
+        # Compare the pci slot name of the devices using xhci and
+        # the pci slot name of the disks,
+        # which is usb3 disks in this case so far,
+        # to make sure the usb3 disk does be on the bus using xhci
+        # TODO: it will be better to extend to be all kinds of drivers.
+        try:
+            udev_devices_xhci = get_udev_xhci_devices(udev_client)
+            for udev_device_xhci in udev_devices_xhci:
+                pci_slot_name = udev_device_xhci.get_property('PCI_SLOT_NAME')
+                for udev_device in udev_devices:
+                    if (pci_slot_name ==
+                            udev_device.get_property('DEVPATH').split('/')[3]):
+                        self.rem_disks_xhci[
+                            udev_device.get_property('DEVNAME')] = 'xhci'
+        except:
+            logging.error("Failed to get driver information.")
 
     def _probe_disks_udisks1(self, bus):
         """
@@ -275,14 +297,14 @@
                         parent_media = parent_props.Get(udisks, "DriveMedia")
                     if self.memorycard:
                         if (dev_bus != 'sdio'
-                            and not FLASH_RE.search(parent_media)
-                            and not CARD_READER_RE.search(parent_model)
-                            and not GENERIC_RE.search(parent_vendor)):
+                                and not FLASH_RE.search(parent_media)
+                                and not CARD_READER_RE.search(parent_model)
+                                and not GENERIC_RE.search(parent_vendor)):
                             continue
                     else:
                         if (FLASH_RE.search(parent_media)
-                            or CARD_READER_RE.search(parent_model)
-                            or GENERIC_RE.search(parent_vendor)):
+                                or CARD_READER_RE.search(parent_model)
+                                or GENERIC_RE.search(parent_vendor)):
                             continue
                     dev_file = str(device_props.Get(udisks, "DeviceFile"))
                     dev_speed = str(device_props.Get(udisks,
@@ -393,6 +415,10 @@
                         help=("Memory cards devices on bus other than sdio "
                               "require this parameter to identify "
                               "them as such"))
+    parser.add_argument('--driver',
+                        choices=['xhci_hcd'],
+                        help=("Detect the driver of the host controller."
+                              "Only xhci_hcd for usb3 is supported so far."))
 
     args = parser.parse_args()
 
@@ -444,7 +470,7 @@
 
                 if errors_mount > 0:
                     print("There're total %d device(s) failed at mounting."
-                           % errors_mount)
+                          % errors_mount)
                     errors += errors_mount
 
                 disks_all = dict(list(test.rem_disks.items())
@@ -452,17 +478,17 @@
 
             if len(disks_all) > 0:
                 print("Found the following mounted %s partitions:"
-                       % ', '.join(args.device))
+                      % ', '.join(args.device))
 
                 for disk, mount_point in disks_all.items():
                     supported_speed = test.rem_disks_speed[disk]
                     print("    %s : %s : %s bits/s" %
-                            (disk, mount_point, supported_speed),
-                            end="")
-                    if (args.min_speed
-                        and int(args.min_speed) > int(supported_speed)):
+                          (disk, mount_point, supported_speed),
+                          end="")
+                    if (args.min_speed and
+                            int(args.min_speed) > int(supported_speed)):
                         print(" (Will not test it, speed is below %s bits/s)" %
-                                args.min_speed, end="")
+                              args.min_speed, end="")
 
                     print("")
 
@@ -478,7 +504,7 @@
                 for count in range(args.count):
                     test_files[count] = RandomData(args.size)
                     write_sizes.append(os.path.getsize(
-                                        test_files[count].tfile.name))
+                        test_files[count].tfile.name))
                     total_write_size = sum(write_sizes)
 
                 try:
@@ -525,7 +551,7 @@
                             for file in target_file_list:
                                 test.clean_up(file)
                             total_write_time = sum(write_times)
-                            avg_write_time = total_write_time / args.count
+                            # avg_write_time = total_write_time / args.count
                             try:
                                 avg_write_speed = ((
                                     total_write_size / total_write_time)
@@ -547,7 +573,7 @@
                               (iteration_write_time / args.iterations))
                         try:
                             avg_write_speed = (iteration_write_size /
-                                              iteration_write_time)
+                                               iteration_write_time)
                         except ZeroDivisionError:
                             avg_write_speed = 0.00
                         finally:
@@ -573,9 +599,17 @@
                     return 1
 
                 else:
-                    #Pass is not assured!
+                    # LP: 1313581
+                    if (args.driver == 'xhci_hcd'):
+                        if(5000000000 == test.rem_disks_speed[disk] and
+                           'xhci' == test.rem_disks_xhci[disk]):
+                            print("\t\tDevice Detected: SuperSpeed USB")
+                            print("\t\tDriver Detected: xhci_hcd")
+                        else:
+                            return 1
+                    # Pass is not assured
                     if (not args.pass_speed or
-                        avg_write_speed >= args.pass_speed):
+                            avg_write_speed >= args.pass_speed):
                         return 0
                     else:
                         print("FAIL: Average speed was lower than desired "

=== modified file 'providers/plainbox-provider-checkbox/jobs/suspend.txt.in'
--- providers/plainbox-provider-checkbox/jobs/suspend.txt.in	2014-04-29 21:10:14 +0000
+++ providers/plainbox-provider-checkbox/jobs/suspend.txt.in	2014-05-02 07:49:00 +0000
@@ -1667,11 +1667,10 @@
 depends: suspend/usb3_insert_after_suspend
 user: root
 estimated_duration: 45.00
-command: removable_storage_test -s 268400000 -m 500000000 -p 60 usb
+command: removable_storage_test -s 268400000 -m 500000000 usb --driver xhci_hcd
 _description:
- This test will check that your USB 3.0 port transfers data at a
- minimum expected speed in accordance with the specification of
- USB 3.0 SuperSpeed mode.
+ This test will check that your USB 3.0 port could be recognized
+ as SuperSpeed USB device using xhci_hcd driver and transfers data correctly.
 
 plugin: user-interact
 id: suspend/mmc-insert-after-suspend

=== modified file 'providers/plainbox-provider-checkbox/jobs/usb.txt.in'
--- providers/plainbox-provider-checkbox/jobs/usb.txt.in	2014-04-10 21:51:48 +0000
+++ providers/plainbox-provider-checkbox/jobs/usb.txt.in	2014-05-02 07:49:00 +0000
@@ -217,8 +217,7 @@
 depends: usb3/insert
 user: root
 estimated_duration: 45.00
-command: removable_storage_test -s 268400000 -m 500000000 -p 60 usb
+command: removable_storage_test -s 268400000 -m 500000000 usb --driver xhci_hcd
 _description:
- This test will check that your USB 3.0 port transfers data at a
- minimum expected speed in accordance with the specification of
- USB 3.0 SuperSpeed mode.
+ This test will check that your USB 3.0 port could be recognized
+ as SuperSpeed USB device using xhci_hcd driver and transfers data correctly.