Message-ID: <CAMZdPi-7NYL3+baxfqSVsQ+X7QdQv4zou-=p_Y8cLSTya3YSjQ@mail.gmail.com>
Date:   Mon, 1 Feb 2021 10:23:56 +0100
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Carl Yin(殷张成) <carl.yin@...ctel.com>
Subject: Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

On Sat, 30 Jan 2021 at 15:42, Bjørn Mork <bjorn@...k.no> wrote:
>
> Loic Poulain <loic.poulain@...aro.org> writes:
>
> > MBIM has initially been specified by USB-IF for transporting data (IP)
> > between a modem and a host over USB. However some modern modems also
> > support MBIM over PCIe (via MHI). In the same way as QMAP(rmnet), it
> > allows to aggregate IP packets and to perform context multiplexing.
> >
> > This change adds minimal MBIM support to MHI, allowing to support MBIM
> > only modems. MBIM being based on USB NCM, it reuses some helpers from
> > the USB stack, but the cdc-mbim driver is too USB coupled to be reused.
>
> Sure, the guts of the MBIM protocol is in cdc_ncm. But you did copy most
> of cdc_mbim_rx_fixup() from cdc_mbim.c so this comment doesn't make
> much sense...

Yes, just wanted to say that I have to duplicate mbim functions here
because of the USB coupling of the cdc_mbim.c version.

>
> > At some point it would be interesting to move on a factorized solution,
> > having a generic MBIM network lib or dedicated MBIM netlink virtual
> > interface support.
>
> I believe that is now or never.  Sorry.  No one is going to fix it
> later.
>
> > +static int mbim_rx_verify_nth16(struct sk_buff *skb)
> > +{
> > +     struct usb_cdc_ncm_nth16 *nth16;
> > +     int ret = -EINVAL;
> > +
> > +     if (skb->len < (sizeof(struct usb_cdc_ncm_nth16) +
> > +                     sizeof(struct usb_cdc_ncm_ndp16))) {
> > +             goto error;
> > +     }
> > +
> > +     nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
> > +
> > +     if (nth16->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN))
> > +             goto error;
> > +
> > +     ret = le16_to_cpu(nth16->wNdpIndex);
> > +error:
> > +     return ret;
> > +}
>
>
> This is a copy of  cdc_ncm_rx_verify_nth16() except that you've dropped
> the debug messages and verification of wBlockLength and wSequence.  It's
> unclear to me why you don't need to verify those fields?

Yes, I can probably re-introduce that, to be aligned with USB version
(I removed that because I initially had no context for mbim).

>
> This function could easily be shared with cdc_ncm instead of duplicating
> it.

Yes but that would request cdc_mbim changes since this function takes
a USB mbim context (cdc_ncm_ctx).

>
> > +static int mbim_rx_verify_ndp16(struct sk_buff *skb, int ndpoffset)
> > +{
> > +     struct usb_cdc_ncm_ndp16 *ndp16;
> > +     int ret = -EINVAL;
> > +
> > +     if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb->len)
> > +             goto error;
> > +
> > +     ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> > +
> > +     if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN)
> > +             goto error;
> > +
> > +     ret = ((le16_to_cpu(ndp16->wLength) -
> > +                                     sizeof(struct usb_cdc_ncm_ndp16)) /
> > +                                     sizeof(struct usb_cdc_ncm_dpe16));
> > +     ret--; /* Last entry is always a NULL terminator */
> > +
> > +     if ((sizeof(struct usb_cdc_ncm_ndp16) +
> > +          ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb->len) {
> > +             ret = -EINVAL;
> > +     }
> > +error:
> > +     return ret;
> > +}
>
> This is an exact replica of cdc_ncm_rx_verify_ndp16() AFAICS, except for
> the removed debug messages.   You do know that netif_dbg() is
> conditional? There is nothing to be saved by removing those lines.

Yes, I tried to make this function generic, it only takes the
skb/offset as parameters, and lets the caller handling error as
desired. but I can certainly re-introduce dbg messages, along with a
ndev parameter (for the print context).

>
> FWIW, you will have to fix the copyright attribution of this file if you
> want to keep this copy here.  Otherwise it just looks like you are
> stealing. And I'll wonder where the rest of the code came from and
> whether you have the right to license that as GPL. Better be clear about
> where you found this and who owns the copyright.  There is no question
> about the rights to use, given the GPL license of the original.

I'll add the proper copyright from cdc-mbim since it's clearly a
partial-copy of it (and no will to hide that).

