| Message ID | 1620744143-26075-2-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cac6fb015f719104e60b1c68c15ca5b734f57b9c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/2] net: wwan: Add unknown port type | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 4 maintainers not CCed: gregkh@linuxfoundation.org linux-usb@vger.kernel.org lee.jones@linaro.org penguin-kernel@i-love.sakura.ne.jp |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Lines should not end with a '(' WARNING: line length of 81 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hey Loic, On Tue, May 11, 2021 at 4:33 PM Loic Poulain <loic.poulain@linaro.org> wrote: > > The WWAN framework provides a unified way to handle WWAN/modems and its > control port(s). It has initially been introduced to support MHI/PCI > modems, offering the same control protocols as the USB variants such as > MBIM, QMI, AT... The WWAN framework exposes these control protocols as > character devices, similarly to cdc-wdm, but in a bus agnostic fashion. > > This change adds registration of the USB modem cdc-wdm control endpoints > to the WWAN framework as standard control ports (wwanXpY...). > > Exposing cdc-wdm through WWAN framework normally maintains backward > compatibility, e.g: > $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get-ids > instead of > $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids > I have some questions regarding how all this would be seen from userspace. Does the MBIM control port retain the ability to query the maximum message size with an ioctl like IOCTL_WDM_MAX_COMMAND? Or is that lost? For the libmbim case this may not be a big deal, as we have a fallback mechanism to read this value from the USB descriptor itself, so just wondering. Is the sysfs hierarchy maintained for this new port type? i.e. if doing "udevadm info -p /sys/class/wwan/wwan0p1QMI -a", would we still see the immediate parent device with DRIVERS=="qmi_wwan" and the correct interface number/class/subclass/protocol attributes? > However, some tools may rely on cdc-wdm driver/device name for device > detection. It is then safer to keep the 'legacy' cdc-wdm character > device to prevent any breakage. This is handled in this change by > API mutual exclusion, only one access method can be used at a time, > either cdc-wdm chardev or WWAN API. How does this mutual exclusion API work? Is the kernel going to expose 2 different chardevs always for the single control port? If so, do we really want to do that? How would we know from userspace that the 2 chardevs apply to the same port? And, which of the chardevs would be exposed first (and notified first by udev), the wwan one or the cdc-wdm one?
Hey, > > On Tue, May 11, 2021 at 4:33 PM Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > The WWAN framework provides a unified way to handle WWAN/modems and its > > > control port(s). It has initially been introduced to support MHI/PCI > > > modems, offering the same control protocols as the USB variants such as > > > MBIM, QMI, AT... The WWAN framework exposes these control protocols as > > > character devices, similarly to cdc-wdm, but in a bus agnostic fashion. > > > > > > This change adds registration of the USB modem cdc-wdm control endpoints > > > to the WWAN framework as standard control ports (wwanXpY...). > > > > > > Exposing cdc-wdm through WWAN framework normally maintains backward > > > compatibility, e.g: > > > $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get-ids > > > instead of > > > $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids > > > > > > > I have some questions regarding how all this would be seen from userspace. > > > > Does the MBIM control port retain the ability to query the maximum > > message size with an ioctl like IOCTL_WDM_MAX_COMMAND? Or is that > > lost? For the libmbim case this may not be a big deal, as we have a > > fallback mechanism to read this value from the USB descriptor itself, > > so just wondering. > > There is no such ioctl but we can add a sysfs property file as > proposed by Dan in the Intel iosm thread. > Yeah, that may be a good thing to add I assume. > > > > Is the sysfs hierarchy maintained for this new port type? i.e. if > > doing "udevadm info -p /sys/class/wwan/wwan0p1QMI -a", would we still > > see the immediate parent device with DRIVERS=="qmi_wwan" and the > > correct interface number/class/subclass/protocol attributes? > > Not an immediate parent since a port is a child of a logical wwan > device, but you'll still be able to get these attributes: > Below, DRIVERS=="qmi_wwan". > > $ udevadm info -p /sys/class/wwan/wwan0p1QMI -a > > looking at device > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0/wwan0p1QMI': > KERNEL=="wwan0p1QMI" > SUBSYSTEM=="wwan" > DRIVER=="" > > looking at parent device > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0': > KERNELS=="wwan0" > SUBSYSTEMS=="wwan" > DRIVERS=="" > > looking at parent device '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2': > KERNELS=="2-3:1.2" > SUBSYSTEMS=="usb" > DRIVERS=="qmi_wwan" > ATTRS{authorized}=="1" > ATTRS{bInterfaceNumber}=="02" > ATTRS{bInterfaceClass}=="ff" > ATTRS{bNumEndpoints}=="03" > ATTRS{bInterfaceProtocol}=="ff" > ATTRS{bAlternateSetting}==" 0" > ATTRS{bInterfaceSubClass}=="ff" > ATTRS{interface}=="RmNet" > ATTRS{supports_autosuspend}=="1" > Ok, that should be fine, and I think we would not need any additional change to handle that. The logic looking for what's the driver in use should still work. > > > > However, some tools may rely on cdc-wdm driver/device name for device > > > detection. It is then safer to keep the 'legacy' cdc-wdm character > > > device to prevent any breakage. This is handled in this change by > > > API mutual exclusion, only one access method can be used at a time, > > > either cdc-wdm chardev or WWAN API. > > > > How does this mutual exclusion API work? Is the kernel going to expose > > 2 different chardevs always for the single control port? > > Yes, if cdc-wdm0 is open, wwan0p1QMI can not be open (-EBUSY), and vice versa. > Oh... but then, what's the benefit of adding the new wwan0p1QMI port? I may be biased because I have always the MM case in mind, and in there we'll need to support both things, but doesn't this new port add more complexity than making it simpler? I would have thought that it's either a cdc-wdm port or a wwan port, but both? Wouldn't it make more sense to default to the new wwan subsystem if the wwan subsystem is built, and otherwise fallback to cdc-wdm? (i.e. a build time option). Having two chardevs to manage exactly the same control port, and having them mutually exclusive is a bit strange. > > really want to do that? > > This conservative way looks safe to me, but feel free to object if any issue. > I don't think adding an additional control port named differently while keeping the cdc-wdm name is adding any simplification in userspace. I understand your point of view, but if there are users setting up configuration with fixed cdc-wdm port names, they're probably not doing it right. I have no idea what's the usual approach of the kernel for this though, are the port names and subsystem considered "kernel API"? I do recall in between 3.4 and 3.6 I think that the subsystem of QMI ports changed from "usb" to "usbmisc"; I would assume your change to be kind of equivalent and therefore not a big deal? > > How would we know from userspace that the 2 > > chardevs apply to the same port? > > They share the same USB interface, and if one is open the other is busy. > Not sure I personally like that complication. > > And, which of the chardevs would be > > exposed first (and notified first by udev), the wwan one or the > > cdc-wdm one? > > cdc-wdm is registered first, though when I run MM with changes to > handle 'WWAN ports', MM is trying to probe wwan first. Maybe because you're manually running MM and the "wwan" subsystem is being iterated first? That would mean that when relying on udev events (i.e. MM is already running when the device is detected) we would get the cdc-wdm port notified first, and we would be using the other chardev. If we were to keep this logic, and if the cdc-wdm port is always created first (and notified first by udev), I would suggest to always keep using the cdc-wdm port in ModemManager, because it's easier to discard "the second" port in the logic :/ > MM is then unable to open /dev/cdc-wdm0, and simply discards that port: > System | device: > /sys/devices/pci0000:00/0000:00:14.0/usb2/2-3 > | drivers: option, qmi_wwan > | plugin: telit > | primary port: wwan0p1QMI > | ports: ttyUSB0 (ignored), ttyUSB1 (ignored), ttyUSB2 (at), > | ttyUSB3 (at), ttyUSB4 (ignored), wwan0 (net), wwan0p1QMI (qmi) > If this logic is kept we could improve this a bit more; e.g. by detecting whether the ports are really the same (e.g. looking at what USB interface number they apply to) and fully discard the second port reported. As said above, if the cdc-wdm port is the first one notified, we would be discarding the wwan port :/ > But before enabling WWAN probe for USB devices in MM, some changes will be requested, such as: > https://gitlab.freedesktop.org/loicpoulain/ModemManager/-/commit/ca3befb464aacc6175820c06c16b2b4b120c1923 > That's fine, yes. And we'd also require some changes like those for the cdc-wdm AT ports in the huawei plugin.
On Wed, 12 May 2021 at 11:04, Aleksander Morgado <aleksander@aleksander.es> wrote: > > Hey, > > > > On Tue, May 11, 2021 at 4:33 PM Loic Poulain <loic.poulain@linaro.org> wrote: > > > > > > > > The WWAN framework provides a unified way to handle WWAN/modems and its > > > > control port(s). It has initially been introduced to support MHI/PCI > > > > modems, offering the same control protocols as the USB variants such as > > > > MBIM, QMI, AT... The WWAN framework exposes these control protocols as > > > > character devices, similarly to cdc-wdm, but in a bus agnostic fashion. > > > > > > > > This change adds registration of the USB modem cdc-wdm control endpoints > > > > to the WWAN framework as standard control ports (wwanXpY...). > > > > > > > > Exposing cdc-wdm through WWAN framework normally maintains backward > > > > compatibility, e.g: > > > > $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get-ids > > > > instead of > > > > $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids > > > > > > > > > > I have some questions regarding how all this would be seen from userspace. > > > > > > Does the MBIM control port retain the ability to query the maximum > > > message size with an ioctl like IOCTL_WDM_MAX_COMMAND? Or is that > > > lost? For the libmbim case this may not be a big deal, as we have a > > > fallback mechanism to read this value from the USB descriptor itself, > > > so just wondering. > > > > There is no such ioctl but we can add a sysfs property file as > > proposed by Dan in the Intel iosm thread. > > > > Yeah, that may be a good thing to add I assume. > > > > > > > Is the sysfs hierarchy maintained for this new port type? i.e. if > > > doing "udevadm info -p /sys/class/wwan/wwan0p1QMI -a", would we still > > > see the immediate parent device with DRIVERS=="qmi_wwan" and the > > > correct interface number/class/subclass/protocol attributes? > > > > Not an immediate parent since a port is a child of a logical wwan > > device, but you'll still be able to get these attributes: > > Below, DRIVERS=="qmi_wwan". > > > > $ udevadm info -p /sys/class/wwan/wwan0p1QMI -a > > > > looking at device > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0/wwan0p1QMI': > > KERNEL=="wwan0p1QMI" > > SUBSYSTEM=="wwan" > > DRIVER=="" > > > > looking at parent device > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0': > > KERNELS=="wwan0" > > SUBSYSTEMS=="wwan" > > DRIVERS=="" > > > > looking at parent device '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2': > > KERNELS=="2-3:1.2" > > SUBSYSTEMS=="usb" > > DRIVERS=="qmi_wwan" > > ATTRS{authorized}=="1" > > ATTRS{bInterfaceNumber}=="02" > > ATTRS{bInterfaceClass}=="ff" > > ATTRS{bNumEndpoints}=="03" > > ATTRS{bInterfaceProtocol}=="ff" > > ATTRS{bAlternateSetting}==" 0" > > ATTRS{bInterfaceSubClass}=="ff" > > ATTRS{interface}=="RmNet" > > ATTRS{supports_autosuspend}=="1" > > > > Ok, that should be fine, and I think we would not need any additional > change to handle that. The logic looking for what's the driver in use > should still work. > > > > > > > However, some tools may rely on cdc-wdm driver/device name for device > > > > detection. It is then safer to keep the 'legacy' cdc-wdm character > > > > device to prevent any breakage. This is handled in this change by > > > > API mutual exclusion, only one access method can be used at a time, > > > > either cdc-wdm chardev or WWAN API. > > > > > > How does this mutual exclusion API work? Is the kernel going to expose > > > 2 different chardevs always for the single control port? > > > > Yes, if cdc-wdm0 is open, wwan0p1QMI can not be open (-EBUSY), and vice versa. > > > > Oh... but then, what's the benefit of adding the new wwan0p1QMI port? > I may be biased because I have always the MM case in mind, and in > there we'll need to support both things, but doesn't this new port add > more complexity than making it simpler? I would have thought that it's > either a cdc-wdm port or a wwan port, but both? Wouldn't it make more > sense to default to the new wwan subsystem if the wwan subsystem is > built, and otherwise fallback to cdc-wdm? (i.e. a build time option). > Having two chardevs to manage exactly the same control port, and > having them mutually exclusive is a bit strange. > > > > > really want to do that? > > > > This conservative way looks safe to me, but feel free to object if any issue. > > > > I don't think adding an additional control port named differently > while keeping the cdc-wdm name is adding any simplification in > userspace. I understand your point of view, but if there are users > setting up configuration with fixed cdc-wdm port names, they're > probably not doing it right. I have no idea what's the usual approach > of the kernel for this though, are the port names and subsystem > considered "kernel API"? I do recall in between 3.4 and 3.6 I think > that the subsystem of QMI ports changed from "usb" to "usbmisc"; I > would assume your change to be kind of equivalent and therefore not a > big deal? The ultimate objective is to have a unified view of WWAN devices, whatever the underlying bus or driver is. Accessing /dev/wwanXpY to submit/receive control packets is strictly equivalent to /dev/cdc-wdmX, the goal of keeping the 'legacy' cdc-wdm chardev, is only to prevent breaking of tools relying on the device name. But, as you said, the point is about considering chardev name/driver change as UAPI change or not. From my point of view, this conservative dual-support approach makes sense, If the user/tool is WWAN framework aware, it uses the /dev/wwanXpY port, otherwise /dev/cdc-wdmX can be used (like using DRM/KMS vs legacy framebuffer). I'm however open discussing other strategies: - Do not change anything, keep USB WWAN devices out of the WWAN subsystem. - Migrate cdc-wdm completely to WWAN and get rid of the cdc-wdm chardev - Build time choice, if CONFIG_WWAN, registered to WWAN, otherwise exposed through cdc-wdm chardev. - Run time choice, use either the new WWAN chardev or the legacy cdc-wdm chardev (this patch) - ... I would be interested in getting input from others/maintainers on this. Regards, Loic
Loic Poulain <loic.poulain@linaro.org> writes: > I'm however open discussing other strategies: > - Do not change anything, keep USB WWAN devices out of the WWAN subsystem. > - Migrate cdc-wdm completely to WWAN and get rid of the cdc-wdm chardev > - Build time choice, if CONFIG_WWAN, registered to WWAN, otherwise > exposed through cdc-wdm chardev. > - Run time choice, use either the new WWAN chardev or the legacy > cdc-wdm chardev (this patch) > - ... > > I would be interested in getting input from others/maintainers on this. I appreciate the work you do. When I am silent on the issue, it's because I don't know the best solution (this is a well proven fact :-) and don't want to affect the results by arbitrary comments. There is one thing I can say though: No build time options, please. Bjørn
On Wed, 2021-05-12 at 13:01 +0200, Loic Poulain wrote: > On Wed, 12 May 2021 at 11:04, Aleksander Morgado > <aleksander@aleksander.es> wrote: > > > > Hey, > > > > > > On Tue, May 11, 2021 at 4:33 PM Loic Poulain < > > > > loic.poulain@linaro.org> wrote: > > > > > > > > > > The WWAN framework provides a unified way to handle > > > > > WWAN/modems and its > > > > > control port(s). It has initially been introduced to support > > > > > MHI/PCI > > > > > modems, offering the same control protocols as the USB > > > > > variants such as > > > > > MBIM, QMI, AT... The WWAN framework exposes these control > > > > > protocols as > > > > > character devices, similarly to cdc-wdm, but in a bus > > > > > agnostic fashion. > > > > > > > > > > This change adds registration of the USB modem cdc-wdm > > > > > control endpoints > > > > > to the WWAN framework as standard control ports (wwanXpY...). > > > > > > > > > > Exposing cdc-wdm through WWAN framework normally maintains > > > > > backward > > > > > compatibility, e.g: > > > > > $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get- > > > > > ids > > > > > instead of > > > > > $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids > > > > > > > > > > > > > I have some questions regarding how all this would be seen from > > > > userspace. > > > > > > > > Does the MBIM control port retain the ability to query the > > > > maximum > > > > message size with an ioctl like IOCTL_WDM_MAX_COMMAND? Or is > > > > that > > > > lost? For the libmbim case this may not be a big deal, as we > > > > have a > > > > fallback mechanism to read this value from the USB descriptor > > > > itself, > > > > so just wondering. > > > > > > There is no such ioctl but we can add a sysfs property file as > > > proposed by Dan in the Intel iosm thread. > > > > > > > Yeah, that may be a good thing to add I assume. > > > > > > > > > > Is the sysfs hierarchy maintained for this new port type? i.e. > > > > if > > > > doing "udevadm info -p /sys/class/wwan/wwan0p1QMI -a", would we > > > > still > > > > see the immediate parent device with DRIVERS=="qmi_wwan" and > > > > the > > > > correct interface number/class/subclass/protocol attributes? > > > > > > Not an immediate parent since a port is a child of a logical wwan > > > device, but you'll still be able to get these attributes: > > > Below, DRIVERS=="qmi_wwan". > > > > > > $ udevadm info -p /sys/class/wwan/wwan0p1QMI -a > > > > > > looking at device > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2- > > > 3:1.2/wwan/wwan0/wwan0p1QMI': > > > KERNEL=="wwan0p1QMI" > > > SUBSYSTEM=="wwan" > > > DRIVER=="" > > > > > > looking at parent device > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0': > > > KERNELS=="wwan0" > > > SUBSYSTEMS=="wwan" > > > DRIVERS=="" > > > > > > looking at parent device > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2': > > > KERNELS=="2-3:1.2" > > > SUBSYSTEMS=="usb" > > > DRIVERS=="qmi_wwan" > > > ATTRS{authorized}=="1" > > > ATTRS{bInterfaceNumber}=="02" > > > ATTRS{bInterfaceClass}=="ff" > > > ATTRS{bNumEndpoints}=="03" > > > ATTRS{bInterfaceProtocol}=="ff" > > > ATTRS{bAlternateSetting}==" 0" > > > ATTRS{bInterfaceSubClass}=="ff" > > > ATTRS{interface}=="RmNet" > > > ATTRS{supports_autosuspend}=="1" > > > > > > > Ok, that should be fine, and I think we would not need any > > additional > > change to handle that. The logic looking for what's the driver in > > use > > should still work. > > > > > > > > > > However, some tools may rely on cdc-wdm driver/device name > > > > > for device > > > > > detection. It is then safer to keep the 'legacy' cdc-wdm > > > > > character > > > > > device to prevent any breakage. This is handled in this > > > > > change by > > > > > API mutual exclusion, only one access method can be used at a > > > > > time, > > > > > either cdc-wdm chardev or WWAN API. > > > > > > > > How does this mutual exclusion API work? Is the kernel going to > > > > expose > > > > 2 different chardevs always for the single control port? > > > > > > Yes, if cdc-wdm0 is open, wwan0p1QMI can not be open (-EBUSY), > > > and vice versa. > > > > > > > Oh... but then, what's the benefit of adding the new wwan0p1QMI > > port? > > I may be biased because I have always the MM case in mind, and in > > there we'll need to support both things, but doesn't this new port > > add > > more complexity than making it simpler? I would have thought that > > it's > > either a cdc-wdm port or a wwan port, but both? Wouldn't it make > > more > > sense to default to the new wwan subsystem if the wwan subsystem is > > built, and otherwise fallback to cdc-wdm? (i.e. a build time > > option). > > Having two chardevs to manage exactly the same control port, and > > having them mutually exclusive is a bit strange. > > > > > > > > really want to do that? > > > > > > This conservative way looks safe to me, but feel free to object > > > if any issue. > > > > > > > I don't think adding an additional control port named differently > > while keeping the cdc-wdm name is adding any simplification in > > userspace. I understand your point of view, but if there are users > > setting up configuration with fixed cdc-wdm port names, they're > > probably not doing it right. I have no idea what's the usual > > approach > > of the kernel for this though, are the port names and subsystem > > considered "kernel API"? I do recall in between 3.4 and 3.6 I think > > that the subsystem of QMI ports changed from "usb" to "usbmisc"; I > > would assume your change to be kind of equivalent and therefore not > > a > > big deal? > > > The ultimate objective is to have a unified view of WWAN devices, > whatever the underlying bus or driver is. Accessing /dev/wwanXpY to > submit/receive control packets is strictly equivalent to > /dev/cdc-wdmX, the goal of keeping the 'legacy' cdc-wdm chardev, is > only to prevent breaking of tools relying on the device name. But, as > you said, the point is about considering chardev name/driver change > as > UAPI change or not. From my point of view, this conservative > dual-support approach makes sense, If the user/tool is WWAN framework > aware, it uses the /dev/wwanXpY port, otherwise /dev/cdc-wdmX can be > used (like using DRM/KMS vs legacy framebuffer). Names change and have changed in the past. It's sometimes painful but tools *already* should not be relying on a specific device name. eg if you have a tool that hardcodes /dev/cdc-wdm0 there is already no guarantee that you'll get the same port next time, especially with USB. Ethernet devices have never been stable, which is why udev and systemd have renamed them to enp0s31f6 and wlp61s0. We also change WWAN ethernet port device names whenever a new device gets tagged with the WWAN hint. I agree with Aleksander here, I think having two different devices is not a great solution and will be more confusing. I do realize that changing the name can break existing setups but again, names are already not stable. > I'm however open discussing other strategies: > - Do not change anything, keep USB WWAN devices out of the WWAN > subsystem. -1 from me, consistency is better. > - Migrate cdc-wdm completely to WWAN and get rid of the cdc-wdm > chardev +1 from me > - Build time choice, if CONFIG_WWAN, registered to WWAN, otherwise > exposed through cdc-wdm chardev. Agree with Bjorn, -1 > - Run time choice, use either the new WWAN chardev or the legacy > cdc-wdm chardev (this patch) Agree with Aleksander, -1. This is even more confusing than a name change. > - ... > > I would be interested in getting input from others/maintainers on > this. Input from Oliver and Greg KH would be useful, since Greg was heavily involved with the ethernet/wifi etc device renaming in the past IIRC. But another thought: why couldn't wwan_create_port() take a devname template like alloc_netdev() does and cdc-wdm can just pass "cdc- wdm%d"? eg, why do we need to change the name? Tools that care about finding WWAN devices should be looking at sysfs attributes/links and TYPE=XXXX in uevent, not at the device name. Nothing should be looking at device name really. Dan
Hi Dan, On Wed, 12 May 2021 at 18:34, Dan Williams <dcbw@gapps.redhat.com> wrote: > > On Wed, 2021-05-12 at 13:01 +0200, Loic Poulain wrote: > > On Wed, 12 May 2021 at 11:04, Aleksander Morgado > > <aleksander@aleksander.es> wrote: > > > > > > Hey, > > > > > > > > On Tue, May 11, 2021 at 4:33 PM Loic Poulain < > > > > > loic.poulain@linaro.org> wrote: > > > > > > > > > > > > The WWAN framework provides a unified way to handle > > > > > > WWAN/modems and its > > > > > > control port(s). It has initially been introduced to support > > > > > > MHI/PCI > > > > > > modems, offering the same control protocols as the USB > > > > > > variants such as > > > > > > MBIM, QMI, AT... The WWAN framework exposes these control > > > > > > protocols as > > > > > > character devices, similarly to cdc-wdm, but in a bus > > > > > > agnostic fashion. > > > > > > > > > > > > This change adds registration of the USB modem cdc-wdm > > > > > > control endpoints > > > > > > to the WWAN framework as standard control ports (wwanXpY...). > > > > > > > > > > > > Exposing cdc-wdm through WWAN framework normally maintains > > > > > > backward > > > > > > compatibility, e.g: > > > > > > $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get- > > > > > > ids > > > > > > instead of > > > > > > $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids > > > > > > > > > > > > > > > > I have some questions regarding how all this would be seen from > > > > > userspace. > > > > > > > > > > Does the MBIM control port retain the ability to query the > > > > > maximum > > > > > message size with an ioctl like IOCTL_WDM_MAX_COMMAND? Or is > > > > > that > > > > > lost? For the libmbim case this may not be a big deal, as we > > > > > have a > > > > > fallback mechanism to read this value from the USB descriptor > > > > > itself, > > > > > so just wondering. > > > > > > > > There is no such ioctl but we can add a sysfs property file as > > > > proposed by Dan in the Intel iosm thread. > > > > > > > > > > Yeah, that may be a good thing to add I assume. > > > > > > > > > > > > > Is the sysfs hierarchy maintained for this new port type? i.e. > > > > > if > > > > > doing "udevadm info -p /sys/class/wwan/wwan0p1QMI -a", would we > > > > > still > > > > > see the immediate parent device with DRIVERS=="qmi_wwan" and > > > > > the > > > > > correct interface number/class/subclass/protocol attributes? > > > > > > > > Not an immediate parent since a port is a child of a logical wwan > > > > device, but you'll still be able to get these attributes: > > > > Below, DRIVERS=="qmi_wwan". > > > > > > > > $ udevadm info -p /sys/class/wwan/wwan0p1QMI -a > > > > > > > > looking at device > > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2- > > > > 3:1.2/wwan/wwan0/wwan0p1QMI': > > > > KERNEL=="wwan0p1QMI" > > > > SUBSYSTEM=="wwan" > > > > DRIVER=="" > > > > > > > > looking at parent device > > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2/wwan/wwan0': > > > > KERNELS=="wwan0" > > > > SUBSYSTEMS=="wwan" > > > > DRIVERS=="" > > > > > > > > looking at parent device > > > > '/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.2': > > > > KERNELS=="2-3:1.2" > > > > SUBSYSTEMS=="usb" > > > > DRIVERS=="qmi_wwan" > > > > ATTRS{authorized}=="1" > > > > ATTRS{bInterfaceNumber}=="02" > > > > ATTRS{bInterfaceClass}=="ff" > > > > ATTRS{bNumEndpoints}=="03" > > > > ATTRS{bInterfaceProtocol}=="ff" > > > > ATTRS{bAlternateSetting}==" 0" > > > > ATTRS{bInterfaceSubClass}=="ff" > > > > ATTRS{interface}=="RmNet" > > > > ATTRS{supports_autosuspend}=="1" > > > > > > > > > > Ok, that should be fine, and I think we would not need any > > > additional > > > change to handle that. The logic looking for what's the driver in > > > use > > > should still work. > > > > > > > > > > > > > However, some tools may rely on cdc-wdm driver/device name > > > > > > for device > > > > > > detection. It is then safer to keep the 'legacy' cdc-wdm > > > > > > character > > > > > > device to prevent any breakage. This is handled in this > > > > > > change by > > > > > > API mutual exclusion, only one access method can be used at a > > > > > > time, > > > > > > either cdc-wdm chardev or WWAN API. > > > > > > > > > > How does this mutual exclusion API work? Is the kernel going to > > > > > expose > > > > > 2 different chardevs always for the single control port? > > > > > > > > Yes, if cdc-wdm0 is open, wwan0p1QMI can not be open (-EBUSY), > > > > and vice versa. > > > > > > > > > > Oh... but then, what's the benefit of adding the new wwan0p1QMI > > > port? > > > I may be biased because I have always the MM case in mind, and in > > > there we'll need to support both things, but doesn't this new port > > > add > > > more complexity than making it simpler? I would have thought that > > > it's > > > either a cdc-wdm port or a wwan port, but both? Wouldn't it make > > > more > > > sense to default to the new wwan subsystem if the wwan subsystem is > > > built, and otherwise fallback to cdc-wdm? (i.e. a build time > > > option). > > > Having two chardevs to manage exactly the same control port, and > > > having them mutually exclusive is a bit strange. > > > > > > > > > > > really want to do that? > > > > > > > > This conservative way looks safe to me, but feel free to object > > > > if any issue. > > > > > > > > > > I don't think adding an additional control port named differently > > > while keeping the cdc-wdm name is adding any simplification in > > > userspace. I understand your point of view, but if there are users > > > setting up configuration with fixed cdc-wdm port names, they're > > > probably not doing it right. I have no idea what's the usual > > > approach > > > of the kernel for this though, are the port names and subsystem > > > considered "kernel API"? I do recall in between 3.4 and 3.6 I think > > > that the subsystem of QMI ports changed from "usb" to "usbmisc"; I > > > would assume your change to be kind of equivalent and therefore not > > > a > > > big deal? > > > > > > The ultimate objective is to have a unified view of WWAN devices, > > whatever the underlying bus or driver is. Accessing /dev/wwanXpY to > > submit/receive control packets is strictly equivalent to > > /dev/cdc-wdmX, the goal of keeping the 'legacy' cdc-wdm chardev, is > > only to prevent breaking of tools relying on the device name. But, as > > you said, the point is about considering chardev name/driver change > > as > > UAPI change or not. From my point of view, this conservative > > dual-support approach makes sense, If the user/tool is WWAN framework > > aware, it uses the /dev/wwanXpY port, otherwise /dev/cdc-wdmX can be > > used (like using DRM/KMS vs legacy framebuffer). > > Names change and have changed in the past. It's sometimes painful but > tools *already* should not be relying on a specific device name. eg if > you have a tool that hardcodes /dev/cdc-wdm0 there is already no > guarantee that you'll get the same port next time, especially with USB. > > Ethernet devices have never been stable, which is why udev and systemd > have renamed them to enp0s31f6 and wlp61s0. We also change WWAN > ethernet port device names whenever a new device gets tagged with the > WWAN hint. > > I agree with Aleksander here, I think having two different devices is > not a great solution and will be more confusing. > > I do realize that changing the name can break existing setups but > again, names are already not stable. > > > I'm however open discussing other strategies: > > - Do not change anything, keep USB WWAN devices out of the WWAN > > subsystem. > > -1 from me, consistency is better. > > > - Migrate cdc-wdm completely to WWAN and get rid of the cdc-wdm > > chardev > > +1 from me > > > - Build time choice, if CONFIG_WWAN, registered to WWAN, otherwise > > exposed through cdc-wdm chardev. > > Agree with Bjorn, -1 > > > - Run time choice, use either the new WWAN chardev or the legacy > > cdc-wdm chardev (this patch) > > Agree with Aleksander, -1. This is even more confusing than a name > change. > > > - ... > > > > I would be interested in getting input from others/maintainers on > > this. > > Input from Oliver and Greg KH would be useful, since Greg was heavily > involved with the ethernet/wifi etc device renaming in the past IIRC. > > But another thought: why couldn't wwan_create_port() take a devname > template like alloc_netdev() does and cdc-wdm can just pass "cdc- > wdm%d"? eg, why do we need to change the name? > Tools that care about > finding WWAN devices should be looking at sysfs attributes/links and > TYPE=XXXX in uevent, not at the device name. Nothing should be looking > at device name really. Right, passing the legacy cdc-wdm dev-name as a parameter seems to be a fair solution, making the transition smoother. Regards, Loic
> > But another thought: why couldn't wwan_create_port() take a devname > > template like alloc_netdev() does and cdc-wdm can just pass "cdc- > > wdm%d"? eg, why do we need to change the name? > > Tools that care about > > finding WWAN devices should be looking at sysfs attributes/links and > > TYPE=XXXX in uevent, not at the device name. Nothing should be looking > > at device name really. > > Right, passing the legacy cdc-wdm dev-name as a parameter seems to be > a fair solution, making the transition smoother. > Reusing the cdc-wdm name is indeed a good compromise; plus then reading port type (QMI, MBIM...) via sysfs attributes as you suggested in your last patch would make it work perfectly fine in userspace.
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c index 5db6627..42fb750 100644 --- a/drivers/net/usb/cdc_mbim.c +++ b/drivers/net/usb/cdc_mbim.c @@ -168,6 +168,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf) subdriver = usb_cdc_wdm_register(ctx->control, &dev->status->desc, le16_to_cpu(ctx->mbim_desc->wMaxControlMessage), + WWAN_PORT_MBIM, cdc_mbim_wdm_manage_power); if (IS_ERR(subdriver)) { ret = PTR_ERR(subdriver); diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c index a87f0da..849b773 100644 --- a/drivers/net/usb/huawei_cdc_ncm.c +++ b/drivers/net/usb/huawei_cdc_ncm.c @@ -96,6 +96,7 @@ static int huawei_cdc_ncm_bind(struct usbnet *usbnet_dev, subdriver = usb_cdc_wdm_register(ctx->control, &usbnet_dev->status->desc, 1024, /* wMaxCommand */ + WWAN_PORT_AT, huawei_cdc_ncm_wdm_manage_power); if (IS_ERR(subdriver)) { ret = PTR_ERR(subdriver); diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c index 17a0505..11c898f 100644 --- a/drivers/net/usb/qmi_wwan.c +++ b/drivers/net/usb/qmi_wwan.c @@ -724,7 +724,8 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev) /* register subdriver */ subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, - 4096, &qmi_wwan_cdc_wdm_manage_power); + 4096, WWAN_PORT_QMI, + &qmi_wwan_cdc_wdm_manage_power); if (IS_ERR(subdriver)) { dev_err(&info->control->dev, "subdriver registration failed\n"); rv = PTR_ERR(subdriver); diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 508b1c3..457b00c 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -21,8 +21,10 @@ #include <linux/uaccess.h> #include <linux/bitops.h> #include <linux/poll.h> +#include <linux/skbuff.h> #include <linux/usb.h> #include <linux/usb/cdc.h> +#include <linux/wwan.h> #include <asm/byteorder.h> #include <asm/unaligned.h> #include <linux/usb/cdc-wdm.h> @@ -55,6 +57,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids); #define WDM_SUSPENDING 8 #define WDM_RESETTING 9 #define WDM_OVERFLOW 10 +#define WDM_WWAN_IN_USE 11 #define WDM_MAX 16 @@ -106,6 +109,9 @@ struct wdm_device { struct list_head device_list; int (*manage_power)(struct usb_interface *, int); + + enum wwan_port_type wwanp_type; + struct wwan_port *wwanp; }; static struct usb_driver wdm_driver; @@ -157,6 +163,8 @@ static void wdm_out_callback(struct urb *urb) wake_up_all(&desc->wait); } +static void wdm_wwan_rx(struct wdm_device *desc, int length); + static void wdm_in_callback(struct urb *urb) { unsigned long flags; @@ -192,6 +200,11 @@ static void wdm_in_callback(struct urb *urb) } } + if (test_bit(WDM_WWAN_IN_USE, &desc->flags)) { + wdm_wwan_rx(desc, length); + goto out; + } + /* * only set a new error if there is no previous error. * Errors are only cleared during read/open @@ -226,6 +239,7 @@ static void wdm_in_callback(struct urb *urb) set_bit(WDM_READ, &desc->flags); wake_up(&desc->wait); } +out: spin_unlock_irqrestore(&desc->iuspin, flags); } @@ -697,6 +711,11 @@ static int wdm_open(struct inode *inode, struct file *file) goto out; file->private_data = desc; + if (test_bit(WDM_WWAN_IN_USE, &desc->flags)) { + rv = -EBUSY; + goto out; + } + rv = usb_autopm_get_interface(desc->intf); if (rv < 0) { dev_err(&desc->intf->dev, "Error autopm - %d\n", rv); @@ -792,6 +811,151 @@ static struct usb_class_driver wdm_class = { .minor_base = WDM_MINOR_BASE, }; +/* --- WWAN framework integration --- */ +#ifdef CONFIG_WWAN +static int wdm_wwan_port_start(struct wwan_port *port) +{ + struct wdm_device *desc = wwan_port_get_drvdata(port); + + /* The interface is both exposed via the WWAN framework and as a + * legacy usbmisc chardev. If chardev is already open, just fail + * to prevent concurrent usage. Otherwise, switch to WWAN mode. + */ + mutex_lock(&wdm_mutex); + if (desc->count) { + mutex_unlock(&wdm_mutex); + return -EBUSY; + } + set_bit(WDM_WWAN_IN_USE, &desc->flags); + mutex_unlock(&wdm_mutex); + + desc->manage_power(desc->intf, 1); + + /* tx is allowed */ + wwan_port_txon(port); + + /* Start getting events */ + return usb_submit_urb(desc->validity, GFP_KERNEL); +} + +static void wdm_wwan_port_stop(struct wwan_port *port) +{ + struct wdm_device *desc = wwan_port_get_drvdata(port); + + /* Stop all transfers and disable WWAN mode */ + kill_urbs(desc); + desc->manage_power(desc->intf, 0); + clear_bit(WDM_READ, &desc->flags); + clear_bit(WDM_WWAN_IN_USE, &desc->flags); +} + +static void wdm_wwan_port_tx_complete(struct urb *urb) +{ + struct sk_buff *skb = urb->context; + struct wdm_device *desc = skb_shinfo(skb)->destructor_arg; + + usb_autopm_put_interface(desc->intf); + wwan_port_txon(desc->wwanp); + kfree_skb(skb); +} + +static int wdm_wwan_port_tx(struct wwan_port *port, struct sk_buff *skb) +{ + struct wdm_device *desc = wwan_port_get_drvdata(port); + struct usb_interface *intf = desc->intf; + struct usb_ctrlrequest *req = desc->orq; + int rv; + + rv = usb_autopm_get_interface(intf); + if (rv) + return rv; + + usb_fill_control_urb( + desc->command, + interface_to_usbdev(intf), + usb_sndctrlpipe(interface_to_usbdev(intf), 0), + (unsigned char *)req, + skb->data, + skb->len, + wdm_wwan_port_tx_complete, + skb + ); + + req->bRequestType = (USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE); + req->bRequest = USB_CDC_SEND_ENCAPSULATED_COMMAND; + req->wValue = 0; + req->wIndex = desc->inum; + req->wLength = cpu_to_le16(skb->len); + + skb_shinfo(skb)->destructor_arg = desc; + + rv = usb_submit_urb(desc->command, GFP_KERNEL); + if (rv) + usb_autopm_put_interface(intf); + else /* One transfer at a time, stop TX until URB completion */ + wwan_port_txoff(port); + + return rv; +} + +static struct wwan_port_ops wdm_wwan_port_ops = { + .start = wdm_wwan_port_start, + .stop = wdm_wwan_port_stop, + .tx = wdm_wwan_port_tx, +}; + +static void wdm_wwan_init(struct wdm_device *desc) +{ + struct usb_interface *intf = desc->intf; + struct wwan_port *port; + + /* Only register to WWAN core if protocol/type is known */ + if (desc->wwanp_type == WWAN_PORT_UNKNOWN) { + dev_info(&intf->dev, "Unknown control protocol\n"); + return; + } + + port = wwan_create_port(&intf->dev, desc->wwanp_type, &wdm_wwan_port_ops, desc); + if (IS_ERR(port)) { + dev_err(&intf->dev, "%s: Unable to create WWAN port\n", + dev_name(intf->usb_dev)); + return; + } + + desc->wwanp = port; +} + +static void wdm_wwan_deinit(struct wdm_device *desc) +{ + if (!desc->wwanp) + return; + + wwan_remove_port(desc->wwanp); + desc->wwanp = NULL; +} + +static void wdm_wwan_rx(struct wdm_device *desc, int length) +{ + struct wwan_port *port = desc->wwanp; + struct sk_buff *skb; + + /* Forward data to WWAN port */ + skb = alloc_skb(length, GFP_ATOMIC); + if (!skb) + return; + + memcpy(skb_put(skb, length), desc->inbuf, length); + wwan_port_rx(port, skb); + + /* inbuf has been copied, it is safe to check for outstanding data */ + schedule_work(&desc->service_outs_intr); +} +#else /* CONFIG_WWAN */ +static void wdm_wwan_init(struct wdm_device *desc) {} +static void wdm_wwan_deinit(struct wdm_device *desc) {} +static void wdm_wwan_rx(struct wdm_device *desc, int length) {} +#endif /* CONFIG_WWAN */ + /* --- error handling --- */ static void wdm_rxwork(struct work_struct *work) { @@ -836,7 +1000,8 @@ static void service_interrupt_work(struct work_struct *work) /* --- hotplug --- */ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor *ep, - u16 bufsize, int (*manage_power)(struct usb_interface *, int)) + u16 bufsize, enum wwan_port_type type, + int (*manage_power)(struct usb_interface *, int)) { int rv = -ENOMEM; struct wdm_device *desc; @@ -853,6 +1018,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor /* this will be expanded and needed in hardware endianness */ desc->inum = cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber); desc->intf = intf; + desc->wwanp_type = type; INIT_WORK(&desc->rxwork, wdm_rxwork); INIT_WORK(&desc->service_outs_intr, service_interrupt_work); @@ -933,6 +1099,9 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor goto err; else dev_info(&intf->dev, "%s: USB WDM device\n", dev_name(intf->usb_dev)); + + wdm_wwan_init(desc); + out: return rv; err: @@ -977,7 +1146,7 @@ static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id) goto err; ep = &iface->endpoint[0].desc; - rv = wdm_create(intf, ep, maxcom, &wdm_manage_power); + rv = wdm_create(intf, ep, maxcom, WWAN_PORT_UNKNOWN, &wdm_manage_power); err: return rv; @@ -988,6 +1157,7 @@ static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id) * @intf: usb interface the subdriver will associate with * @ep: interrupt endpoint to monitor for notifications * @bufsize: maximum message size to support for read/write + * @type: Type/protocol of the transported data (MBIM, QMI...) * @manage_power: call-back invoked during open and release to * manage the device's power * Create WDM usb class character device and associate it with intf @@ -1005,12 +1175,12 @@ static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id) */ struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf, struct usb_endpoint_descriptor *ep, - int bufsize, + int bufsize, enum wwan_port_type type, int (*manage_power)(struct usb_interface *, int)) { int rv; - rv = wdm_create(intf, ep, bufsize, manage_power); + rv = wdm_create(intf, ep, bufsize, type, manage_power); if (rv < 0) goto err; @@ -1029,6 +1199,8 @@ static void wdm_disconnect(struct usb_interface *intf) desc = wdm_find_device(intf); mutex_lock(&wdm_mutex); + wdm_wwan_deinit(desc); + /* the spinlock makes sure no new urbs are generated in the callbacks */ spin_lock_irqsave(&desc->iuspin, flags); set_bit(WDM_DISCONNECTING, &desc->flags); diff --git a/include/linux/usb/cdc-wdm.h b/include/linux/usb/cdc-wdm.h index 9b895f9..9f5a51f 100644 --- a/include/linux/usb/cdc-wdm.h +++ b/include/linux/usb/cdc-wdm.h @@ -12,11 +12,12 @@ #ifndef __LINUX_USB_CDC_WDM_H #define __LINUX_USB_CDC_WDM_H +#include <linux/wwan.h> #include <uapi/linux/usb/cdc-wdm.h> extern struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf, struct usb_endpoint_descriptor *ep, - int bufsize, + int bufsize, enum wwan_port_type type, int (*manage_power)(struct usb_interface *, int)); #endif /* __LINUX_USB_CDC_WDM_H */
The WWAN framework provides a unified way to handle WWAN/modems and its control port(s). It has initially been introduced to support MHI/PCI modems, offering the same control protocols as the USB variants such as MBIM, QMI, AT... The WWAN framework exposes these control protocols as character devices, similarly to cdc-wdm, but in a bus agnostic fashion. This change adds registration of the USB modem cdc-wdm control endpoints to the WWAN framework as standard control ports (wwanXpY...). Exposing cdc-wdm through WWAN framework normally maintains backward compatibility, e.g: $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get-ids instead of $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids However, some tools may rely on cdc-wdm driver/device name for device detection. It is then safer to keep the 'legacy' cdc-wdm character device to prevent any breakage. This is handled in this change by API mutual exclusion, only one access method can be used at a time, either cdc-wdm chardev or WWAN API. Note that unknown channel types (other than MBIM, AT or MBIM) are not registered to the WWAN framework. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- v2: - Fix pm runtime while ongoing transmission - Use WWAN port type instead of CDC specific ones - Change port_start ordering - Handle usb_submit_urb return value - Fix build issue reported by kernel-test-rebot drivers/net/usb/cdc_mbim.c | 1 + drivers/net/usb/huawei_cdc_ncm.c | 1 + drivers/net/usb/qmi_wwan.c | 3 +- drivers/usb/class/cdc-wdm.c | 180 ++++++++++++++++++++++++++++++++++++++- include/linux/usb/cdc-wdm.h | 3 +- 5 files changed, 182 insertions(+), 6 deletions(-)