Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759808AbbEEQJx (ORCPT ); Tue, 5 May 2015 12:09:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58459 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2993360AbbEEPTv (ORCPT ); Tue, 5 May 2015 11:19:51 -0400 Date: Tue, 5 May 2015 09:19:37 -0600 From: Pete Zaitcev To: Arnd Bergmann Cc: Tina Ruchandani , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, zaitcev@redhat.com Subject: Re: [PATCH] USB: usbmon: Remove timeval usage for timestamp Message-ID: <20150505091937.58beda9f@guren.zaitcev.lan> In-Reply-To: <4936805.OkgPExQ11v@wuerfel> References: <20150505061433.GA4690@tinar> <4936805.OkgPExQ11v@wuerfel> Organization: Red Hat, Inc. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2416 Lines: 62 On Tue, 05 May 2015 11:24:16 +0200 Arnd Bergmann wrote: > On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote: > > static inline unsigned int mon_get_timestamp(void) > > { > > - struct timeval tval; > > + struct timespec64 now; > > unsigned int stamp; > > > > - do_gettimeofday(&tval); > > - stamp = tval.tv_sec & 0xFFF; /* 2^32 = 4294967296. Limit to 4096s. */ > > - stamp = stamp * 1000000 + tval.tv_usec; > > + getnstimeofday64(&now); > > + stamp = now.tv_sec & 0xFFF; /* now.tv_sec is 64-bit. Limit to 4096s */ > > + stamp = stamp * USEC_PER_SEC + now.tv_nsec / NSEC_PER_USEC; > > return stamp; > > } > > Your conversion looks entirely correct, but the original code is a bit > odd here as it does not use the entire range of the 32-bit microsecond > value, and counts from 0 to 4096000000us instead of the more intuitive > 0 to 4294967296 us range before wrapping around. The intent was to create a rolling timestamp that is not too large. Remember that the text format is intended to be eyeballed. The wrap point could be anything. No consideration was given to what's intuitive. > it might be more obvious what is going on, but it would slightly change > the output in the debugfs file to use the full range. Do we know what > behavior is expected by normal user space here? > [...] > I also wonder if we should make the output use monotonic time instead The only guarantee we give is that the time corresponds to microseconds. This is sometimes necessary to debug things in microntrollers in USB devices. A monotonic time is fine. One thing though, I object to Tina's new comment. It does not matter what now.tv_sec is. The comment is about the destination, that is the stamp. So, please don't change it, unless you change the type of the ep->tstamp. > static inline unsigned int mon_get_timestamp(void) > { > return ktime_to_us(ktime_get_real()); > } > > it might be more obvious what is going on, but it would slightly change Now you made the truncation explicit, so if anyhing it's less obvious. The code is fine, but at least add a comment to that effect, if you don't want to tack %4096000000 or %4000000000. -- Pete -- 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/