[v2,08/29] usb: usbtmc: Add ioctl for generic requests on control

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Jul 21, 2018 at 11:11:55AM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Wed, Jul 18, 2018 at 10:45:41AM +0200, Guido Kiener wrote:
> > > Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the
> > > control pipe.  Used by specific applications of IVI Foundation,
> > > Inc. to implement VISA API functions: viUsbControlIn/Out.
> > > 
> > > The maximum length of control request is set to 4k.
> > > 
> > > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
> > > Reviewed-by: Steve Bayless <steve_bayless@keysight.com>
> > > ---
> > >  drivers/usb/class/usbtmc.c   | 151 +++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/usb/tmc.h |  15 ++++
> > >  2 files changed, 166 insertions(+)
> > > 
> > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > > index d685db78b80b..846599dd0c84 100644
> > > --- a/drivers/usb/class/usbtmc.c
> > > +++ b/drivers/usb/class/usbtmc.c
> > > @@ -5,6 +5,7 @@
> > >   * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
> > >   * Copyright (C) 2008 Novell, Inc.
> > >   * Copyright (C) 2008 Greg Kroah-Hartman <gregkh@suse.de>
> > > + * Copyright (C) 2018 IVI Foundation, Inc.
> > >   */
> > > 
> > >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > @@ -36,6 +37,9 @@
> > >  /* Default USB timeout (in milliseconds) */
> > >  #define USBTMC_TIMEOUT		5000
> > > 
> > > +/* I/O buffer size used in generic read/write functions */
> > > +#define USBTMC_BUFSIZE		(4096)
> > > +
> > >  /*
> > >   * Maximum number of read cycles to empty bulk in endpoint during CLEAR and
> > >   * ABORT_BULK_IN requests. Ends the loop if (for whatever reason) a short
> > > @@ -126,6 +130,17 @@ struct usbtmc_file_data {
> > >  	bool           term_char_enabled;
> > >  };
> > > 
> > > +#ifdef CONFIG_COMPAT
> > > +
> > > +struct compat_usbtmc_ctrlrequest {
> > > +	struct usbtmc_request req;
> > > +	compat_uptr_t data;
> > > +} __packed;
> > 
> > Wait, why?  You are creating a new api here, why do you need a 32bit
> > compat layer?  Just create it in a way that works for both layouts.
> > Just make your pointer a 64bit value and cast it to the pointer when
> > using it.  Other ioctls do this in lots of places, making things simpler
> > overall.  That way you don't need an ioctl compat call at all.
> > 
> > Please rework the code that way, don't create new apis that have to add
> > "old" 32bit compatibility support to them.  That's just extra work you
> > don't need to do here.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Maybe I'm still missing something. You wanted us that the driver supports
> 32 bit applications on 64 bit systems. Therefore we added the patch
> [PATCH v2 07/29] usb: usbtmc: Add support for 32 bit compat applications
> I do not know any other way to make ioctl functions working for
> 32 bit application, except adding the line .compat_ioctl =
> usbtmc_compat_ioctl.
> Please note that all other ioctl calls like USBTMC_IOCTL_CLEAR etc. do not
> work for 32 applications as well when we omit patch 07.
> 
> Please let us first agree on patch 07 and then we can continue to discuss
> patch 08.

"patch 07" was about 300 patches ago in the number of patches I have
reviewed today, sorry, I don't reallhy remember what it looked like but
I think it was something like adding a custom compat_ioctl handler,
right?

If so, no, that's not correct, your compat_ioctl function should be the
same as your "normal" ioctl function.  You do not need a separate
function for 32 vs. 64 bits if you do everything correctly.

So yes, you are right, 32bit applications will not work until you set
compat_ioctl, but just be sure to set it to the correct thing.

So feel free to resend this type of change, but just do not make any
special ioctl handlers for 32bits.  If you do that, you have not
designed your ioctls correctly.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html