Bug 209469 - [bisected] usb_modeswitch is broken on Linux 5.9, at least with Snapdragon X12 LTE modem
Summary: [bisected] usb_modeswitch is broken on Linux 5.9, at least with Snapdragon X1...
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: USB (show other bugs)
Hardware: All Linux
: P1 high
Assignee: Default virtual assignee for Drivers/USB
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-02 03:11 UTC by RussianNeuroMancer
Modified: 2021-06-11 02:14 UTC (History)
5 users (show)

See Also:
Kernel Version: 5.9rc6
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
usb_modeswitch log on Linux 5.8.12 (2.41 KB, text/plain)
2020-10-02 03:12 UTC, RussianNeuroMancer
Details
usb_modeswitch log on Linux 5.9rc8 (2.47 KB, text/plain)
2020-10-07 06:24 UTC, RussianNeuroMancer
Details

Description RussianNeuroMancer 2020-10-02 03:11:53 UTC
Hello!

I found that usb_modeswitch can't enable Snapdragon X12 LTE modem on Linux 5.9rc6:

> USB_ModeSwitch log from Sun Sep 27 06:27:13 +02 2020
> Raw parameters: --switch-mode 2-3
> Use global config file: /etc/usb_modeswitch.conf
>
> Use global config file: /etc/usb_modeswitch.conf
> Use top device dir /sys/bus/usb/devices/2-3
> Check class of first interface ...
>  No access to first interface. Exit

While on Linux 5.8.12 it works just fine - see attached log.

Bisect lead to following commit: a45aca510b73b745f27f39c6bb590b1743ea1792
PM: sleep: core: Emit changed uevent on wakeup_sysfs_add/remove
Udev rules that depend on the power/wakeup attribute don't get triggered
correctly if device_set_wakeup_capable is called after the device is
created. This can happen for several reasons (driver sets wakeup after
device is created, wakeup is changed on parent device, etc) and it seems
reasonable to emit a changed event when adding or removing attributes on
the device.

https://github.com/torvalds/linux/commit/a45aca510b73b745f27f39c6bb590b1743ea1792

I verified that usb_modeswitch works on Linux 5.9rc6 build with reverted commit a45aca510b73b745f27f39c6bb590b1743ea1792
It produce same log as with Linux 5.8.12 and modem also works just fine.
Comment 1 RussianNeuroMancer 2020-10-02 03:12:45 UTC
Created attachment 292757 [details]
usb_modeswitch log on Linux 5.8.12
Comment 2 Abhishek Pandit-Subedi 2020-10-02 03:40:11 UTC
This looks like it's implying the notification (UEVENT_CHANGE) is being seen but the device doesn't exist yet ("no access to first interface").

There's probably a missing check to see if the UEVENT_ADDED has been emitted yet (or the device has been created). Will start taking a look.
Comment 3 Abhishek Pandit-Subedi 2020-10-02 04:09:39 UTC
I can't reproduce the original issue but if my suspicion is correct, the following should fix the issue (won't emit KOBJ_CHANGE unless object is already in sysfs, i.e. already emitted KOBJ_ADD).

Could you give this a try on top of 5.9?

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7998affa45d49a..f08197e907d5ce 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -473,6 +473,11 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
        if (action == KOBJ_REMOVE)
                kobj->state_remove_uevent_sent = 1;

+       if (action == KOBJ_CHANGE && !kobj->state_in_sysfs) {
+               pr_debug("kobject: can't emit KOBJ_CHANGE until in sysfs\n");
+               return -EINVAL;
+       }
+
        pr_debug("kobject: '%s' (%p): %s\n",
                 kobject_name(kobj), kobj, __func__);
Comment 4 RussianNeuroMancer 2020-10-03 21:51:41 UTC
Thank you for looking into this issue, Abhishek!

> Could you give this a try on top of 5.9?

I just tested this patch and unfortunately it didn't help - usb_modeswitch output is the same and modem is not enabled.
Comment 5 Abhishek Pandit-Subedi 2020-10-05 23:38:05 UTC
Can you share what udev rules you're using for usb_modeswitch?

If you set your udev rule with ACTION=="add", does it fix your issue? (I would assume yes because you would just ignore the "change" notification)
Comment 6 RussianNeuroMancer 2020-10-06 06:31:20 UTC
> Can you share what udev rules you're using for usb_modeswitch?

Here is the couple of lines from file /lib/udev/rules.d/40-usb_modeswitch.rules shipped as part of usb-modeswitch-data package (version 20191128-3).

> # HP lt4220 NGFF Card
> ATTR{idVendor}=="03f0", ATTR{idProduct}=="0857", RUN+="usb_modeswitch '/%k'"

> If you set your udev rule with ACTION=="add", does it fix your issue?

Please clarify, in the beginning /lib/udev/rules.d/40-usb_modeswitch.rules there is line:
> ACTION!="add|change"
And I should change it to this: 
> ACTION=="add"
Correct?
Comment 7 Abhishek Pandit-Subedi 2020-10-07 05:39:00 UTC
Ah, is it this file? https://github.com/digidietze/usb-modeswitch-data/blob/master/40-usb_modeswitch.rules#L5

Try modifying that to ACTION!="add". If the bisected change was indeed the cause of the bug, this udev rule will likely fix it.

If that works, I think the patch I posted above should work as well. Is it possible that the above patch works as intended but `usb_modeswitch` causes the mode to switch back to the original? i.e. 0 -> 1 -> 0?
Comment 8 RussianNeuroMancer 2020-10-07 06:24:23 UTC
Created attachment 292877 [details]
usb_modeswitch log on Linux 5.9rc8

> If the bisected change was indeed the cause of the bug, this udev rule will
> likely fix it.

