Received-SPF: neutral (google.com: 212.227.126.130 is neither permitted nor denied by best guess record for domain of arnd@arndb.de) client-ip=212.227.126.130;
From: Arnd Bergmann <arnd@arndb.de>
To: outreachy-kernel@googlegroups.com
Cc: Deepa Dinamani <deepa.kernel@gmail.com>, y2038@lists.linaro.org
Subject: Re: [Outreachy kernel] [PATCH v2] usb: misc: usbtest: Remove timeval usage
Date: Wed, 04 Nov 2015 23:02:22 +0100
Message-ID: <3914202.EJCPxKyaSg@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <1445908614-20560-1-git-send-email-deepa.kernel@gmail.com>
References: <1445908614-20560-1-git-send-email-deepa.kernel@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

On Monday 26 October 2015 18:16:54 Deepa Dinamani wrote:
> timeval is deprecated and not y2038 safe.  Its size also changes according
> to 32 bit/ 64 bit compilation.  Replace it with 32 and 64 bit versions of
> its individual fields, giving two ioctls with different code values.
> The two ioctls are necessary to maintain the 32 bit and 64 bit userspace
> compatibility with a 64/32 bit kernel.
> 
> Change unsigned to __u32 types for a definitive userspace interface.
> This is in accordance with the psABI that the unsigned type is always
> 32 bits.
> 
> Also use motonic timer instead of real time to ensure positive delta
> values.
> 
> Refactor usbtest_ioctl for readability to isolate the handling of the
> testing timing measurement.
> 
> The official testusb userspace tool can be changed in a separate patch
> to reflect the __u32 changes as well. It can use the usbtest_param_32
> struct, since 32 bit seconds is long enough for test durations.
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>

Sorry for taking so long to reply. This looks really good, great work!

I have just one small comment, please address this and then send the
patch to the respective maintainers. Unfortunately, we are in a merge
window now, so it might not get picked up right away.

> +	getrawmonotonic64(&start);
> +
> +	retval = usbtest_do_ioctl(intf, param_32);
> +	if (retval)
> +		goto free_mutex;
> +
> +	getrawmonotonic64(&end);
> +
> +	duration = timespec64_sub(end, start);
> +
> +	temp.duration_sec = duration.tv_sec;
> +	temp.duration_usec = duration.tv_nsec/1000;

I would use ktime_get_ts64() instead of getrawmonotonic64(): We normally
don't want the 'raw' time in drivers but the normal monotonic time
that ktime_get_* uses.

One more small style comment: I would use NSECS_PER_USEC instead of
the open-coded '1000'.

	Arnd