On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann arnd@arndb.de wrote:
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.
I was trying to address the problem that the old interface was not right to begin with. And, thought maybe replacing the whole interface would be a good idea. I explain more below.
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.
If we are keeping timeval in the interface, then I agree we don't need ktime_t. It was my understanding from the y2038 summary that you were recommending eliminating 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
This was going to be in a follow on patch. But, I was not sure if it should be done as part of outreachy. The driver in the current state is self contained in this .c file. All definitions should really be moved to a uapi header file as the original fixme comment indicates.
But the only current user program that seems to be using it is in the kernel tools. The structures and the ioctl are indeed redefined there.
Also, the previous struct and IOCTL definitions have not changed because of the above reason. I kept them intact for compatibility with existing binaries.
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.
So, you are suggesting using compat_timeval and timeval structs for the IOCTL's and hence maintaining the same ioctl code but use the difference in sizes to get different values. My understanding was that we don't want to expose any new timeval interfaces to the user. This is the reason I was adding the new interface.
On future 32-bit kernels however, timeval and compat_timeval would have the same size. This seems a concern at the offset. Unless we define the compat ioctl under #ifdef this will not work because of same size values for both ioctls.
USB framework does not support .compat_ioctl in the struct usb_driver. Isn't the general guideline to stay away from the .compat ioclt and provide a single interface which works for both 32/ 64 bit versions?
USB framework's higher level ioctl framework already supports a .compat_ioctl and pointers are fixed before coming into this usbtest driver. Are you suggesting .compat_ioctl extension to the usb_driver, or rewriting the driver as a cdev?
The other ioctls take care of individual compat data types by internally using #ifdef CONFIG_COMPAT.
/*-------------------------------------------------------------------------*/
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.
These whitespace changes got added by mistake. Will remove these.
Arnd
- Deepa