Use sysfs interface on recent Linux kernels

[hnez]

Hi,

we are currently developing a Linux based embedded device on which we want to be able to power on/off the individual USB ports.
Wiring up the on-board hub for individual port switching and using uhubctl as control software was thus an obvious choice.
We could however not get the desired reliability when turning ports off, even when using the retry parameter.

To solve this problem one of my colleagues has implemented a sysfs interface to control the power status from userspace which will land in the upcoming 6.0 Linux Kernel release.
We have since switched to using the sysfs directly to control port power instead of uhubctl in our own software, but would nevertheless like to see support for it in uhubctl.
The benefit of using the sysfs interface is that Linux will not get confused about the state of the devices on the hub ports and will not try to power it back on immediately.

This PR is only a bare bones implementation of how to use the interface to set the status. It could however also be used to get the current status and it could even be possible to implement a version of of uhubctl on top of it that does not depend on libusb at all. I would like to hear your opinion of how deep the sysfs interface integration should go before investing more time into an implementation.

I've marked the PR as draft as Linux Kernel 6.0 is not yet released and as I did not do a lot of testing yet.

Greetings
Leonard

[mvp]

Thank you for submitting this!

Your patch looks great, except I would check access to disable sysfs interface more carefully.

Overall this is excellent idea, however, if you or Michael Grzeschik are working on sysfs interface, why not fix underlying kernel issue properly in proposed Linux kernel patch? That is, make kernel to properly remove devices being turned off and send appropriate notifications to userspace (udev) upon receiving power off from libusb? Then, your patch will not be necessary at all.

If sysfs interace gets committed and becomes official way to do this in Linux kernel, then obviously we would have no choice and use it with your patch.
But I woud wait until it hits official Linux kernel before accepting your patch into uhubctl.

[mvp]

If user is not using sudo, this path exists but cannot be opened for writing, perhaps it should complain here about permissions?
E.g. we can use access() to check if file exists. If it does not, we fall back to libusb interface silently.
But if disable file exists and we cannot open it for writing, user should be aware that he is doing something wrong.

[mgrzeschik]

Thank you for submitting this!

Your patch looks great, except I would check access to disable sysfs interface more carefully.

Overall this is excellent idea, however, if you or Michael Grzeschik are working on sysfs interface, why not fix underlying kernel issue properly in proposed Linux kernel patch? That is, make kernel to properly remove devices being turned off and send appropriate notifications to userspace (udev) upon receiving power off from libusb? Then, your patch will not be necessary at all.

If sysfs interace gets committed and becomes official way to do this in Linux kernel, then obviously we would have no choice and use it with your patch. But I woud wait until it hits official Linux kernel before accepting your patch into uhubctl.

This mentiones patch is already mainline:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/core/port.c?id=f061f43d7418cb62b8d073e221ec75d3f5b89e17

[mvp]

Thank you for submitting this!
Your patch looks great, except I would check access to disable sysfs interface more carefully.
Overall this is excellent idea, however, if you or Michael Grzeschik are working on sysfs interface, why not fix underlying kernel issue properly in proposed Linux kernel patch? That is, make kernel to properly remove devices being turned off and send appropriate notifications to userspace (udev) upon receiving power off from libusb? Then, your patch will not be necessary at all.
If sysfs interace gets committed and becomes official way to do this in Linux kernel, then obviously we would have no choice and use it with your patch. But I woud wait until it hits official Linux kernel before accepting your patch into uhubctl.

This mentiones patch is already mainline: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/core/port.c?id=f061f43d7418cb62b8d073e221ec75d3f5b89e17

Thanks, good to know. I understand why one might want to use sysfs interface to control port power without using any extra tools - makes it quite convenient.

But I am still confused why exact same functionality of proper power switching (already implemented by port.c:disable_store()) cannot be invoked by kernel when libusb sends USB_PORT_FEAT_POWER USB control message:

                            rc = libusb_control_transfer(devh,
                                LIBUSB_REQUEST_TYPE_CLASS | LIBUSB_RECIPIENT_OTHER,
                                request, USB_PORT_FEAT_POWER,
                                port, NULL, 0, USB_CTRL_GET_TIMEOUT
                            );

Don't get me wrong, if this is all we've got, so be it, let's change uhubctl to use sysfs interface.

But I just find it weird that Linux kernel has to do things non-standard way compared to any other POSIX OS out there.

Sending hub control messages should do whatever is written in USB spec, including power switching. Linux kernel is capable of doing it, why we have to invent ways to take detours and do it weird way???

