Message-ID: <87ft2izdtw.fsf@miraculix.mork.no>
Date:   Sat, 30 Jan 2021 15:42:03 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Loic Poulain <loic.poulain@...aro.org>
Cc:     kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org,
        carl.yin@...ctel.com
Subject: Re: [PATCH net-next 3/3] net: mhi: Add mbim proto

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

> 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?

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

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

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.

> +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?

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?

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

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.

> +
> +		/* 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.

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

Powered by blists - more mailing lists