usb: usbtest: Add new ioctl interface

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.

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.

Refactor usbtest_ioctl for readability to clarify that the only difference is in how delta time is returned.

Signed-off-by Deepa Dinamani <[email protected]>

---
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(-)

drivers/usb/misc/usbtest.c +5 -1

index 637f3f7..00daff3 100644
@@ -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;

Quoted 5 lines · 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.

Quoted 7 lines

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.

Quoted 11 lines

Refactor usbtest_ioctl for readability to clarify that the only difference is in how delta time is returned.

Signed-off-by Deepa Dinamani <[email protected]>

---
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

+++++++++++++++++++++++++++++----------------

drivers/usb/misc/usbtest.c static void complicated_callback(struct urb *urb); · line 22

Quoted 8 lines

 1 file changed, 134 insertions(+), 74 deletions(-)

drivers/usb/misc/usbtest.c +0 -0

index 637f3f7..00daff3 100644
@@ -22,7 +22,11 @@ static void complicated_callback(struct urb *urb);
 

/*-------------------------------------------------------------------------*/

Quoted 15 lines

drivers/usb/misc/usbtest.c +5 -1

 /* 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;
 };

drivers/usb/misc/usbtest.c struct usbtest_param { · line 40

Quoted 2 lines

drivers/usb/misc/usbtest.c +1 -1

-#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

drivers/usb/misc/usbtest.c usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) · line 2590

Quoted 13 lines

drivers/usb/misc/usbtest.c +12 -0

+     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.

Quoted 1 line

/*-------------------------------------------------------------------------*/

Quoted 4 lines

static unsigned force_interrupt;

drivers/usb/misc/usbtest.c +0 -0

@@ -2891,4 +2952,3 @@ module_exit(usbtest_exit);
 

drivers/usb/misc/usbtest.c module_exit(usbtest_exit); · line 2894

Quoted 4 lines

drivers/usb/misc/usbtest.c +0 -1

 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

Quoted 4 lines · Arnd Bergmann

Quoted 3 lines · On Wednesday 21 October 2015 15:17:14 Deepa Dinamani wrote:

Rename old ioctl and data types to COMPAT_USBTEST_REQUEST and usbtest_param_compat, respectively.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 19 lines

drivers/usb/misc/usbtest.c usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) · line 2590

Quoted 12 lines

drivers/usb/misc/usbtest.c +12 -0

+     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.

Quoted 5 lines

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.

Quoted 6 lines · On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · On Wednesday 21 October 2015 15:17:14 Deepa Dinamani wrote:

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

This ensures that the time duration is always positive. Also use

ktime

Quoted 1 line

Quoted 1 line

Quoted 1 line

api's for time routines accomodating moving all of kernel to use 64

bit

Quoted 11 lines

Quoted 6 lines

Quoted 1 line

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.

It's a little more complicated: We try to avoid breaking compilation of user space programs if at all possible, so if they use timeval today, it's better not to require an API change as far as user space is concerned.

On the other hand, we know that we have to do significant changes to glibc, and any interfaces that are between the kernel and libc (i.e. most system calls) should stop passing timeval. Instead the libc can provide its own helper functions and e.g. implement gettimeofday() by calling clock_gettime() and then converting the result.

Quoted 1 line

Quoted 1 line

The most important point to notice here is that the definition is in a

.c

Quoted 12 lines

Quoted 5 lines

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.

Quoted 5 lines

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

Quoted 2 lines

the above reason.
I kept them intact for compatibility with existing binaries.

Yes, and that is the right idea. I suspect we will have to provide different definitions for kernel and user space though, in one form or another. You are right that we want to remove 'timeval' from the kernel, and in order to keep user space unchanged, this means defining the structure like

struct usbtest_param_32 {
        /* inputs */
        __u32                test_num;       /* 0..(TEST_CASES-1) */
        __u32                iterations;
        ...
        __s32                 duration_sec;
        __s32                 duration_usec;
};

which is a well-defined binary version of what 32-bit user space sees today. We also need the 64-bit version of that, for both normal 64-bit tasks and future 32-bit tasks that are built with the old structure definition.

Optionally, we can introduce a better interface with a new command number as your patch does, but that should be a separate patch, and we have to see if the USB maintainers want to do this or not.

