Re: [PATCH] usbnet: improve/fix status interrupt endpoint interval

[Andreas Mohr <andi@lisas.de>]

On Thu, Jun 06, 2013 at 09:33:28AM +0800, Ming Lei wrote:
> On Thu, Jun 6, 2013 at 12:34 AM, Andreas Mohr <andi@lisas.de> wrote:
> > Hi,
> >
> > On Wed, Jun 05, 2013 at 09:22:25AM +0800, Ming Lei wrote:
> >> On Wed, Jun 5, 2013 at 2:28 AM, Andreas Mohr <andi@lisas.de> wrote:
> >> >   Value 8 now managed to reduce powertop wakeups from ~ 540 to ~ 155
> >>
> >> It means that your device only returns current link status instead of link
> >> change. IMO, it isn't a good behaviour for the device.
> >
> > I don't quite understand that.
> 
> It is only concluded by the data you provided,  when you get ~540 wakeups,
> that means basically device will return data for each polling from HC.
> 
> Also I am wondering why you get ~540 wakeups, instead of ~360(330 + 30)
> (30 is guessed from ~155 wakup in 8ms interval)

Hmm, right, with raw interval value having been specified as 3,
this should have ended up as a cooked value of 3ms on full-speed,
thus, given no further mcs7830 wakeup activity, we should remain at 330 something.
Will investigate some more (e.g. a quick look at usbmon log timing, too).


> Did you check intr_complete() returns OK every time?

Good hint, will check.


> >> It isn't a hard demand, which only means the poll interval by which HC
> >> sends IN token to device.
> >
> > I believe this number is meant to be a hard demand by the *device*,
> > since a device is the authoritative party to know best about its
> > own servicing requirements.
> 
> Actually, just see quirks for USB devices, there are many devices which
> isn't worthy of trust, :-)

I can easily believe that, having had my more than fair share of trouble already ;)


> Also some problems should have been reported on current interval
> value(larger than interval of endpoint) if it was hard demand, but luckly
> looks no such report found.

Yep.


> As I said before, the link change is a low frequency event, so longer
> interval used by usbnet driver should be OK, right?

...minus the status flags for frames, which surely would be interesting, too :)


(and minus the frequency requirements of the mcs7830 link change
"20 times" averaging mechanism, which expects a sufficiently high rate)

> > Or, IOW, the thing that is a USB descriptor is to be seen as a *protocol*
> > where a device signals its requirements (hopefully accurately, though!).
> > And if it indicates a 1ms bInterval (which is "the requested maximum(!!)
> > number of milliseconds between transaction attempts" [lvr usbfaq]),
> 
> USB spec 2.0 doesn't say it is a maximum period between transactions,
> and only mentions that is a "desired bus access period", see "5.7.4
> Interrupt Transfer Bus Access Constraints".

Oh, so perhaps we have a usbfaq which actually is a FAQWBA? ;)
(I should really have a look at the specs directly...)


> > Is proper damage-less (overflows...) handling here a promise/guarantee
> > that's made by the USB specs?
> 
> Even there is overflow, it happens inside device, and it depends on
> implementation of device itself.

IOW, the device is free to blow up on its own.


> > Otherwise I wouldn't be so confident that a device is acting this way ;)
> 
> If so, you can use the dev->status->desc.bInterval, so you may complain
> too much wakeup and CPU power consumed, and we need leverage.

Yep, would surely be very useful to come up with a usbnet-side mechanism which
flexibly keeps wakeups at a minimum, while still retrieving all data
that it can get, and this for all devices(drivers) as handled by usbnet.


> >> For usbnet, generally speaking, the interrupt pipe is for polling the
> >> link change, which is a very low frequency event, so you don't need to
> >> worry about missing events if the interval is increased.
> >
> > Yeah, but then those status bits also contain other error info for every packet
> > processed, thus it's also very useful to achieve polling that's frequent
> > enough to properly grab info for all transferred ether frames, rather
> > than merely concentrating on link change info.
> 
> Actually, most of usbnet drivers only use interrupt pipe to retrieve link
> change(asix, smsc, ...). But if your device may provide other information
> via interrupt pipe and your driver requires that, you may keep the
> desired interval with extra power if it is worthy.
> 
> Could you let us know what device may provide ether frame info
> and how your drivers use that?

That should be all devices supported by mcs7830.c,
where it's probably the

+       MCS7830_STATUS_ETHER_FRAME_OK                   = 0x0001,
+       MCS7830_STATUS_RETRIES_MORE_THAN_16             = 0x0002,
+       MCS7830_STATUS_COLLISION_OCCURRED_AFTER_64BYTES = 0x0004,
+       MCS7830_STATUS_PACKET_ABORTED_EXCESS_DEFERRAL   = 0x0008,

+       MCS7830_STATUS_TX_STATUS_VECTOR_BITS_VALID      = 0x4000,

bits, provided for all transferred ether frames, when polled frequently enough.


> > Sure, but that handling seems a bit static. An ideal service level would be
> > getting notified of all link changes in a sufficiently slow way,
> > *and* always catching status flags of *all* frames, too.
> 
> I think we need to know how useful the status flags of all frames are first.


> > I'm still unsure how/whether to reword my documentation part, though.
> > If you happen to have some ideas about some updates,
> > that would be great.
> 
> Since the interval policy you concerned in the doc has been used so
> long and we can discuss it further, also it is another problem, not much
> related with the fix patch.
> 
> How about separating the document from the fix patch?

Sounds very useful, will do (split the lower part of the comment).

Andreas Mohr