From 19b4864346f42b52b9aaacf5fcafb6fb5001281c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 16 Jul 2020 11:40:53 +0200 Subject: [PATCH 1/5] hwdb/autosuspend: add missing parenthesis --- tools/make-autosuspend-rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/make-autosuspend-rules.py b/tools/make-autosuspend-rules.py index e13ca33f6f..a20edc0f34 100755 --- a/tools/make-autosuspend-rules.py +++ b/tools/make-autosuspend-rules.py @@ -14,7 +14,7 @@ for entry in chromiumos.gen_autosuspend_rules.PCI_IDS: device = int(device, 16) print('pci:v{:08X}d{:08X}*'.format(vendor, device)) -print('# usb:vp (4 uppercase hexadecimal digits twice') +print('# usb:vp (4 uppercase hexadecimal digits twice)') for entry in chromiumos.gen_autosuspend_rules.USB_IDS: vendor, product = entry.split(':') vendor = int(vendor, 16) From df7667323d1a4a75a9a1fff1aa0aeb0d74d8a108 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 16 Jul 2020 16:24:14 +0200 Subject: [PATCH 2/5] udev: change the modalias string for usb devices to include the device name When the kernel does not provide a modalias, we generate our own for usb devices. For some reason, we generated the expected usb:vXXXXpYYYY string, suffixed by "*". It was added that way already in 796b06c21b62d13c9021e2fbd9c58a5c6edb2764, but I think that was a mistake, and Kay was thinking about the match pattern instead of the matched string. For example, for a qemu device: old: "usb:v0627p0001*" new: "usb:v0627p0001:QEMU USB Tablet" On the match side, all hwdb files in the wild seem to be using match patterns with "*" at the end. So we can add more stuff to our generated modalias with impunity. This will allow more obvious and more certain matches on USB devices. In principle the vendor+product id should be unique, but it's only 8 digits, and there's a high chance of people getting this wrong. And matching the wrong device would be quite problematic. By including the name in the match string we make a mismatch much less likely. --- src/udev/udev-builtin-hwdb.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/udev/udev-builtin-hwdb.c b/src/udev/udev-builtin-hwdb.c index 8e86d2f0d1..59ae6c7ad4 100644 --- a/src/udev/udev-builtin-hwdb.c +++ b/src/udev/udev-builtin-hwdb.c @@ -47,7 +47,7 @@ int udev_builtin_hwdb_lookup(sd_device *dev, } static const char *modalias_usb(sd_device *dev, char *s, size_t size) { - const char *v, *p; + const char *v, *p, *n = NULL; uint16_t vn, pn; if (sd_device_get_sysattr_value(dev, "idVendor", &v) < 0) @@ -58,15 +58,16 @@ static const char *modalias_usb(sd_device *dev, char *s, size_t size) { return NULL; if (safe_atoux16(p, &pn) < 0) return NULL; - snprintf(s, size, "usb:v%04Xp%04X*", vn, pn); + (void) sd_device_get_sysattr_value(dev, "product", &n); + + snprintf(s, size, "usb:v%04Xp%04X:%s", vn, pn, strempty(n)); return s; } static int udev_builtin_hwdb_search(sd_device *dev, sd_device *srcdev, const char *subsystem, const char *prefix, const char *filter, bool test) { - sd_device *d; - char s[16]; + char s[LINE_MAX]; bool last = false; int r = 0; @@ -75,7 +76,7 @@ static int udev_builtin_hwdb_search(sd_device *dev, sd_device *srcdev, if (!srcdev) srcdev = dev; - for (d = srcdev; d; ) { + for (sd_device *d = srcdev; d; ) { const char *dsubsys, *devtype, *modalias = NULL; if (sd_device_get_subsystem(d, &dsubsys) < 0) @@ -101,6 +102,8 @@ static int udev_builtin_hwdb_search(sd_device *dev, sd_device *srcdev, if (!modalias) goto next; + log_device_debug(dev, "hwdb modalias key: \"%s\"", modalias); + r = udev_builtin_hwdb_lookup(dev, prefix, modalias, filter, test); if (r > 0) break; From ec8bebbcc27ebbd9fa606743199c456ed5e3874c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Jul 2020 18:13:40 +0200 Subject: [PATCH 3/5] Add autosuspend rules for emulated QEMU devices This effectively partially reverts "rules: remove all power management from udev" / e2452eef02a839e1928f4ffd893c93a460474ab6. The rules for emulated QEMU hardware were removed in one fell swoop with other rules which were causing problems. But the qemu rules were working properly (and were adjusted through patches over time). Nowadays we have a hwdb for this, so add hwdb entries using the new detailed modalias. https://github.com/systemd/systemd/pull/353#issuecomment-658810289 --- hwdb.d/60-autosuspend.hwdb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hwdb.d/60-autosuspend.hwdb b/hwdb.d/60-autosuspend.hwdb index 0de49afa6c..e742a33ea6 100644 --- a/hwdb.d/60-autosuspend.hwdb +++ b/hwdb.d/60-autosuspend.hwdb @@ -36,6 +36,16 @@ usb:v058Fp9540* ID_AUTOSUSPEND=1 +######################################### +# QEMU +######################################### + +# Emulated USB HID devices +usb:v0627p0001:*QEMU USB Keyboard* +usb:v0627p0001:*QEMU USB Mouse* +usb:v0627p0001:*QEMU USB Tablet* + ID_AUTOSUSPEND=1 + ######################################### # Wacom ######################################### From 457763aa0393a9f11e30071bb3794caf1a4f2f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 17 Jul 2020 07:44:10 +0200 Subject: [PATCH 4/5] hwdb: allow spaces in usb: matches and similar patterns In the past we didn't have any matches like that, so the parser was stricter than necessary, but now we have, so allow that. --- hwdb.d/parse_hwdb.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hwdb.d/parse_hwdb.py b/hwdb.d/parse_hwdb.py index 8e1b5fdafa..c9851ca64c 100755 --- a/hwdb.d/parse_hwdb.py +++ b/hwdb.d/parse_hwdb.py @@ -87,7 +87,7 @@ def hwdb_grammar(): for category, conn in TYPES.items()) matchline_typed = Combine(prefix + Word(printables + ' ' + '®')) - matchline_general = Combine(Or(GENERAL_MATCHES) + ':' + Word(printables)) + matchline_general = Combine(Or(GENERAL_MATCHES) + ':' + Word(printables + ' ' + '®')) matchline = (matchline_typed | matchline_general) + EOL propertyline = (White(' ', exact=1).suppress() + From 77547d5313ea916d2fb64ca5a8812734e9b50f92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 17 Jul 2020 11:09:31 +0200 Subject: [PATCH 5/5] hwdb: check that uppercase digits are used in modalias patterns This is all confusing as hell, becuase in some places lowercase hexadecimal digits are used, and in other places uppercase. This adds a check for the most common case that we and others got wrong. I tried to extend the general grammar in hwdb_grammar() to include this check, but it quickly became very complicated and didn't seem to work properly. Doing initial parsing with more general rules is easier and also seems to give better error messages: /home/zbyszek/src/systemd-work/build/../hwdb.d/60-autosuspend.hwdb: 3 match groups, 5 matches, 3 properties Pattern 'v058fp9540*' is invalid: Expected W:(0123...), found 'f' (at char 4), (line:1, col:5) --- hwdb.d/parse_hwdb.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/hwdb.d/parse_hwdb.py b/hwdb.d/parse_hwdb.py index c9851ca64c..025133416f 100755 --- a/hwdb.d/parse_hwdb.py +++ b/hwdb.d/parse_hwdb.py @@ -79,6 +79,9 @@ GENERAL_MATCHES = {'acpi', 'OUI', } +def upperhex_word(length): + return Word(nums + 'ABCDEF', exact=length) + @lru_cache() def hwdb_grammar(): ParserElement.setDefaultWhitespaceChars('') @@ -180,8 +183,27 @@ def parse(fname): return [] return [convert_properties(g) for g in parsed.GROUPS] -def check_match_uniqueness(groups): +def check_matches(groups): matches = sum((group[0] for group in groups), []) + + # This is a partial check. The other cases could be also done, but those + # two are most commonly wrong. + grammars = { 'usb' : 'v' + upperhex_word(4) + Optional('p' + upperhex_word(4)), + 'pci' : 'v' + upperhex_word(8) + Optional('d' + upperhex_word(8)), + } + + for match in matches: + prefix, rest = match.split(':', maxsplit=1) + gr = grammars.get(prefix) + if gr: + try: + gr.parseString(rest) + except ParseBaseException as e: + error('Pattern {!r} is invalid: {}', rest, e) + continue + if rest[-1] not in '*:': + error('pattern {} does not end with "*" or ":"', match) + matches.sort() prev = None for match in matches: @@ -256,7 +278,7 @@ if __name__ == '__main__': for fname in args: groups = parse(fname) print_summary(fname, groups) - check_match_uniqueness(groups) + check_matches(groups) check_properties(groups) sys.exit(ERROR)