Quoted 26 lines

Quoted 20 lines

drivers/usb/misc/usbtest.c usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) · line 2590

Quoted 13 lines

drivers/usb/misc/usbtest.c +12 -0

+     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.

Quoted 5 lines

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.

Quoted 7 lines

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.

My plan is a bit different: I want to remove all references to timeval from the kernel, except for the bits we need for backwards compatibility.

In this driver, we are actually lucky that the structure has never been visible to user space before, because that means nothing expects to find the timeval mentioned in a particular kernel header, and we can change it as I suggested above without breaking compilation, because user space still sees its own definition of the same struct.

Quoted 1 line

On future 32-bit kernels however, timeval and compat_timeval would have

the

Quoted 4 lines

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.

Good thinking, but this can be avoided by having two fixed-size structure definitions that are known to be different.

Quoted 2 lines

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

Quoted 1 line

a single interface which works for both 32/ 64 bit versions?

Ah, I wasn't aware that the USB framework didn't have that. You are exactly right that in general it's best to define ioctl structures to be compatible for both, but the problem in this driver is specifically that it didn't do that.

Quoted 2 lines

USB framework's higher level ioctl framework already supports a .compat_ioctl and pointers are fixed before coming into this usbtest

driver.

Quoted 1 line

Are you suggesting .compat_ioctl extension to the usb_driver, or

rewriting

Quoted 1 line

the driver as a cdev?

Once the handler is changed to handle both versions of the structure, we don't need a separate .compat_ioctl any more, we just need to make sure that the handler gets called for both. I haven't checked this but according to your description I expect that is what happens already/

Quoted 2 lines

The other ioctls take care of individual compat data types by internally using #ifdef CONFIG_COMPAT.

Ok. In my system call patch series, I introduce a new CONFIG_COMPAT_TIME symbol that specifically deals with compatibility mode for 32-bit time_t on both 32-bit and 64-bit architectures. This is the #ifdef we should be using here as well in principle. My patches however are not merged yet, but there is no harm in leaving that code active here, as both versions
provide a correct API and do not overflow in 2038.

Arnd

Quoted 17 lines · Arnd Bergmann

Quoted 1 line · On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:

On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann <[email protected]> wrote:

Yes, and that is the right idea. I suspect we will have to provide different definitions for kernel and user space though, in one form or another. You are right that we want to remove 'timeval' from the kernel, and in order to keep user space unchanged, this means defining the structure like

struct usbtest_param_32 {
        /* inputs */
        __u32                test_num;       /* 0..(TEST_CASES-1) */
        __u32                iterations;
        ...
        __s32                 duration_sec;
        __s32                 duration_usec;
};

Quoted 9 lines

which is a well-defined binary version of what 32-bit user space sees today. We also need the 64-bit version of that, for both normal 64-bit tasks and future 32-bit tasks that are built with the old structure definition.

Optionally, we can introduce a better interface with a new command number as your patch does, but that should be a separate patch, and we have to see if the USB maintainers want to do this or not.

There are two problems with the original ioctl interface:

1. Because of the use of timeval, compatibility is broken between 32 bit and 64 bit binaries.

This has nothing to do with y2038 problem at all.
This is the case with all interfaces using timeval itself and has nothing to do with this one
particular bad interface design.

The struct you suggested above will work to map to two separate ioctls. But, if this is a generic problem, shouldn't the above solution be in some common file?
For instance we could have this:

struct timeval_32 {
        __s32                 duration_sec;
        __s32                 duration_usec;
};

And, a similar struct for timeval_64.

This would also mean adding api's to fill the structures defined above. Basically an entire layer.

This is not necessary for this driver as the struct's are not exposed. My guess is also that there aren't many applications using this because of the way it needs redeclaring everything in the application.

Since the original implementation is broken already, my first preference was to fix the interface with the new interface itself.

My intention was to not fix the broken interface, but to replace or provide a new interface.

Wouldn't the following be better?

#ifdef CONFIG_COMPAT
old ioctl(code = 100)
use old timeval struct
#if CONFIG_COMPAT_TIME
use compat_timeval instead of timeval

#else
          new ioctl(code = 101)

#endif

The old ioctl support to be eventually removed completely.

2. The y2038 problem in the driver is quite straight forward as to picking the right calls to fill in whatever data types we choose above.