[mgrzeschik]

Because when we just invoke the mentioned message, the kernel will just not get to know anything about the hub port state change. Usually the ehci core does not get any message/interrupt if a simple port on a hub is disabled. This way it is impossible for the kernel to properly disable all associated and probed devices behind that port. When doing it the way the kernel implements
it now, we can properly remove the whole branch of connected devices.

[mvp]

@mgrzeschik , if I understand it correctly, it is the kernel which is processing hub control message coming from userspace:

rc = libusb_control_transfer(devh,
    LIBUSB_REQUEST_TYPE_CLASS | LIBUSB_RECIPIENT_OTHER,
    request, USB_PORT_FEAT_POWER,
    port, NULL, 0, USB_CTRL_GET_TIMEOUT
);

so kernel should be able to react on this properly (including ehci).

But anyway, I won't stand in the way - lets do this if that the only way Linux is supporting this now.

I think we should try sysfs interface on Linux by default, but to add an option to NOT call sysfs interface if any problem arises. E.g. add option with provisional name --nosysfs (-S) which will make uhubctl not use sysfs interface if requested.
Also, this change will make usage without sudo quite a bit more complicated. Currently one needs to configure udev permissions once and uhubctl works without sudo every single time even after reboots. With sysfs, it will basically never work without sudo, unless we find a way to permanently change permissions for /sys/bus/usb/devices/%s:%d.0/%s-port%i/disable

[mgrzeschik]

The controlltransfer you are refering to, that the kernel is getting thrown from the userspace is directly transmitted to the
ehci-controller. The kernel will only see that there is "some" control transfer without known the data structure.
So for the kernel to really know if it will need to additionally throw out some usb leaves, because it was an explicit
call to some connected hub, it would have to introspect every single control transfer and scan for the pattern.
I doubt that anyone would want that.

I think it would make sense to change the properties. So the default case will stay as it is.
But when someone has to go the extra mile to call uhubctl with sudo anyway, the user than can also
explicitly call the option --sysfs (-S) on commandline.

What do you think?

[mvp]

Yes, adding this PR under --sysfs (-S) sounds totally reasonable.
So default behavior will stay the way it is (including supporting root-less uhubctl operation if udev permissions are set up). And if one has Linux 6.x host and wants super-reliable turn off, they would have to invoke sudo uhubctl -S.

[mgrzeschik]

After reviewing the patch once again, I just realized that the suggested autodetection should be prefered and the --sysfs is actually unnecessary. When a user will call the code without root privileges or on a older system, the newer binary will fallback
into the old code path anyway. So the userinterface is more convenient and shouldn''t change anything.

I think the code is mergable as is.

[hnez]

Hi @mvp,

udev

including supporting root-less uhubctl operation if udev permissions are set up

I've just added a commit to the PR that adds a section to the README on how to set up root-less access to the sysfs interface.
This also has the added benefit of being much more selective on what non-root users are allowed to do with the hub (e.g. only turn power on/off and not being allowed to send basically any USB command).

These rules are not yet tested that well as I am currently developing directly on an embedded device where more or less everything runs as root. I will do some testing before marking the PR as non-draft.

If user is not using sudo, this path exists but cannot be opened for writing, perhaps it should complain here about permissions?
E.g. we can use access() to check if file exists. If it does not, we fall back to libusb interface silently.
But if disable file exists and we cannot open it for writing, user should be aware that he is doing something wrong.

This sounds good to me. I will implement it tomorrow/soon.
E.g. something like this:

• …/disable does not exist
  1. Silently assume we are running on Linux < 6.0
  2. Use libusb
• .../disable exists but is not writable
  1. Print a message about missing permissions, falling back to libusb and a pointer on where to find the correct udev rules for sysfs-based switching.
  2. Use libusb

Command line Parameters

adding this PR under --sysfs (-S) sounds totally reasonable.

I would prefer not to to this as it would make using uhubctl in scripts or wrappers like labgrid more complicated.
A user would either have to know precisely which uhubctl they are using/going to use or query e.g. the --help to find out if the --sysfs parameter is available.
Would you be okay with not adding a commandline parameter but instead relying on a sensible fallback mechanism to the libusb based approach?

[mvp]

Thank you for adding udev rules for sysfs, much appreciated!

We can push it as is, but I think that it would be helpful to change README.md to not split Linux 6 vs older Linux into separate paragraphs because they share everything except for 2-3 lines.

