RFC: [PATCH 3/3] usb: cdc_ncm: MirrorLink booster for N60x,70x

RFC: [PATCH 3/3] usb: cdc_ncm: MirrorLink booster for N60x,70x

[Loic Domaigne]

This patch allows to boost the performance if used for MirrorLink(TM). It
is derived from a patch provided by Nokia-LC/Berlin. It optimizes the transfer
speed for the Nokia N60x,70x. which requires special settings (jumbo frame, 
no batching)

This patch applies to longterm kernel version 3.4.35.

Signed-Off-By: Loic Domaigne <loic.domaigne-NEfzRHNIQpPQT0dZR+AlfA@public.gmane.org> 

--- linux-3.4.35/drivers/net/usb/cdc_ncm.c.patch2	2013-03-05 10:34:56.639566116 +0100
+++ linux-3.4.35/drivers/net/usb/cdc_ncm.c	2013-03-05 10:54:33.941773425 +0100
@@ -38,6 +38,22 @@
  * SUCH DAMAGE.
  */
 
+/* MirrorLink(TM) Booster patch
+ *
+ * Copyright (c) jambit GmbH
+ * Contact: Loic Domaigne <loic.domaigne-NEfzRHNIQpPQT0dZR+AlfA@public.gmane.org>
+ *
+ * Contribution:
+ * Part of this work is derived from a patch provided by Nokia-LC/Berlin
+ * (thanks Juan!), which was licensed under the terms of the GNU General
+ * Public License (GPL) Version 2
+ *
+ * Licensing:
+ * This patch is available to you under the terms of the GNU General Public
+ * License (GPL) Version 2
+ *
+ * Disclaimer: same as above.
+ */
 #include <linux/version.h> /* LINUX_VERSION_CODE and KERNEL_VERSION macro */
 #include <linux/module.h>
 #include <linux/init.h>
@@ -52,7 +68,7 @@
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc.h>
 
-#define	DRIVER_VERSION				"14-Mar-2012"
+#define	DRIVER_VERSION				"08-June-2012"
 
 /* CDC NCM subclass 3.2.1 */
 #define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
@@ -182,6 +198,38 @@ cdc_ncm_get_drvinfo(struct net_device *n
 	usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
 }
 
+void mirrorlink_booster(struct cdc_ncm_ctx *ctx, struct usbnet *dev)
+{
+#define NOKIA 0x0421
+#define N6x7x 0x1419
+
+	if (ctx->udev->descriptor.idVendor == NOKIA) {
+
+		switch (ctx->udev->descriptor.idProduct) {
+		case N6x7x:
+			/* The Nokia N60x,70x (productId 0x1419) needs:
+			 * - Jumbo Frame (MTU 8kB)
+			 * - Disable TX batching to improve latency
+			 */
+			if (dev != NULL)
+				/* called during NCM bind */
+				dev->hard_mtu = 8192+ETH_HLEN;
+			else {
+				/* called during NCM setup */
+				pr_info(KBUILD_MODNAME ":  jambit MirrorLink booster for "
+					"Nokia 60x,70x family");
+				ctx->tx_max_datagrams  = 1;
+				ctx->max_datagram_size = 8192+ETH_HLEN;
+			}
+			break;
+
+		default:
+			/* no other Nokia yet */
+			break;
+		}
+	}
+}
+
 static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 {
 	u32 val;
@@ -414,9 +462,10 @@ size_err:
 	}
 
 max_dgram_err:
+	mirrorlink_booster(ctx, NULL);
 	if (ctx->netdev->mtu != (ctx->max_datagram_size - ETH_HLEN))
 		ctx->netdev->mtu = ctx->max_datagram_size - ETH_HLEN;

Re: RFC: [PATCH 3/3] usb: cdc_ncm: MirrorLink booster for N60x,70x

[Bjørn Mork]

Loic Domaigne <loic.domaigne-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> writes:

> This patch allows to boost the performance if used for MirrorLink(TM). It
> is derived from a patch provided by Nokia-LC/Berlin. It optimizes the transfer
> speed for the Nokia N60x,70x. which requires special settings (jumbo frame, 
> no batching)

This could probably be reworked to a real device specific quirk if
necessary.  But it will need some refinement and justification.

