On Sat, 30 Jan 2021 at 15:42, Bjørn Mork <bj...@mork.no> wrote:
>
> Loic Poulain <loic.poul...@linaro.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
