Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754563Ab3FFGyM (ORCPT ); Thu, 6 Jun 2013 02:54:12 -0400 Received: from rhlx01.hs-esslingen.de ([129.143.116.10]:58281 "EHLO rhlx01.hs-esslingen.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754015Ab3FFGyJ (ORCPT ); Thu, 6 Jun 2013 02:54:09 -0400 Date: Thu, 6 Jun 2013 08:54:07 +0200 From: Andreas Mohr To: Ming Lei Cc: Andreas Mohr , "David S. Miller" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, OndrejZary Subject: Re: [PATCH] usbnet: improve/fix status interrupt endpoint interval Message-ID: <20130606065407.GA12910@rhlx01.hs-esslingen.de> References: <20130604182830.GA13186@rhlx01.hs-esslingen.de> <20130605163426.GA18818@rhlx01.hs-esslingen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Priority: none User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5903 Lines: 146 On Thu, Jun 06, 2013 at 09:33:28AM +0800, Ming Lei wrote: > On Thu, Jun 6, 2013 at 12:34 AM, Andreas Mohr 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/