Message-ID: <CAMZdPi9zABtXoKiUuE9mmbnYsSmZoVWR+nLAdq0O5b7=Ghh-rg@mail.gmail.com>
Date:   Wed, 12 May 2021 13:01:55 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Aleksander Morgado <aleksander@...ksander.es>,
        Jakub Kicinski <kuba@...nel.org>,
        Oliver Neukum <oliver@...kum.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Bjørn Mork <bjorn@...k.no>,
        Network Development <netdev@...r.kernel.org>,
        dcbw@...ps.redhat.com
Subject: Re: [PATCH net-next v2 2/2] usb: class: cdc-wdm: WWAN framework integration

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).

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

Powered by blists - more mailing lists