Hi Oliver.
Oliver Neukum wrote:
>>>> +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().
Technically we could use mod_timer(). The only problem I see is that using
mod_timer() at every ethernet TX frame would significantly extend the
timeout before the actual TX, if the outgoing Ethernet packets comes every 0.8ms
while the timeout is 1.0ms. Currently 32-ethernet packets can be accumulated,
which means that with mod_timer() we could have an effective delay of 32*1.0ms.
>>> 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.
The USB-net layer which the CDC-NCM driver is using, will do the kind of buffering you are referring to. We have tried to keep our changes isolated to the CDC-NCM module.
In the future we could consider move the Ethernet packet accumulation functionality into the USB-net module, which is one layer up.
> 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?
We cannot find any references to "manage_power" under drivers/usb? And we cannot see that the cdc-ether.c driver does more than setting the "udriver->supports_autosuspend" flag. Could you please be a little bit more specific, perhaps showing us fragments of some example code that utilize USB power save on Linux?
Regards
Sjur Brændeland
Hans Petter Selasky
--
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