> +			/* The Nokia N60x,70x (productId 0x1419) needs:
> +			 * - Jumbo Frame (MTU 8kB)
> +			 * - Disable TX batching to improve latency
> +			 */
> +			if (dev != NULL)
> +				/* called during NCM bind */
> +				dev->hard_mtu = 8192+ETH_HLEN;
> +			else {
> +				/* called during NCM setup */
> +				pr_info(KBUILD_MODNAME ":  jambit MirrorLink booster for "
> +					"Nokia 60x,70x family");
> +				ctx->tx_max_datagrams  = 1;
> +				ctx->max_datagram_size = 8192+ETH_HLEN;
> +			}

This looks really strange to me at first glance.  An important part of
the NCM protocol is the ability for the host to request these values
from the device, and the only real point of using NCM in the first place
is the bundling.  If the device has higher performance without bundling,
then why the h is it using NCM?

And if it really wants jumbo datagrams, then why doesn't it say so when
the driver ask?

Are the above values based on some perceived performance, or are there
actual measurements behind this?   Yes, sure the latency will drop with
no bundling, but that is sort of the trade-off you chose when you chose
NCM...

> @@ -1269,5 +1320,7 @@ module_exit(cdc_ncm_exit);
>  #endif
>  
>  MODULE_AUTHOR("Hans Petter Selasky");
> +MODULE_AUTHOR("Loic Domaigne");
>  MODULE_DESCRIPTION("USB CDC NCM host driver");
> +MODULE_DESCRIPTION("MirrorLink Booster by jambit GmbH");
>  MODULE_LICENSE("Dual BSD/GPL");

I do not think this is appropriate at all for a device specific bug
workaround...


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: [PATCH 3/3] usb: cdc_ncm: MirrorLink booster for N60x,70x

[Loic Domaigne]

On Sat, Mar 09, 2013 at 12:22:42PM +0100, Bjørn Mork wrote:
> Loic Domaigne <loic.domaigne@googlemail.com> writes:
> 
> > +			/* The Nokia N60x,70x (productId 0x1419) needs:
> > +			 * - Jumbo Frame (MTU 8kB)
> > +			 * - Disable TX batching to improve latency
> > +			 */
> > +			if (dev != NULL)
> > +				/* called during NCM bind */
> > +				dev->hard_mtu = 8192+ETH_HLEN;
> > +			else {
> > +				/* called during NCM setup */
> > +				pr_info(KBUILD_MODNAME ":  jambit MirrorLink booster for "
> > +					"Nokia 60x,70x family");
> > +				ctx->tx_max_datagrams  = 1;
> > +				ctx->max_datagram_size = 8192+ETH_HLEN;
> > +			}
> 
> This looks really strange to me at first glance.  An important part of
> the NCM protocol is the ability for the host to request these values
> from the device, 

Your questions are legitimate.

This has been ages since I wrote this patch. My answers might not be
absolutely accurate.

Regarding tx_max_datagrams, this could be I guess communicated by the device 
using the NTB Parameter structure / wNtbOutMaxDatagrams. Of course, Errata K 
kicks in... IIRC the device answered "no limitation". Hence I constructed NTBs 
with 2 or more datagrams and sent them to the device. The device reacted 
properly.


> and the only real point of using NCM in the first place
> is the bundling.  If the device has higher performance without bundling,
> then why the h is it using NCM?

Because MirrorLink specifies this protocol.


> And if it really wants jumbo datagrams, then why doesn't it say so when
> the driver ask?

IIRC, the driver (at that time) was only setting the MTU if the device provided
the *optional* {get|set}MaxDatagramSize capability. Which is not the case here.
So the driver assumed 1514 bytes.

I don't remember anymore what the device is passing us through the ENFD. That
something I'll need to doublecheck.

> Are the above values based on some perceived performance, or are there
> actual measurements behind this?   Yes, sure the latency will drop with
> no bundling, but that is sort of the trade-off you chose when you chose
> NCM...

Yes, we have hard numbers for MirrorLink use cases.


> > @@ -1269,5 +1320,7 @@ module_exit(cdc_ncm_exit);
> >  #endif
> >  
> >  MODULE_AUTHOR("Hans Petter Selasky");
> > +MODULE_AUTHOR("Loic Domaigne");
> >  MODULE_DESCRIPTION("USB CDC NCM host driver");
> > +MODULE_DESCRIPTION("MirrorLink Booster by jambit GmbH");
> >  MODULE_LICENSE("Dual BSD/GPL");
> 
> I do not think this is appropriate at all for a device specific bug
> workaround...

We absolutely don't cling to those lines.

Their sole purpose was to help me checking that the 'patched' driver is used.
It was planned to tweak other devices (again within the MirrorLink) context,
but I got other assignments.


Thanks Bjørn for your comments! 

Cheers,
Loic