[mvp]

Just add this to relevant existing block to say this:

# Linux 5.x or earlier (its ok to have this block present in newer Linux):
SUBSYSTEM=="usb", ATTR{idVendor}=="2001", MODE="0666"

# Linux 6.0 or later (its ok to have this block present in older Linux):
SUBSYSTEM=="usb", DRIVER=="hub", \
    RUN="/bin/sh -c \"chmod --silent 666 $sys$devpath/*-port*/disable\""

[hnez]

Yeah, I like that better than the split sections. And have reworked my previous commits to match this.

It also turned out that having just the rule to change the …/disable permissions is not enough, as libusb is still used for a lot of things.

[mvp]

Similarly, add this to relevant existing block to say this (note added -f so it can work on older Linux without spamming logs):

# Linux 5.x or earlier (its ok to have this block present in newer Linux):
SUBSYSTEM=="usb", ATTR{idVendor}=="2001", MODE="0664", GROUP="dialout"

# Linux 6.0 or later (its ok to have this block present in older Linux):
SUBSYSTEM=="usb", DRIVER=="hub", \
    RUN+="/bin/sh -c \"chown -f root:dialout $sys$devpath/*-port*/disable\"" \
    RUN+="/bin/sh -c \"chmod -f 660 $sys$devpath/*-port*/disable\""

[hnez]

Using only -f is not enough to keep udev from spamming the log, as chown/chmod still return an error code when the operation fails (even though they do not print a message).
I've also added || true to the commandline to make sure that does not happen.

[mvp]

Linux 6.0 paragraph and Kernels before 6.0 have 95% in common.
I would prefer to refactor this to remove Linux 6.0 separate paragraph, and instead add Linux 6.0 specific blocks as outlined below.
We should suggest distro packagers to supply one udev rule file which contains support for both Linux 6.0 and older.

[mvp]

Just move this to start on Linux permissions paragraph to say something like this:

On Linux Kernel 6.0 and later (e.g. `uname -r` shows a version numer greater or equal to 6.0) there is
a standard interface to turn USB hub ports on or off, and `uhubctl` will try using it instead of `libusb`
if it is supported by current kernel.
Rules below have slight differences specific to Linux 6.x, but you can have both enabled
if you are not sure which kernel version you have or will have.

[mvp]

numer => number

[mvp]

I've just added a commit to the PR that adds a section to the README on how to set up root-less access to the sysfs interface. This also has the added benefit of being much more selective on what non-root users are allowed to do with the hub (e.g. only turn power on/off and not being allowed to send basically any USB command).
These rules are not yet tested that well as I am currently developing directly on an embedded device where more or less everything runs as root. I will do some testing before marking the PR as non-draft.

Thanks a lot!

something like this:

…/disable does not exist
Silently assume we are running on Linux < 6.0
Use libusb
.../disable exists but is not writable
Print a message about missing permissions, falling back to libusb and a pointer on where to find the correct udev rules for sysfs-based switching.
Use libusb
Command line Parameters
adding this PR under --sysfs (-S) sounds totally reasonable.

Perhaps only add --nosysfs (-S) to only disable sysfs magic on Linux 6? I mean make --sysfs default but still allow command line option to disable it (however unlikely it is needed, but maybe someone will hit some bugs).

I would prefer not to to this as it would make using uhubctl in scripts or wrappers like labgrid more complicated.
A user would either have to know precisely which uhubctl they are using/going to use or query e.g. the --help to find out if the --sysfs parameter is available.
Would you be okay with not adding a commandline parameter but instead relying on a sensible fallback mechanism to the libusb based approach?

--nosysfs should be good solution for sensible defaults under most usage scenarios.

[hnez]

--nosysfs should be good solution for sensible defaults under most usage scenarios.

Sounds sensible to me. Implementing it required a bunch of #ifdef __gnu_linux__s that I am not particularly fond of, but I figured they would be better than confronting users of other OSes with options that do not apply to them.

I've tested:

• Previous wide-open udev rule on Linux 6.0 (e.g. there is a …/disable attribute but the permissions are wrong)
  → uhubctl prints an error about permission issues while writing to …/disable and sucessfully falls back to libusb based switching.
• Previous wide-open udev rule on Linux 6.0 with -S/--nosysfs option
  → No error shown and successful switching using libusb.
• New wide-open udev rule on Linux 6.0
  → No error shown by udev or uhubctl, successful switching via sysfs.