It's indeed fixed this issue, thanks for advice! However, as I understand, to make this fix actually useful for other affected users with similar hardware few conditions have to be meet:

1. usb_modeswitch devs agree to modify udev rule. I am not developer, but is there is chance that "|change" is here for a reason and have to be kept for some other device that actually need it?
2. After that usb_modeswitch devs release a new version of usb-modeswitch-data, which happen not very frequently, as you can see: https://launchpad.net/ubuntu/+source/usb-modeswitch-data
3. Distributions package and ship new version of usb-modeswitch-data.

Until this three conditions are meet some other devices supported by usb_modeswitch will require this workaround (editing udev rule manually) and user intervention on Linux 5.9 and newer - is that correct? I'm afraid Snapdragon X12 LTE is not only device (out of 414 devices supported by usb_modeswitch) that is broken by this change :(

> Is it possible that the above patch works as intended but `usb_modeswitch`
> causes the mode to switch back to the original? i.e. 0 -> 1 -> 0?

Unfortunately, I don't know for sure. When usb_modeswitch work normally - it find device in default mode (1) and then switch it to Configuration 3, and then modem works. I attached log taken from Linux 5.9rc8 after editing udev rule. If you need log from Linux 5.8 with original udev rule - please let me know.
Comment 9 Abhishek Pandit-Subedi 2020-10-07 23:58:08 UTC
I'm going to send in the patch from comment#3 since it's probably a good idea to have.

As for fixing up the udev rule, I think the usb_modeswitch developers and the distribution maintainers need to be made aware of this regression so they can fix it. 

I will also ask linux maintainers on what's a good way to inform users about this change so they can check for breakage and apply fixes to their rules. gregkh@ might have an opinion on what's the right way to do things (and it may mean just reverting this change for 5.9).
Comment 10 RussianNeuroMancer 2020-10-10 05:33:28 UTC
> gregkh@ might have an opinion on what's the right way to do things (and it
> may mean just reverting this change for 5.9).

Is there decision regarding 5.9 release?
Comment 11 Abhishek Pandit-Subedi 2020-10-19 20:31:50 UTC
I put up the patch for review but forgot to send it upstream for comment. I will go ahead and do that today.

I tried to send an email to the maintainer of usb_modeswitch as well but the email just bounced. Could you please reach out to that project to make sure they update their udev scripts?
Comment 12 Abhishek Pandit-Subedi 2020-10-19 20:32:24 UTC
I put up the patch for review but forgot to send it upstream for comment. I will go ahead and do that today.

I tried to send an email to the maintainer of usb_modeswitch as well but the email just bounced. Could you please reach out to that project to make sure they update their udev scripts?
Comment 13 RussianNeuroMancer 2020-10-20 04:51:04 UTC
> Could you please reach out to that project to make sure they update their
> udev scripts?

I'm afraid it's impossible at this moment, because if email won't work and latest post on project forum by project maintainer was six weeks ago 
https://www.draisberghof.de/usb_modeswitch/bb/viewtopic.php?f=4&t=2954&p=19650#p19650 in my opinion it's better act in assumption that issue can not be solved from usb_modeswitch side in short timeframe and commit should be reverted after all. What you think about it?
Comment 14 RussianNeuroMancer 2020-10-20 04:59:56 UTC
Simply put, I have exactly same means of contacting project mainter as you do. This email you sent with my address in CC remain unanswered. And it's seems like we can't contact him via forum, because at least now he don't answer on forum too - there is nothing I can do about it, unfortunately.
Comment 15 Josua Dietze 2020-11-04 07:19:55 UTC
Hi, usb_modeswitch maintainer here.
Sorry for the lack of response, I have been out of the loop for a while.

I just remember that the addition of "change" to the udev rule was a bug fix for annother setup. I'll search my archive.
Comment 16 Josua Dietze 2020-11-04 19:15:51 UTC
So here is the mail exchange from 2011 that lead me to add the "change" event to the udev rule.
In a nutshell, it was about treating devices that are already connected at boot but not set up yet.


-x-snip-x-


15.08.2011 15:33, Josua Dietze пишет:
> Am 15.08.2011 13:02, schrieb Paul Wolneykien:
>> I've found that my USB modem is not switched when the system reboots.
>> The reason for that is that the kernel emits only "change" events for
>> the devices it finds already plugged-in when `udevadm trigger' is called
>> on the udev service start. However, only `add' events are processed by
>> the modeswitch udev rule script. The attached patch fixes the problem.
>
> That is an important finding. The problem is likely to affect not all
> udev versions; I am testing on Suse, Fedora, Debian old/current, Ubuntu
> old/current, Mandriva and Xandros and have not encountered this.
>
> However, there is a Debian bug report which may be related to that issue.
>
> Did you try to simplify the fix even further with:
>
> -ACTION!="add", GOTO="modeswitch_rules_end"
> +ACTION!="add|change", GOTO="modeswitch_rules_end"
Comment 17 Tomas Janousek 2021-01-01 12:40:43 UTC
This also breaks usbguard: https://github.com/USBGuard/usbguard/issues/444

The problem with that is that usbguard will deauthorize temporarily allowed devices upon reception of any change uevent, and it worked so far because there weren't any other change uevents emitted, at least in the simple case of a USB mouse or something. I believe this should be fixed in usbguard, but I thought you folks should know about this anyway.
Comment 18 RussianNeuroMancer 2021-06-11 02:14:00 UTC
> In a nutshell, it was about treating devices that are already connected at
> boot but not set up yet.

Please clarify, is this should be fixed on kernel side or on usb modeswitch side? (Or maybe it was already fixed somewhere and I just doesn't know about it.)

Note You need to log in before you can comment on or make changes to this bug.