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 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) + +/* Parameters to the usbtest driver */ +struct usbtest_param { + /* inputs */ + unsigned test_num; /* 0..(TEST_CASES-1) */ + unsigned iterations; + unsigned length; + unsigned vary; + unsigned sglen; + + /* outputs */ + s64 duration_ns; +}; + +/* + * COMPAT IOCTL interface to the driver. + * The interface is deprecated and should not be used by new code. + */ +#define USBTEST_REQUEST_COMPAT _IOWR('U', 100, struct usbtest_param_compat) +/* IOCTL interface to the driver. */ +#define USBTEST_REQUEST _IOWR('U', 101, struct usbtest_param)
/*-------------------------------------------------------------------------*/
@@ -2049,81 +2073,20 @@ static int test_unaligned_bulk( return retval; }
-/*-------------------------------------------------------------------------*/ - -/* We only have this one interface to user space, through usbfs. - * User mode code can scan usbfs to find N different devices (maybe on - * different busses) to use when testing, and allocate one thread per - * test. So discovery is simplified, and we have no device naming issues. - * - * Don't use these only as stress/load tests. Use them along with with - * other USB bus activity: plugging, unplugging, mousing, mp3 playback, - * video capture, and so on. Run different tests at different times, in - * different sequences. Nothing here should interact with other devices, - * except indirectly by consuming USB bandwidth and CPU resources for test - * threads and request completion. But the only way to know that for sure - * is to test when HC queues are in use by many devices. - * - * WARNING: Because usbfs grabs udev->dev.sem before calling this ioctl(), - * it locks out usbcore in certain code paths. Notably, if you disconnect - * the device-under-test, hub_wq will wait block forever waiting for the - * ioctl to complete ... so that usb_disconnect() can abort the pending - * urbs and then call usbtest_disconnect(). To abort a test, you're best - * off just killing the userspace task and waiting for it to exit. - */ - +/* Run tests. */ static int -usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) +usbtest_do_ioctl(struct usb_interface *intf, struct usbtest_param *param) { struct usbtest_dev *dev = usb_get_intfdata(intf); struct usb_device *udev = testdev_to_usbdev(dev); - struct usbtest_param *param = buf; - int retval = -EOPNOTSUPP; struct urb *urb; struct scatterlist *sg; struct usb_sg_request req; - struct timeval start; unsigned i; - - /* FIXME USBDEVFS_CONNECTINFO doesn't say how fast the device is. */ - - pattern = mod_pattern; - - if (code != USBTEST_REQUEST) - return -EOPNOTSUPP; + int retval = -EOPNOTSUPP;
if (param->iterations <= 0) return -EINVAL; - - if (param->sglen > MAX_SGLEN) - return -EINVAL; - - if (mutex_lock_interruptible(&dev->lock)) - return -ERESTARTSYS; - - /* FIXME: What if a system sleep starts while a test is running? */ - - /* some devices, like ez-usb default devices, need a non-default - * altsetting to have any active endpoints. some tests change - * altsettings; force a default so most tests don't need to check. - */ - if (dev->info->alt >= 0) { - int res; - - if (intf->altsetting->desc.bInterfaceNumber) { - mutex_unlock(&dev->lock); - return -ENODEV; - } - res = set_altsetting(dev, dev->info->alt); - if (res) { - dev_err(&intf->dev, - "set altsetting to %d failed, %d\n", - dev->info->alt, res); - mutex_unlock(&dev->lock); - return res; - } - } - /* * Just a bunch of test cases that every HCD is expected to handle. * @@ -2133,7 +2096,6 @@ usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) * FIXME add more tests! cancel requests, verify the data, control * queueing, concurrent read+write threads, and so on. */ - do_gettimeofday(&start); switch (param->test_num) {
case 0: @@ -2548,17 +2510,116 @@ usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) dev->in_pipe, NULL, 0); break; } - do_gettimeofday(¶m->duration); - param->duration.tv_sec -= start.tv_sec; - param->duration.tv_usec -= start.tv_usec; - if (param->duration.tv_usec < 0) { - param->duration.tv_usec += 1000 * 1000; - param->duration.tv_sec -= 1; + return retval; +} + +/*-------------------------------------------------------------------------*/ + +/* We only have this one interface to user space, through usbfs. + * User mode code can scan usbfs to find N different devices (maybe on + * different busses) to use when testing, and allocate one thread per + * test. So discovery is simplified, and we have no device naming issues. + * + * Don't use these only as stress/load tests. Use them along with with + * other USB bus activity: plugging, unplugging, mousing, mp3 playback, + * video capture, and so on. Run different tests at different times, in + * different sequences. Nothing here should interact with other devices, + * except indirectly by consuming USB bandwidth and CPU resources for test + * threads and request completion. But the only way to know that for sure + * is to test when HC queues are in use by many devices. + * + * WARNING: Because usbfs grabs udev->dev.sem before calling this ioctl(), + * it locks out usbcore in certain code paths. Notably, if you disconnect + * the device-under-test, hub_wq will wait block forever waiting for the + * ioctl to complete ... so that usb_disconnect() can abort the pending + * urbs and then call usbtest_disconnect(). To abort a test, you're best + * off just killing the userspace task and waiting for it to exit. + */ + +static int +usbtest_ioctl(struct usb_interface *intf, unsigned int code, void *buf) +{ + + struct usbtest_dev *dev = usb_get_intfdata(intf); + struct usbtest_param_compat *param_compat = buf; + struct usbtest_param temp; + struct usbtest_param *param = buf; + ktime_t start; + ktime_t end; + ktime_t duration; + int retval = -EOPNOTSUPP; + + /* FIXME USBDEVFS_CONNECTINFO doesn't say how fast the device is. */ + + pattern = mod_pattern; + + if (mutex_lock_interruptible(&dev->lock)) + return -ERESTARTSYS; + + /* FIXME: What if a system sleep starts while a test is running? */ + + /* some devices, like ez-usb default devices, need a non-default + * altsetting to have any active endpoints. some tests change + * altsettings; force a default so most tests don't need to check. + */ + if (dev->info->alt >= 0) { + if (intf->altsetting->desc.bInterfaceNumber) { + retval = -ENODEV; + goto free_mutex; + } + retval = set_altsetting(dev, dev->info->alt); + if (retval) { + dev_err(&intf->dev, + "set altsetting to %d failed, %d\n", + dev->info->alt, retval); + goto free_mutex; + } + } + + 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; + + default: + retval = -EOPNOTSUPP; + goto free_mutex; + } + + start = ktime_get(); + + retval = usbtest_do_ioctl(intf, param); + if (retval) + goto free_mutex; + + end = ktime_get(); + + duration = ktime_sub(end, start); + + switch (code) { + case USBTEST_REQUEST_COMPAT: + param_compat->duration = ktime_to_timeval(duration); + break; + + case USBTEST_REQUEST: + param->duration_ns = ktime_to_ns(duration); + break; } + +free_mutex: mutex_unlock(&dev->lock); return retval; }
+ /*-------------------------------------------------------------------------*/
static unsigned force_interrupt; @@ -2891,4 +2952,3 @@ module_exit(usbtest_exit);
MODULE_DESCRIPTION("USB Core/HCD Testing Driver"); MODULE_LICENSE("GPL"); -
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
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
On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
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:
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.
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.
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.
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.
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.
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.
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.
Good thinking, but this can be avoided by having two fixed-size structure definitions that are known to be different.
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?
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.
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?
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/
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
On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
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:
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.
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.
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.
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.
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.
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.
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.
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.
Good thinking, but this can be avoided by having two fixed-size structure definitions that are known to be different.
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?
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.
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?
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/
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
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.
-Deepa
On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann arnd@arndb.de 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; };
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:
- 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.
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; };
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.
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_...
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.
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.
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.
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.
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
Sorry, I'm not really following here. Doesn't this break existing 32-bit users that don't even care about the y2038 issue?
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.
- 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.
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.
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?
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/
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.
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
On Thu, Oct 22, 2015 at 1:45 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann arnd@arndb.de
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; };
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:
- 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.
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; };
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.
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_...
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.
Ok. I can do this. I can get back to you early next week.
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.
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.
Ok. I can contact the usb linux mailing list if they know of any other or just to fish around for their opinion. I'll keep you in the cc. Let me know if you would prefer the whole 2038/ outreachy mailing list instead.
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.
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
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
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.
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.
- 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.
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.
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?
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/
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.
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.
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.
Arnd
- Deepa
On Friday 23 October 2015 14:54:58 Deepa Dinamani wrote:
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.
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
That's not what I meant here. I mean using 32-bit tv_sec plus 32-bit tv_usec is always sufficient, because no test can run for more than 68 years and overflow the 32 bit number.
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
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.
What I want COMPAT_TIME to mean eventually is to support old binaries that are known to be broken in 2038. This one is not does not suffer from the overflow, so we can leave it enabled unconditionally.
#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.
That would work too, and the USB maintainers might like this method. My preference would still be to keep the existing interface as the primary way and not force a change on the users, no matter how bad the original decision was.
From looking at http://www.linux-usb.org/usbtest/, my impression
is that this tool was never packaged properly for distributions and instead a simple .c file is offered for direct download. This means we probably have lots copies of this file in random places that are being used but never updated, even if we can change the master copy to use another interface. The existing file will still compile on future libc versions, but will fail to work correctly for now apparent reason if we deprecate that ioctl command.
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.
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.
You can have a really long changelog text to describe this if you want.
Summarizing the next steps:
- Figure out how many ioctl or uapi need 64-bit timeval.
Sounds good.
- Should compat fix for existing usbtest ioctl be based on top of your
COMPAT_TIME changes?
I would prefer to see a fix for the driver that does not depend on my patches. Just do one that handles both variants of the data structure on both 32-bit and 64-bit architectures, without any #ifdef.
- 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.
Arnd
On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:
- 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.
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
On Sat, Oct 24, 2015 at 1:38 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:
- 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.
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.
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.
Arnd
-Deepa
On Sun, Oct 25, 2015 at 7:58 PM, Deepa Dinamani deepa.kernel@gmail.com wrote:
On Sat, Oct 24, 2015 at 1:38 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:
- 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.
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.
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.
Arnd
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.
-Deepa
On Friday 30 October 2015 10:55:10 Deepa Dinamani wrote:
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.
Ah, excellent!
I think there are a few that might not be on the list because I've already addressed them in another patch:
* include/uapi/linux/can/bcm.h (my patch is already merged) * include/uapi/linux/input.h (hard to fix, working on it) * include/uapi/linux/videodev2.h (also nontrivial, has been discussed at last weeks media summit, patch under work) * drivers/char/ppdev.c (patch exists, needs to get merged)
Each of the above is a bit different and needs a custom solution anyway, but it's definitely good to know that there are no others.
I submitted a v2 of the patch that doesn't require a new timeval struct accordingly.
Thanks,
Arnd
-
Arnd Bergmann -
Deepa Dinamani