[PATCH] usb: usbtest: Add new ioctl interface

[PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

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(&param->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");
-
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

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

Re: [Outreachy kernel] [PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 6728 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 9281 bytes --]

Re: [Outreachy kernel] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

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

[PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 9510 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 11969 bytes --]

Re: [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

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

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

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

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

[PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 9186 bytes --]

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:
> >
> > 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.
>
> > 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_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.
>

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

[-- Attachment #2: Type: text/html, Size: 12360 bytes --]

Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

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:
> 
> 1. Figure out how many ioctl or uapi need 64-bit timeval.

Sounds good.

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

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

	Arnd

Re: [Outreachy kernel] Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

On Saturday 24 October 2015 00:19:06 Arnd Bergmann wrote:
> > 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.
> 

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

Re: [Outreachy kernel] Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2266 bytes --]

Re: [Outreachy kernel] Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3103 bytes --]

Re: [Outreachy kernel] Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann]

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

[PATCH] usb: usbtest: Add new ioctl interface

[Deepa Dinamani]

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>
---
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(&param->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");
-
-- 
1.9.1