Quoted 15 lines

Quoted 2 lines

USB framework's higher level ioctl framework already supports a .compat_ioctl and pointers are fixed before coming into this usbtest

driver.

Quoted 1 line

Are you suggesting .compat_ioctl extension to the usb_driver, or

rewriting

Quoted 1 line

the driver as a cdev?

Once the handler is changed to handle both versions of the structure, we don't need a separate .compat_ioctl any more, we just need to make sure that the handler gets called for both. I haven't checked this but according to your description I expect that is what happens already/

Quoted 2 lines

The other ioctls take care of individual compat data types by internally using #ifdef CONFIG_COMPAT.

Quoted 9 lines

Ok. In my system call patch series, I introduce a new CONFIG_COMPAT_TIME symbol that specifically deals with compatibility mode for 32-bit time_t on both 32-bit and 64-bit architectures. This is the #ifdef we should be using here as well in principle. My patches however are not merged yet, but there is no harm in leaving that code active here, as both versions
provide a correct API and do not overflow in 2038.

Does it help to review a patch of this kind with a design document? Does the kernel review system allow something like this? Some of the questions that popped up are the ones I asked myself before. It might let the thought process be more evident.

Quoted 3 lines · On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:

Quoted 2 lines · Arnd Bergmann

Quoted 1 line · On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:

On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann <[email protected]>

Quoted 27 lines · wrote:

Quoted 15 lines

Yes, and that is the right idea. I suspect we will have to provide different definitions for kernel and user space though, in one form or another. You are right that we want to remove 'timeval' from the kernel, and in order to keep user space unchanged, this means defining the structure like

struct usbtest_param_32 {
        /* inputs */
        __u32                test_num;       /* 0..(TEST_CASES-1) */
        __u32                iterations;
        ...
        __s32                 duration_sec;
        __s32                 duration_usec;
};

Quoted 9 lines

which is a well-defined binary version of what 32-bit user space sees today. We also need the 64-bit version of that, for both normal 64-bit tasks and future 32-bit tasks that are built with the old structure definition.

Optionally, we can introduce a better interface with a new command number as your patch does, but that should be a separate patch, and we have to see if the USB maintainers want to do this or not.

There are two problems with the original ioctl interface:

Quoted 7 lines

1. Because of the use of timeval, compatibility is broken between 32 bit and 64 bit binaries.

This has nothing to do with y2038 problem at all.
This is the case with all interfaces using timeval itself and has nothing to do with this one
particular bad interface design.

Right.

Quoted 2 lines

The struct you suggested above will work to map to two separate ioctls. But, if this is a generic problem, shouldn't the above solution be in

some

Quoted 7 lines

common file?
For instance we could have this:

struct timeval_32 {
        __s32                 duration_sec;
        __s32                 duration_usec;
};

This is essentially 'struct compat_timeval', which I intend to keep around in the kernel for backwards compatibility and will make available to 32-bit kernels that currently can't use it. The patches still need a few more review rounds though.

Quoted 4 lines

And, a similar struct for timeval_64.

This would also mean adding api's to fill the structures defined above. Basically an entire layer.

We should really see how many drivers need this. I have shown that the core kernel does not need it with my patches, as all system calls that use timeval are already deprecated. I have a list of drivers that need to be converted at

https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_0YiQwis/edit#gid=346406462

on the second sheet. All the ones that have an ABI exposed to user space are marked 'uapi' or 'ioctl', and we could check which ones of these would be helped by having a generic set of helpers for timeval_64.

Quoted 3 lines

This is not necessary for this driver as the struct's are not exposed. My guess is also that there aren't many applications using this because

of

Quoted 1 line

the way it needs redeclaring everything in the application.

Agreed. My guess is that there is only one application using it at all, but it's hard to know for sure. If we could prove that there is only one, and we can change it to use a new ioctl command if that is present in some header file, all this could be simplified.

Quoted 3 lines

Since the original implementation is broken already, my first preference was to fix the interface with the new interface itself.

I wouldn't call the original implementation broken, except for the 64-bit compat problem. What makes it broken is that the ioctl data structure changes in user space when that changes the defintion of 'struct timeval', but since the data returned here is a difference of two times, the current 32-bit tv_sec variable is actually good enough.

