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

["Bjørn Mork" <bjorn@mork.no>]

Oliver Neukum <oneukum@suse.de> writes:

> 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.
>
> It seems to me that this can be fixed introducing a method for bundling,
> which tells usbnet how packets have been aggregated. To have performance
> usbnet strives to always keep at least two transfers in flight.
>
> The code isn't complete and I need to get a device for testing, but to get
> your opinion, I ask you to comment on what I have now.

I haven't tested this on any device either, but it looks like the right
direction to me.  Note that there are several other existing minidrivers
which could take advantage of this code. A quick look found that both
sierra_net and rndis_host have some unbundling support in their
rx_fixups, but both ignore tx bundling. rndis_host has the following
explanation in tx_fixup:

        /* fill out the RNDIS header.  we won't bother trying to batch
         * packets; Linux minimizes wasted bandwidth through tx queues.
         */

Although that may be true for the host, I am not sure it will be true
for every device?


> @@ -874,71 +840,37 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
>  	/* return skb */
>  	ctx->tx_curr_skb = NULL;
>  	ctx->netdev->stats.tx_packets += ctx->tx_curr_frame_num;


Maybe all the statistics could be moved back to usbnet now that it will
see all packets again?


> index f87cf62..bb2f622 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -33,6 +33,7 @@ struct usbnet {
>  	wait_queue_head_t	*wait;
>  	struct mutex		phy_mutex;
>  	unsigned char		suspend_count;
> +	atomic_t		tx_in_flight;
>  
>  	/* i/o info: pipes etc */
>  	unsigned		in, out;


So you don't keep any timer here?  The idea is that you always will
transmit immediately unless there are two transmit's in flight?  I
believe that is a change which has to be tested against real devices.
They may be optimized for receiving bundles.


Not really related, but I am still worrying how MBIM is going to fit
into the usbnet model where you have a strict relation between one
netdev and one USB interface.  Do you see any way usbnet could be
extended to manage a list of netdevs?

All the netdev related functions doing

	struct usbnet		*dev = netdev_priv(net);

would still work.  But all the USB related functions using dev->net to
get _the_ netdev would fail.  Maybe this will be too difficult to
implement within the usbnet framework at all?


Bjørn