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: <87ehmf36qd.fsf@nemi.mork.no>
Date:	Thu, 06 Sep 2012 10:17:46 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	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

Oliver Neukum <oneukum@...e.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
--
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