Quoted 1 line

My intention was to not fix the broken interface, but to replace or

provide

Quoted 4 lines

a new interface.

Wouldn't the following be better?

Quoted 8 lines

#ifdef CONFIG_COMPAT
old ioctl(code = 100)
use old timeval struct
#if CONFIG_COMPAT_TIME
use compat_timeval instead of timeval

#else
          new ioctl(code = 101)

#endif

Sorry, I'm not really following here. Doesn't this break existing 32-bit users that don't even care about the y2038 issue?

Quoted 1 line

The old ioctl support to be eventually removed completely.

We should only ever plan to break existing user space if there is no other option at all. Remember that most users don't care about the y2038 problem because they are on 64-bit systems that have a couple of bugs we need to fix in the next few years, or because they are on 32-bit systems that they will stop using before it becomes a problem. The implied rules that result from this are:

- nothing we do should require changes for 64-bit tasks

- all 32-bit tasks should keep working on future kernels (both 32-bit and 64-bit) unless they explicitly disable CONFIG_COMPAT_TIME

- CONFIG_COMPAT_TIME and all code under it will be removed in 2038 at the latest, but perhaps not much earlier.

- if possible, source code that produces a working 32-bit binary today should not need modifications to work when compiled against a libc with 64-bit time_t and run on a kernel without CONFIG_COMPAT_TIME.

Quoted 1 line

2. The y2038 problem in the driver is quite straight forward as to

picking

Quoted 1 line

the right calls to fill in whatever data types we choose above.

I would argue that it's exactly the same problem for 32-bit and 64-bit kernels in this driver: you can have user space binaries with different definitions of timeval and we want both of them to work.

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

USB framework's higher level ioctl framework already supports a .compat_ioctl and pointers are fixed before coming into this usbtest

driver.

Quoted 1 line

Are you suggesting .compat_ioctl extension to the usb_driver, or

rewriting

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

the driver as a cdev?

Once the handler is changed to handle both versions of the structure, we don't need a separate .compat_ioctl any more, we just need to make sure that the handler gets called for both. I haven't checked this but according to your description I expect that is what happens already/

Quoted 1 line

The other ioctls take care of individual compat data types by

internally

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

using #ifdef CONFIG_COMPAT.

Ok. In my system call patch series, I introduce a new

CONFIG_COMPAT_TIME

Quoted 1 line

Quoted 1 line

symbol that specifically deals with compatibility mode for 32-bit

time_t

Quoted 7 lines

Quoted 7 lines

on both 32-bit and 64-bit architectures. This is the #ifdef we should be using here as well in principle. My patches however are not merged yet, but there is no harm in leaving that code active here, as both versions
provide a correct API and do not overflow in 2038.

Quoted 4 lines

Does it help to review a patch of this kind with a design document? Does the kernel review system allow something like this? Some of the questions that popped up are the ones I asked myself before. It might let the thought process be more evident.

Most kernel developers work better with patches than with design documents.

Arnd

Quoted 4 lines

Quoted 3 lines

Since the original implementation is broken already, my first preference was to fix the interface with the new interface itself.

Quoted 8 lines

I wouldn't call the original implementation broken, except for the 64-bit compat problem. What makes it broken is that the ioctl data structure changes in user space when that changes the defintion of 'struct timeval', but since the data returned here is a difference of two times, the current 32-bit tv_sec variable is actually good enough.

I don't think seconds granularity is sufficient here for usb operations. This page shows some tests which run for less than a second:
http://blackfin.uclinux.org/doku.php?id=linux-kernel:usb-gadget:zero

Quoted 15 lines

Quoted 1 line

My intention was to not fix the broken interface, but to replace or

provide

Quoted 4 lines

a new interface.

Wouldn't the following be better?

Quoted 8 lines

#ifdef CONFIG_COMPAT
old ioctl(code = 100)
use old timeval struct
#if CONFIG_COMPAT_TIME
use compat_timeval instead of timeval

#else
          new ioctl(code = 101)

#endif

Quoted 3 lines

Sorry, I'm not really following here. Doesn't this break existing 32-bit users that don't even care about the y2038 issue?

Maybe I misunderstood what CONFIG_COMPAT_TIME was doing. What I meant was that we should keep the old interface only for older binaries.
All new code should only use new interface.
That was the eventual goal.

