Am Donnerstag, 10. Juni 2010 10:49:28 schrieb Sjur BRENDELAND:
> >> +cdc_ncm_tx_timeout_start(struct cdc_ncm_softc *sc)
> >> +{
> >> + if (timer_pending(&sc->sc_tx_timer) == 0) {
> >
> > Arg. What is this supposed to test for?
>
> At every TX we start a timer unless it's already started.
>
> We need a timeout when to send out the USB frame, because we are accumulating Ethernet packets and don't know when we have got the last one in the outgoing network queue. The cdc_ncm_tx_timeout_start() function is called at every TX, but we don't reset the timer at every TX, which means we have to check timer_pending() to avoid re-starting the timer before it has elapsed.
You can use mod_timer().
> >> + /*
> >> + * The ethernet API we are using does not support transmitting
> >> + * multiple ethernet frames in a single call. This driver will
> >> + * accumulate multiple ethernet frames and send out a larger
> >> + * USB frame when the USB buffer is full, or a single jiffies
> >> + * timeout happens.
> >> + */
> >
> > So you send out a partially filled packet even if another packet is in
> > flight?
>
> Yes, we send out packets either when queue if full or at timeout.
Well, this is still less efficient than the technique the video drivers use.
The key to top performance on USB is to have three buffers.
(if you send. usbvideo gets away with two buffers because it receives.)
The HCD must not become idle. To accomplish that you use two buffers.
As the first buffer is finished, the HCD starts transfering the second buffer
and does a callback on the first buffer.
Upon that callback you must submit a new buffer (or technically its URB).
To do so the best way is to have a third buffer which you fill while two
URBs are in flight and submit as the first buffer is finished. Then you
start filling the old first buffer.
No timer and optimal performance.
> > > +static struct usb_driver cdc_ncm_driver = {
> > > + .name = "cdc_ncm",
> > > + .id_table = cdc_devs,
> > > + .probe = cdc_ncm_probe,
> > > + .disconnect = cdc_ncm_disconnect,
> > > + .suspend = cdc_ncm_suspend,
> > > + .resume = cdc_ncm_resume,
> > > + .supports_autosuspend = 1,
> >
> > Where do you do that?
>
> Are you referring to "supports_autosuspend"? Could you elaborate your question please.
You tell usbcore to use autosuspend but you include no support for detecting
idleness in the driver. As you hook into usbnet you can use the framework
usbnet provides. But if you do that you must implement the "manage_power"
callback like cdc-ether does. You do neither. So how do you expect this
to work?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
