netdev - Re: changing usbnet's API to better deal with cdc-ncm

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a9x33656.fsf@nemi.mork.no>
Date:	Thu, 06 Sep 2012 10:30:29 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Oliver Neukum <oneukum@...e.de>, alexey.orishko@...ricsson.com,
	netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: changing usbnet's API to better deal with cdc-ncm

Ming Lei <tom.leiming@...il.com> writes:
> On Thu, Sep 6, 2012 at 4:12 AM, Oliver Neukum <oneukum@...e.de> wrote:
>> Hi,
>>
>> looking at cdc-ncm it seeems to me that cdc-ncm is forced to play
>> very dirty games because usbnet doesn't have a notion about aggregating
>> packets for a single transfer.
>
> The Ethernet API we are using does not support transmitting multiple Ethernet
> frames in a single call, so the aggregation things should be done inside low
> level driver, in fact it is just what some wlan(802.11n) drivers have
> been doing.
>
> IMO, the current .tx_fixup is intelligent enough to handle aggregation:
>
>       - return a skb_out for sending if low level drivers think it is
> ready to send
>         the aggregated frames
>      -  return NULL if  the low level drivers think they need to wait
> for more frames
>
> Of course, the low level drivers need to start a timer to trigger sending
> remainder frames in case of timeout and no further frames come from
> upper protocol stack.
>
> Looks the introduced .tx_bundle is not necessary since .tx_fixup is OK.

The minidriver does not have any information about tx in progress.  The
strategy above, which is what cdc_ncm uses today, is fine as long as you
always want to wait as long as you always want to fill as many frames as
possible in each packet.  But what if the queue is empty and the device
is just sitting there doing nothing while you are waiting for more
frames?  Then you are just adding unnecessary latency.

There are also several usbnet minidrivers for protocols with frame
aggregation support.  Most of them do not currently implement tx
aggregation, possibly due to the complexity.  This makes a common
aggregation framework interesting in any case.  Reimplementing similar
loginc in multiple minidrivers is meaningless.

I believe Oliver is adding very useful functionality to usbnet here.
Functionality which at least cdc_ncm and possibly other existing
minidrivers can take advantage of immediately.  There also seems to be a
trend towards more complex aggregating protocols as the devices get
smarter and speeds go up.

BJørn
--
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