#ifdef CONFIG_COMPAT_TIME
old ioctl(code = 100)
support 64 bit and 32 bit timeval

#endif
         new ioctl(code = 101)

By the time all support for 32-bit time is removed, use of the old interface should be removed.

Timeval was a bad struct to expose to userspace anyway.

In this regard, I was going to fix up the usbtest tool to use new interface also.

Quoted 16 lines

Quoted 1 line

Quoted 1 line

Ok. In my system call patch series, I introduce a new

CONFIG_COMPAT_TIME

Quoted 1 line

Quoted 1 line

symbol that specifically deals with compatibility mode for 32-bit

time_t

Quoted 11 lines

Quoted 7 lines

on both 32-bit and 64-bit architectures. This is the #ifdef we should be using here as well in principle. My patches however are not merged yet, but there is no harm in leaving that code active here, as both versions
provide a correct API and do not overflow in 2038.

Does it help to review a patch of this kind with a design document? Does the kernel review system allow something like this? Some of the questions that popped up are the ones I asked myself before. It might let the thought process be more evident.

Quoted 2 lines

Most kernel developers work better with patches than with design documents.

I was saying patches only communicate the final product. For instance, I did consider using .compat_ioctl, timespec64 etc. I cannot really tell the story as to why this is the best way to do it unless someone asks.

Summarizing the next steps:

1. Figure out how many ioctl or uapi need 64-bit timeval.

2. Should compat fix for existing usbtest ioctl be based on top of your COMPAT_TIME changes?

3. Contact usb list about usbtest ioctl uses and about proposed new ioctl.

Quoted 1 line

3. Contact usb list about usbtest ioctl uses and about proposed new ioctl.

Yes, but don't make this one a priority. Adding a new ioctl is just the icing on the cake, and that patch mainly serves to highlight the fact that the original API was implemented poorly and how it should have been done instead. It's more important for us to avoid making things worse with the 64-bit time_t change.

Quoted 1 line · On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:

Quoted 1 line

3. Contact usb list about usbtest ioctl uses and about proposed new

ioctl.

Quoted 7 lines

Yes, but don't make this one a priority. Adding a new ioctl is just the icing on the cake, and that patch mainly serves to highlight the fact that the original API was implemented poorly and how it should have been done instead. It's more important for us to avoid making things worse with the 64-bit time_t change.

I've given this a little more thought now: I think we should support both variations of the current interface in both the kernel and in the official copy of the user space tool, for maximum compatibility.

If we add another ioctl, that means we have to support all three variants in both kernel and user space for the foreseeable future, that's only worth it if we can add some real value in the new interface. Instead of a new ioctl, we could also choose to add a debugfs interface that might be easier to use. Still, that is only a mild improvement to a rarely used driver and a lot of work, so don't spend too much time on that one.

Arnd

Quoted 11 lines · Arnd Bergmann

Quoted 1 line · On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:

Quoted 1 line

3. Contact usb list about usbtest ioctl uses and about proposed new

ioctl.

Quoted 7 lines

Yes, but don't make this one a priority. Adding a new ioctl is just the icing on the cake, and that patch mainly serves to highlight the fact that the original API was implemented poorly and how it should have been done instead. It's more important for us to avoid making things worse with the 64-bit time_t change.

Quoted 4 lines

I've given this a little more thought now: I think we should support both variations of the current interface in both the kernel and in the official copy of the user space tool, for maximum compatibility.

If timeval remains as is, I don't see how user space needs to change. Agree that kernel module can support both so that either binary would work.

Quoted 7 lines

If we add another ioctl, that means we have to support all three variants in both kernel and user space for the foreseeable future, that's only worth it if we can add some real value in the new interface. Instead of a new ioctl, we could also choose to add a debugfs interface that might be easier to use. Still, that is only a mild improvement to a rarely used driver and a lot of work, so don't spend too much time on that one.

Ok. will let this one be for now.

Quoted 3 lines

Arnd

Quoted 2 lines

I looked at the spreadsheet(sheet 3). Nothing else actually needs timeval being exposed to userspace.
Either whatever uses timeval, is only using it internally or uses time_t to user.

I submitted a v2 of the patch that doesn't require a new timeval struct accordingly.