On Wednesday 21 October 2015 15:17:14 Deepa Dinamani wrote:
The new USBTEST_REQUEST ioctl is intended to eventually replace the old timeval exposing interface. struct timeval can have different sizes based on whether the kernel/ userspace are compiled in 32 bit/ 64 bit mode. The new ioctl exposes the duration the tests run as a scalar nanosecond value.
Hi Deepa,
This is an interesting approach, but you are going in a slightly wrong direction. The new ioctl definition is indeed safe for y2038 and gets it all right, but it is not compatible at source level, so existing user space programs will require source changes in order to use it. I think this can be avoided, and we also don't really need the new command to solve the y2038 problem.
Rename old ioctl and data types to COMPAT_USBTEST_REQUEST and usbtest_param_compat, respectively.
The change also uses monotonic time instead of real time for both ioctls. This ensures that the time duration is always positive. Also use ktime api's for time routines accomodating moving all of kernel to use 64 bit time eventually.
Changing to monotonic time is good here. Using ktime_t is often a good idea, but I think in this case it causes more problems because we end up having to convert it back into timeval.
Refactor usbtest_ioctl for readability to clarify that the only difference is in how delta time is returned.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Resending due to incorrect recipient list
This patch is intended for the next branch of the usb tree: git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
drivers/usb/misc/usbtest.c | 208 +++++++++++++++++++++++++++++---------------- 1 file changed, 134 insertions(+), 74 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c index 637f3f7..00daff3 100644 --- a/drivers/usb/misc/usbtest.c +++ b/drivers/usb/misc/usbtest.c @@ -22,7 +22,11 @@ static void complicated_callback(struct urb *urb); /*-------------------------------------------------------------------------*/ /* FIXME make these public somewhere; usbdevfs.h? */ -struct usbtest_param {
+/* Compat parameter for the compat interface.
- This is deprecated due to the timeval output parameter.
- */
+struct usbtest_param_compat { /* inputs */ unsigned test_num; /* 0..(TEST_CASES-1) */ unsigned iterations; @@ -33,7 +37,27 @@ struct usbtest_param { /* outputs */ struct timeval duration; }; -#define USBTEST_REQUEST _IOWR('U', 100, struct usbtest_param)
The most important point to notice here is that the definition is in a .c file, where user space cannot access it. This means that any program calling it already has a copy of this definition, and changing the definition here will not help you the way it would if the definition was in include/uapi/linux/*.h
- switch (code) {
- case USBTEST_REQUEST_COMPAT:
temp.test_num = param_compat->test_num;temp.iterations = param_compat->iterations;temp.length = param_compat->length;temp.sglen = param_compat->sglen;temp.vary = param_compat->vary;param = &temp;break;- case USBTEST_REQUEST:
break;
This part looks ok, you just need to adapt it to using the right structures. Basically what we want to do here is to handle both variants of 'struct timeval' that user space might see. Based on the _IOWR() macro definition, they give us two different command codes that you can test for in the same way here.
It also makes sense to add support for the .compat_ioctl at the same time, because 64-bit kernels with 32-bit user space have the exact same problem in this driver that future 32-bit kernels will have with new vs old user space.
/*-------------------------------------------------------------------------*/ static unsigned force_interrupt; @@ -2891,4 +2952,3 @@ module_exit(usbtest_exit); MODULE_DESCRIPTION("USB Core/HCD Testing Driver"); MODULE_LICENSE("GPL");
These two changes should not be here, as they are unrelated to what you are doing.
Arnd