> > +static int mbim_rx_fixup(struct net_device *ndev, struct sk_buff *skb)
> > +{
> > +     int ndpoffset;
> > +
> > +     /* Check NTB header signature and retrieve first NDP offset */
> > +     ndpoffset = mbim_rx_verify_nth16(skb);
> > +     if (ndpoffset < 0) {
> > +             netdev_err(ndev, "MBIM: Incorrect NTB header\n");
> > +             goto error;
> > +     }
> > +
> > +     /* Process each NDP */
> > +     while (1) {
> > +             struct usb_cdc_ncm_ndp16 *ndp16;
> > +             struct usb_cdc_ncm_dpe16 *dpe16;
> > +             int nframes, n;
> > +
> > +             /* Check NDP header and retrieve number of datagrams */
> > +             nframes = mbim_rx_verify_ndp16(skb, ndpoffset);
> > +             if (nframes < 0) {
> > +                     netdev_err(ndev, "MBIM: Incorrect NDP16\n");
> > +                     goto error;
> > +             }
> > +
> > +             /* Only support the IPS session 0 for now */
> > +             ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> > +             switch (ndp16->dwSignature & cpu_to_le32(0x00ffffff)) {
> > +             case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
> > +                     break;
> > +             default:
> > +                     netdev_err(ndev, "MBIM: Unsupported NDP type\n");
> > +                     goto next_ndp;
> > +             }
>
> You don't support DSS?  Why?  That's mandatory in the MBIM spec, isn't
> it?  Can we have an MBIM driver without that support?  And if so, should
> completely valid MBIM frames cause an error message?

Well, this is a subset of MBIM, since a lot of the MBIM/NCM does not
apply for MHI/PCIe. In MHI context, the IP channel is used for network
data transport only, and MBIM simply brings aggregation and
multiplexing. Other modem functions/services are exposed via other
dedicated MHI channels.

>
> And IP multiplexing isn't supported either?  And you simply ignore the
> session ID?  How is that intended to work?  What happens here when the
> driver receives IP packets from two different APNs?

You're Right, this is on purpose, I would like to keep the initial
implementation simple, working for the main use case. So multi-pdn
context is simply not supported in that series. Moreover, I can not
test multi-context for now, but I plan to add that in a follow-up
series/patch.

> But please, just implement the IP multiplexing.  You do that for rmnet,
> right?

I especially would like to discuss the implementation since the
architecture is quite different from rmnet netlink (do we want to
create additional ifaces or to align with the VLAN cdc-mbim trick...).

>
> At least provide some plan on how you want to add it. Don't paint
> yourself into a corner.  Userspace will need a way to manage the MBIM
> transport and the multiplexed IP sessions independently.  E.g. take down
> the netdev associated with IPS session 0 without breaking IPS session 1.
>
> Locking this netdev to one session will be a problem.  I know, because
> I've made that mistake.

I think it makes sense to align with cdc-mbim behavior here, for
compatiblity, so the lower transport interface will have to stay up
for the additional session/context interfaces. Let me know we should
do that in another way.

>
> > +
> > +             /* de-aggregate and deliver IP packets */
> > +             dpe16 = ndp16->dpe16;
> > +             for (n = 0; n < nframes; n++, dpe16++) {
> > +                     u16 dgram_offset = le16_to_cpu(dpe16->wDatagramIndex);
> > +                     u16 dgram_len = le16_to_cpu(dpe16->wDatagramLength);
> > +                     struct sk_buff *skbn;
> > +
> > +                     if (!dgram_offset || !dgram_len)
> > +                             break; /* null terminator */
> > +
> > +                     skbn = netdev_alloc_skb(ndev, dgram_len);
> > +                     if (!skbn)
> > +                             continue;
> > +
> > +                     skb_put(skbn, dgram_len);
> > +                     memcpy(skbn->data, skb->data + dgram_offset, dgram_len);
> > +
> > +                     switch (skbn->data[0] & 0xf0) {
> > +                     case 0x40:
> > +                             skbn->protocol = htons(ETH_P_IP);
> > +                             break;
> > +                     case 0x60:
> > +                             skbn->protocol = htons(ETH_P_IPV6);
> > +                             break;
> > +                     default:
> > +                             netdev_err(ndev, "MBIM: unknown protocol\n");
> > +                             continue;
> > +                     }
> > +
> > +                     netif_rx(skbn);
> > +             }
> > +next_ndp:
> > +             /* Other NDP to process? */
> > +             ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
> > +             if (!ndpoffset)
> > +                     break;
> > +     }
> > +
> > +     /* free skb */
> > +     dev_consume_skb_any(skb);
> > +     return 0;
> > +error:
> > +     dev_kfree_skb_any(skb);
> > +     return -EIO;
> > +}
>
>
> Except for the missing feature, this is still mostly a copy
> cdc_mbim_rx_fixup(). Please respect the copyright on code you are
> copying.  You are obviously free to use this under the GPL, but the
> original author still retains copyright on it.

For sure, will do.

>
> FWIW, I can understand why you want to use a slightly modified copy in
> this case, since the original is tied both to usbnet and to the weird
> VLAN mapping.  So that's fine with me.
>
>
> Bjørn

Thanks,
Loic

Powered by blists - more mailing lists