Date:   Wed, 19 May 2021 11:25:46 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Dan Williams <dcbw@...ps.redhat.com>
Cc:     Aleksander Morgado <aleksander@...ksander.es>,
        Jakub Kicinski <kuba@...nel.org>,
        Oliver Neukum <oliver@...kum.org>,
        "David S. Miller" <davem@...emloft.net>,
        Bjørn Mork <bjorn@...k.no>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] usb: class: cdc-wdm: WWAN framework integration

Hi Dan,





On Wed, 12 May 2021 at 18:34, Dan Williams <dcbw@...ps.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@...ksander.es> wrote:
> > >
> > > Hey,
> > >
> > > > > On Tue, May 11, 2021 at 4:33 PM Loic Poulain <
> > > > > loic.poulain@...aro.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

Powered by blists - more mailing lists