• New wide-open udev rule on Linux 5.19 (e.g. no /disable attribute)
  → No error shown by udev or uhubctl sucessful fallback to libusb.
• New group based udev rule on Linux 5.19 / 6.0
  → No error shown by udev or uhubctl. Expected behavior on both systems (fallback to libusb/use sysfs).

I think with the addition of the new udev rules the PR is now in a usable state, so will mark is non-draft.

[mvp]

Thank you for this work! I will follow up with few cleanups

[mvp]

Pushed couple followup fixes dd5ff8f and 732ad2d.

[mvp]

@hnez , @mgrzeschik : now that Linux kernel 6.0 is released, can you please confirm that uhubctl built from master works on 6.0 as expected?
I would like to release new version of uhubctl, perhaps 2.5.0.
Thanks!

[hnez]

Hi,

sorry for the long delay, I've been on vacation. I've just tested uhubctl on our embedded device with an (when it comes to USB) unpatched 6.0 kernel.

Using strace I can see that the sysfs interface was used and that power on/off/toggle did what I would have expected:

root@lxatac-00010:~ strace uhubctl -l 1-1 -p 1 -a off 2>&1  | grep disable -A 2
openat(AT_FDCWD, "/sys/bus/usb/devices/1-1:1.0/1-1-port1/disable", O_WRONLY) = 8
write(8, "1", 1)                        = 1
close(8)                                = 0

root@lxatac-00010:~ strace uhubctl -l 1-1 -p 1 -a on 2>&1  | grep disable -A 2
openat(AT_FDCWD, "/sys/bus/usb/devices/1-1:1.0/1-1-port1/disable", O_WRONLY) = 8
write(8, "0", 1)                        = 1
close(8)

root@lxatac-00010:~ strace uhubctl -l 1-1 -p 1 -a toggle 2>&1  | grep disable -A 2
openat(AT_FDCWD, "/sys/bus/usb/devices/1-1:1.0/1-1-port1/disable", O_WRONLY) = 8
write(8, "1", 1)                        = 1
close(8)

Our custom USB2514B based hardware is however the only hub I have at hand that supports individual port power switching.

I will be in the office on Thursday and will have a look if I can find some other hub and another host to test with, maybe a USB 3.x one. I do however not have too high hopes of actually finding such a hub.

[mgrzeschik]

To have this usb3 hub case tested:

I did succesfully test this with an usb3 hub.

After triggering an usb3 port to be disabled by uhubctl
the corresponding usb2 peer port got disabled aswell.

uhubctl -a off -l 2-1 -p 2
Current status for hub 2-1 [2109:0813 VIA Labs, Inc. USB3.0 Hub, USB 3.00, 4 ports, ppps]
Port 1: 02a0 power 5gbps Rx.Detect
Port 2: 0080 off
Port 3: 02a0 power 5gbps Rx.Detect
Port 4: 02a0 power 5gbps Rx.Detect
Current status for hub 1-1 [2109:2813 VIA Labs, Inc. USB2.0 Hub, USB 2.10, 4 ports, ppps]
Port 1: 0100 power
Port 2: 0000 off
Port 3: 0100 power
Port 4: 0100 power

cat /sys/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/1-1-port2/disable
1
cat /sys/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/1-1-port2/peer/disable
1

This got reversed for both ports after enabling one of the ports again.

uhubctl -a on -l 1-1 -p 1
Current status for hub 2-1 [2109:0813 VIA Labs, Inc. USB3.0 Hub, USB 3.00, 4 ports, ppps]
Port 1: 02a0 power 5gbps Rx.Detect
Port 2: 02a0 power 5gbps Rx.Detect
Port 3: 02a0 power 5gbps Rx.Detect
Port 4: 02a0 power 5gbps Rx.Detect
Current status for hub 1-1 [2109:2813 VIA Labs, Inc. USB2.0 Hub, USB 2.10, 4 ports, ppps]
Port 1: 0100 power
Port 2: 0100 power
Port 3: 0100 power
Port 4: 0100 power

cat /sys/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/1-1-port2/disable
0
cat /sys/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/1-1-port2/peer/disable
0

I also validated this by enumerating usb2 or usb3 devices on this ports. All corresponding usb
devices got disconnected properly after disabling the ports with uhubctl.

[mvp]

Thank you @mgrzeschik! uhubctl 2.5.0 which contains this fix is already added into testing repositories of major Linux distros: Debian, Fedora, Raspbian, OpenSUSE. I would expect it to be present in all Linux package repositories in few months.