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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 14 Mar 2013 23:15:37 +0100
From:	Loic Domaigne <loic.domaigne@...glemail.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: RFC: [PATCH 3/3] usb: cdc_ncm: MirrorLink booster for N60x,70x

On Sat, Mar 09, 2013 at 12:22:42PM +0100, Bjørn Mork wrote:
> Loic Domaigne <loic.domaigne@...glemail.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html