diff mbox

[06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls

Message ID 20180517170336.8426-7-guido@kiener-muenchen.de (mailing list archive)
State New, archived
Headers show

Commit Message

Guido Kiener May 17, 2018, 5:03 p.m. UTC
- ioctl USBTMC_IOCTL_WRITE sends an (a)synchronous generic message
  to bulk OUT. The message is split into chunks of 4k (page size).
  Message size is aligned to 32 bit boundaries.

  With flag USBTMC_FLAG_ASYNC the ioctl is non blocking.
  With flag USBTMC_FLAG_APPEND additional urbs are queued and
  out_status/out_transfer_size is not reset. EPOLLOUT | EPOLLWRNORM
  is signaled when all submitted urbs are completed.

- ioctl USBTMC_IOCTL_WRITE_RESULT copies current out_transfer_size
  to given __u64 pointer and returns current out_status of the last
  USBTMC_IOCTL_WRITE call.

- ioctl USBTMC_IOCTL_READ reads an (a)synchronous generic message
  from bulk IN. Depending on transfer_size the function submits one
  (<=4kB) or more urbs (up to 16) with a size of 4k.

  The flag USBTMC_FLAG_IGNORE_TRAILER can be used when the transmission
  size is already known. Then the function does not truncate the
  transfer_size to a multiple of 4 kB, but does reserve extra space
  to receive the final short or zero length packet. Note that the
  instrument is allowed to send up to wMaxPacketSize - 1 bytes at the
  end of a message to avoid sending a zero length packet.

  With flag USBTMC_FLAG_ASYNC the ioctl is non blocking. When no
  received data is available, the read function submits as many urbs as
  needed to receive transfer_size bytes. However the number of flying
  urbs (=4kB) is limited to 16 even with subsequent calls of this ioctl.
  Returns -EAGAIN when non blocking and no data is received.
  Signals EPOLLIN | EPOLLRDNORM when asynchronous urbs are ready to
  be read.

  The usbtmc_message.message pointer can be NULL to just trigger
  reading data. However -EINVAL is returned when data is available,
  and the message pointer is NULL.

- ioctl USBTMC_IOCTL_CANCEL_IO cancels last USBTMC_IOCTL_READ and
  USBTMC_IOCTL_WRITE functions which returns -ECANCELED. A subsequent
  call to USBTMC_IOCTL_READ or USBTMC_IOCTL_WRITE_RESULT returns
  -ECANCELED with information about current transferred data.

- ioctl USBTMC_IOCTL_CLEANUP_IO kills all submitted urbs to OUT and
  IN bulk, and clears all received data from IN bulk. Internal
  transfer counters and error states are reset.

- Flush flying urbs when file handle is closed or device is
  suspended.

Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
Reviewed-by: Steve Bayless <steve_bayless@keysight.com>
---
 drivers/usb/class/usbtmc.c   | 778 ++++++++++++++++++++++++++++++++++-
 include/uapi/linux/usb/tmc.h |  21 +
 2 files changed, 797 insertions(+), 2 deletions(-)

Comments

Greg KH May 18, 2018, 12:44 p.m. UTC | #1
On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> - ioctl USBTMC_IOCTL_WRITE sends an (a)synchronous generic message
>   to bulk OUT. The message is split into chunks of 4k (page size).
>   Message size is aligned to 32 bit boundaries.
> 
>   With flag USBTMC_FLAG_ASYNC the ioctl is non blocking.
>   With flag USBTMC_FLAG_APPEND additional urbs are queued and
>   out_status/out_transfer_size is not reset. EPOLLOUT | EPOLLWRNORM
>   is signaled when all submitted urbs are completed.
> 
> - ioctl USBTMC_IOCTL_WRITE_RESULT copies current out_transfer_size
>   to given __u64 pointer and returns current out_status of the last
>   USBTMC_IOCTL_WRITE call.
> 
> - ioctl USBTMC_IOCTL_READ reads an (a)synchronous generic message
>   from bulk IN. Depending on transfer_size the function submits one
>   (<=4kB) or more urbs (up to 16) with a size of 4k.
> 
>   The flag USBTMC_FLAG_IGNORE_TRAILER can be used when the transmission
>   size is already known. Then the function does not truncate the
>   transfer_size to a multiple of 4 kB, but does reserve extra space
>   to receive the final short or zero length packet. Note that the
>   instrument is allowed to send up to wMaxPacketSize - 1 bytes at the
>   end of a message to avoid sending a zero length packet.

Again, one patch per ioctl please, it makes it easier to review.

And why are you allowing arbitrary bulk messages now?

What good is this spec if everyone goes around it?

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
Greg KH May 18, 2018, 12:46 p.m. UTC | #2
On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> +/*
> + * usbtmc_message->flags:
> + */
> +#define USBTMC_FLAG_ASYNC		0x0001
> +#define USBTMC_FLAG_APPEND		0x0002
> +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
> +
> +struct usbtmc_message {
> +	void __user *message; /* pointer to header and data */
> +	__u64 transfer_size; /* size of bytes to transfer */
> +	__u64 transferred; /* size of received/written bytes */
> +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> +} __attribute__ ((packed));

Very odd structure.  Your userspace pointer is going to be totally out
of alignment on 32bit systems running on a 64bit kernel.  Why have a
separate pointer at all?  Why not just put the mesage at the end of this
structure directly with something like:
	__u8 message[0];
?

Much easier and you don't have to mess with the whole compatible ioctl
thunking layer (which I think you ignored here, which means you all
didn't test it...)

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
Guido Kiener May 18, 2018, 2:21 p.m. UTC | #3
Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
>> - ioctl USBTMC_IOCTL_WRITE sends an (a)synchronous generic message
>>   to bulk OUT. The message is split into chunks of 4k (page size).
>>   Message size is aligned to 32 bit boundaries.
>>
>>   With flag USBTMC_FLAG_ASYNC the ioctl is non blocking.
>>   With flag USBTMC_FLAG_APPEND additional urbs are queued and
>>   out_status/out_transfer_size is not reset. EPOLLOUT | EPOLLWRNORM
>>   is signaled when all submitted urbs are completed.
>>
>> - ioctl USBTMC_IOCTL_WRITE_RESULT copies current out_transfer_size
>>   to given __u64 pointer and returns current out_status of the last
>>   USBTMC_IOCTL_WRITE call.
>>
>> - ioctl USBTMC_IOCTL_READ reads an (a)synchronous generic message
>>   from bulk IN. Depending on transfer_size the function submits one
>>   (<=4kB) or more urbs (up to 16) with a size of 4k.
>>
>>   The flag USBTMC_FLAG_IGNORE_TRAILER can be used when the transmission
>>   size is already known. Then the function does not truncate the
>>   transfer_size to a multiple of 4 kB, but does reserve extra space
>>   to receive the final short or zero length packet. Note that the
>>   instrument is allowed to send up to wMaxPacketSize - 1 bytes at the
>>   end of a message to avoid sending a zero length packet.
>
> Again, one patch per ioctl please, it makes it easier to review.
>
> And why are you allowing arbitrary bulk messages now?

All VISA applications have specific flavours in dealing with timeout,
algorithms to abort a message and reset a device. VISA applications need
a maximum of freedom in communication with an instrument to quickly react
with workarounds. There is no time to wait several weeks until a kernel
fix repairs a production line.

The current patches are a trade-off to meet the requirements of the
IVI foundation and to give back convenient ioctls (timeout, termchar)
to the community for the traditional usage of the usbtmc driver.

In case of R&S I (hope) we will use the read, write functions for synchronous
calls, and the new ioctls (USBTMC_IOCTL_READ/USBTMC_IOCTL_WRITE) for
asynchronous calls.

We need at least all generic ioctls. Other patches and ioctls for testing
halt conditions and abort we can withdraw.

>
> What good is this spec if everyone goes around it?
>
> 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
Guido Kiener May 18, 2018, 2:48 p.m. UTC | #4
Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
>> +/*
>> + * usbtmc_message->flags:
>> + */
>> +#define USBTMC_FLAG_ASYNC		0x0001
>> +#define USBTMC_FLAG_APPEND		0x0002
>> +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
>> +
>> +struct usbtmc_message {
>> +	void __user *message; /* pointer to header and data */
>> +	__u64 transfer_size; /* size of bytes to transfer */
>> +	__u64 transferred; /* size of received/written bytes */
>> +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
>> +} __attribute__ ((packed));
>
> Very odd structure.  Your userspace pointer is going to be totally out
> of alignment on 32bit systems running on a 64bit kernel.  Why have a
> separate pointer at all?  Why not just put the mesage at the end of this
> structure directly with something like:
> 	__u8 message[0];
> ?
>

Oh yes, I should know that from another project. Indead we did not test it
with 32 bit applications on 64 platforms.

When we use your proposal then we just can use
#define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)?


> Much easier and you don't have to mess with the whole compatible ioctl
> thunking layer (which I think you ignored here, which means you all
> didn't test it...)
>
> 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
Greg KH May 18, 2018, 2:54 p.m. UTC | #5
On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> > > +/*
> > > + * usbtmc_message->flags:
> > > + */
> > > +#define USBTMC_FLAG_ASYNC		0x0001
> > > +#define USBTMC_FLAG_APPEND		0x0002
> > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
> > > +
> > > +struct usbtmc_message {
> > > +	void __user *message; /* pointer to header and data */
> > > +	__u64 transfer_size; /* size of bytes to transfer */
> > > +	__u64 transferred; /* size of received/written bytes */
> > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> > > +} __attribute__ ((packed));
> > 
> > Very odd structure.  Your userspace pointer is going to be totally out
> > of alignment on 32bit systems running on a 64bit kernel.  Why have a
> > separate pointer at all?  Why not just put the mesage at the end of this
> > structure directly with something like:
> > 	__u8 message[0];
> > ?
> > 
> 
> Oh yes, I should know that from another project. Indead we did not test it
> with 32 bit applications on 64 platforms.
> 
> When we use your proposal then we just can use
> #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)?

Why 13?  Use the structure please, that way you know you always get it
right.

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
Guido Kiener May 18, 2018, 3:13 p.m. UTC | #6
Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
>>
>> Zitat von Greg KH <gregkh@linuxfoundation.org>:
>>
>> > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
>> > > +/*
>> > > + * usbtmc_message->flags:
>> > > + */
>> > > +#define USBTMC_FLAG_ASYNC		0x0001
>> > > +#define USBTMC_FLAG_APPEND		0x0002
>> > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
>> > > +
>> > > +struct usbtmc_message {
>> > > +	void __user *message; /* pointer to header and data */
>> > > +	__u64 transfer_size; /* size of bytes to transfer */
>> > > +	__u64 transferred; /* size of received/written bytes */
>> > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
>> > > +} __attribute__ ((packed));
>> >
>> > Very odd structure.  Your userspace pointer is going to be totally out
>> > of alignment on 32bit systems running on a 64bit kernel.  Why have a
>> > separate pointer at all?  Why not just put the mesage at the end of this
>> > structure directly with something like:
>> > 	__u8 message[0];
>> > ?
>> >
>>
>> Oh yes, I should know that from another project. Indead we did not test it
>> with 32 bit applications on 64 platforms.
>>
>> When we use your proposal then we just can use
>> #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)?
>
> Why 13?  Use the structure please, that way you know you always get it
> right.

Sorry. I should just read the documentation. Dave helps me. Thanks.

>
> 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
Guido Kiener May 21, 2018, 8:41 p.m. UTC | #7
Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
>>
>> Zitat von Greg KH <gregkh@linuxfoundation.org>:
>>
>> > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
>> > > +/*
>> > > + * usbtmc_message->flags:
>> > > + */
>> > > +#define USBTMC_FLAG_ASYNC		0x0001
>> > > +#define USBTMC_FLAG_APPEND		0x0002
>> > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
>> > > +
>> > > +struct usbtmc_message {
>> > > +	void __user *message; /* pointer to header and data */
>> > > +	__u64 transfer_size; /* size of bytes to transfer */
>> > > +	__u64 transferred; /* size of received/written bytes */
>> > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
>> > > +} __attribute__ ((packed));
>> >
>> > Very odd structure.  Your userspace pointer is going to be totally out
>> > of alignment on 32bit systems running on a 64bit kernel.  Why have a
>> > separate pointer at all?  Why not just put the mesage at the end of this
>> > structure directly with something like:
>> > 	__u8 message[0];
>> > ?
>> >

Using a __u8 message[0] ends up with an extra malloc, memcpy, and free
operation in userland, since we always have to append the data of a given
user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop
this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s
to 29,5 MByte/s with USB 2.0.

I hope this struct looks better (in compat mode):

struct usbtmc_message {
     __u64 transfer_size; /* size of bytes to transfer */
     __u64 transferred; /* size of received/written bytes */
     void __user *message; /* pointer to header and data */
     __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
} __attribute__ ((packed));

BTW the driver has no .compat_ioctl entry. So I wonder how it could work
with 32 bit applications on 64 bit systems before submission of our patches.
Do I miss something?

>>
>> Oh yes, I should know that from another project. Indead we did not test it
>> with 32 bit applications on 64 platforms.
>>
>> When we use your proposal then we just can use
>> #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)?
>
> Why 13?  Use the structure please, that way you know you always get it
> right.
>
> 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
Greg KH May 22, 2018, 10 a.m. UTC | #8
On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
> > > 
> > > Zitat von Greg KH <gregkh@linuxfoundation.org>:
> > > 
> > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> > > > > +/*
> > > > > + * usbtmc_message->flags:
> > > > > + */
> > > > > +#define USBTMC_FLAG_ASYNC		0x0001
> > > > > +#define USBTMC_FLAG_APPEND		0x0002
> > > > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
> > > > > +
> > > > > +struct usbtmc_message {
> > > > > +	void __user *message; /* pointer to header and data */
> > > > > +	__u64 transfer_size; /* size of bytes to transfer */
> > > > > +	__u64 transferred; /* size of received/written bytes */
> > > > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> > > > > +} __attribute__ ((packed));
> > > >
> > > > Very odd structure.  Your userspace pointer is going to be totally out
> > > > of alignment on 32bit systems running on a 64bit kernel.  Why have a
> > > > separate pointer at all?  Why not just put the mesage at the end of this
> > > > structure directly with something like:
> > > > 	__u8 message[0];
> > > > ?
> > > >
> 
> Using a __u8 message[0] ends up with an extra malloc, memcpy, and free
> operation in userland, since we always have to append the data of a given
> user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop
> this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s
> to 29,5 MByte/s with USB 2.0.

Really?  That feels like you are doing something really odd.  I guess
you need to figure out how you get the data to/from userspace.

> I hope this struct looks better (in compat mode):
> 
> struct usbtmc_message {
>     __u64 transfer_size; /* size of bytes to transfer */
>     __u64 transferred; /* size of received/written bytes */
>     void __user *message; /* pointer to header and data */
>     __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> } __attribute__ ((packed));

No, that will not work at all.  Again, think about the size of that
pointer.

> BTW the driver has no .compat_ioctl entry.

Because you didn't need it until now.

> So I wonder how it could work with 32 bit applications on 64 bit
> systems before submission of our patches.  Do I miss something?

You are creating structures that have different sizes on those two
different userspaces.  Because of that, you would be forced to have a
compat layer.  The smart thing to do is to design the interface to not
have that type of problem at all, don't create new apis that are not
just 64-bit sane to start with.

Copying memory is fast, really fast, much much faster than sending the
data across the USB connection.  If you are seeing major problems here,
then I would first look at your userspace code, and then see what the
kernel code does.

How about you wait for this new api until you get all of the other stuff
implemented/merged?  It looks like you all need to really think about
how this will all work out.

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
Guido Kiener May 22, 2018, 11:36 a.m. UTC | #9
Zitat von Greg KH <gregkh@linuxfoundation.org>:

> On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote:
>>
>> Zitat von Greg KH <gregkh@linuxfoundation.org>:
>>
>> > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
>> > >
>> > > Zitat von Greg KH <gregkh@linuxfoundation.org>:
>> > >
>> > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
>> > > > > +/*
>> > > > > + * usbtmc_message->flags:
>> > > > > + */
>> > > > > +#define USBTMC_FLAG_ASYNC		0x0001
>> > > > > +#define USBTMC_FLAG_APPEND		0x0002
>> > > > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
>> > > > > +
>> > > > > +struct usbtmc_message {
>> > > > > +	void __user *message; /* pointer to header and data */
>> > > > > +	__u64 transfer_size; /* size of bytes to transfer */
>> > > > > +	__u64 transferred; /* size of received/written bytes */
>> > > > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
>> > > > > +} __attribute__ ((packed));
>> > > >
>> > > > Very odd structure.  Your userspace pointer is going to be totally out
>> > > > of alignment on 32bit systems running on a 64bit kernel.  Why have a
>> > > > separate pointer at all?  Why not just put the mesage at the  
>> end of this
>> > > > structure directly with something like:
>> > > > 	__u8 message[0];
>> > > > ?
>> > > >
>>
>> Using a __u8 message[0] ends up with an extra malloc, memcpy, and free
>> operation in userland, since we always have to append the data of a given
>> user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop
>> this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s
>> to 29,5 MByte/s with USB 2.0.
>
> Really?  That feels like you are doing something really odd.  I guess
> you need to figure out how you get the data to/from userspace.

A typically user API is something like this:
ssize_t write(int fd, const void *buf, size_t count);
ssize_t send(int sockfd, const void *buf, size_t len, int flags);

We use a similar function:
ViStatus viWrite(ViSession vi, ViBuf buf, ViUInt32 count, ViPUInt32 retCount)

When a user calls viWrite(vi, "*IDN? + 4 MB ...\n", 40000000, &retcont) how
can we pass the big buf to the kernel without any copy operation?

Here is a sample algorithm with a minimum (4kB) of copy operation:
See function: tmc_raw_write_common(..) in
https://github.com/GuidoKiener/linux-usbtmc/blob/master/test-raw.c


>
>> I hope this struct looks better (in compat mode):
>>
>> struct usbtmc_message {
>>     __u64 transfer_size; /* size of bytes to transfer */
>>     __u64 transferred; /* size of received/written bytes */
>>     void __user *message; /* pointer to header and data */
>>     __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
>> } __attribute__ ((packed));
>
> No, that will not work at all.  Again, think about the size of that
> pointer.
>

The compat structure is:

struct compat_usbtmc_message {
	u64 transfer_size;
	u64 transferred;
	compat_uptr_t message;
	u32 flags;
} __packed;

For AMD64 it works.

>> BTW the driver has no .compat_ioctl entry.
>
> Because you didn't need it until now.

ioctl(fd,USBTMC_IOCTL_CLEAR) returns errno = 25 (=ENOTTY) in Linux
Kernel 4.15 when running test-raw32 created with
gcc -m32 test-raw.c -o test-raw32
Does it work with other kernel versions?

>
>> So I wonder how it could work with 32 bit applications on 64 bit
>> systems before submission of our patches.  Do I miss something?
>
> You are creating structures that have different sizes on those two
> different userspaces.  Because of that, you would be forced to have a
> compat layer.  The smart thing to do is to design the interface to not
> have that type of problem at all, don't create new apis that are not
> just 64-bit sane to start with.
>
> Copying memory is fast, really fast, much much faster than sending the
> data across the USB connection.  If you are seeing major problems here,
> then I would first look at your userspace code, and then see what the
> kernel code does.

I just do not like to be slower than libusb or other operating systems.

>
> How about you wait for this new api until you get all of the other stuff
> implemented/merged?  It looks like you all need to really think about
> how this will all work out.
>
> 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
Greg KH May 22, 2018, 1:37 p.m. UTC | #10
On Tue, May 22, 2018 at 11:36:13AM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote:
> > > 
> > > Zitat von Greg KH <gregkh@linuxfoundation.org>:
> > > 
> > > > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote:
> > > > >
> > > > > Zitat von Greg KH <gregkh@linuxfoundation.org>:
> > > > >
> > > > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote:
> > > > > > > +/*
> > > > > > > + * usbtmc_message->flags:
> > > > > > > + */
> > > > > > > +#define USBTMC_FLAG_ASYNC		0x0001
> > > > > > > +#define USBTMC_FLAG_APPEND		0x0002
> > > > > > > +#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
> > > > > > > +
> > > > > > > +struct usbtmc_message {
> > > > > > > +	void __user *message; /* pointer to header and data */
> > > > > > > +	__u64 transfer_size; /* size of bytes to transfer */
> > > > > > > +	__u64 transferred; /* size of received/written bytes */
> > > > > > > +	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> > > > > > > +} __attribute__ ((packed));
> > > > > >
> > > > > > Very odd structure.  Your userspace pointer is going to be totally out
> > > > > > of alignment on 32bit systems running on a 64bit kernel.  Why have a
> > > > > > separate pointer at all?  Why not just put the mesage at the
> > > end of this
> > > > > > structure directly with something like:
> > > > > > 	__u8 message[0];
> > > > > > ?
> > > > > >
> > > 
> > > Using a __u8 message[0] ends up with an extra malloc, memcpy, and free
> > > operation in userland, since we always have to append the data of a given
> > > user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop
> > > this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s
> > > to 29,5 MByte/s with USB 2.0.
> > 
> > Really?  That feels like you are doing something really odd.  I guess
> > you need to figure out how you get the data to/from userspace.
> 
> A typically user API is something like this:
> ssize_t write(int fd, const void *buf, size_t count);
> ssize_t send(int sockfd, const void *buf, size_t len, int flags);
> 
> We use a similar function:
> ViStatus viWrite(ViSession vi, ViBuf buf, ViUInt32 count, ViPUInt32 retCount)
> 
> When a user calls viWrite(vi, "*IDN? + 4 MB ...\n", 40000000, &retcont) how
> can we pass the big buf to the kernel without any copy operation?

Your function looks _just_ like a normal write() call, why are you even
trying to wrap it in an ioctl?  Just use write()!

> > > I hope this struct looks better (in compat mode):
> > > 
> > > struct usbtmc_message {
> > >     __u64 transfer_size; /* size of bytes to transfer */
> > >     __u64 transferred; /* size of received/written bytes */
> > >     void __user *message; /* pointer to header and data */
> > >     __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
> > > } __attribute__ ((packed));
> > 
> > No, that will not work at all.  Again, think about the size of that
> > pointer.
> > 
> 
> The compat structure is:
> 
> struct compat_usbtmc_message {
> 	u64 transfer_size;
> 	u64 transferred;
> 	compat_uptr_t message;
> 	u32 flags;
> } __packed;
> 
> For AMD64 it works.

Show me the size and layout of both structures for a 32bit and 64bit
userspace please.  There are tools that do this.

Again, this is not how to do it!  You do not put a variable sized
variable in the middle of a structure.  void * changes size!  Either
align everything properly or throw it at the end or even better yet,
DON'T DO THIS AT ALL, JUST USE write()! :)

> > > BTW the driver has no .compat_ioctl entry.
> > 
> > Because you didn't need it until now.
> 
> ioctl(fd,USBTMC_IOCTL_CLEAR) returns errno = 25 (=ENOTTY) in Linux
> Kernel 4.15 when running test-raw32 created with
> gcc -m32 test-raw.c -o test-raw32
> Does it work with other kernel versions?

I have no idea what you are testing here, sorry.

> > > So I wonder how it could work with 32 bit applications on 64 bit
> > > systems before submission of our patches.  Do I miss something?
> > 
> > You are creating structures that have different sizes on those two
> > different userspaces.  Because of that, you would be forced to have a
> > compat layer.  The smart thing to do is to design the interface to not
> > have that type of problem at all, don't create new apis that are not
> > just 64-bit sane to start with.
> > 
> > Copying memory is fast, really fast, much much faster than sending the
> > data across the USB connection.  If you are seeing major problems here,
> > then I would first look at your userspace code, and then see what the
> > kernel code does.
> 
> I just do not like to be slower than libusb or other operating systems.

If you do this correctly, your bottleneck will be the USB connection.

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

Patch

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 00c2e51a23a7..de664a345e69 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -36,6 +36,11 @@ 
 /* Default USB timeout (in milliseconds) */
 #define USBTMC_TIMEOUT		5000
 
+/* Max number of urbs used in write transfers */
+#define MAX_URBS_IN_FLIGHT	16
+/* 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
@@ -79,6 +84,9 @@  struct usbtmc_device_data {
 	u8 bTag_last_write;	/* needed for abort */
 	u8 bTag_last_read;	/* needed for abort */
 
+	/* packet size of IN bulk */
+	u16            wMaxPacketSize;
+
 	/* data for interrupt in endpoint handling */
 	u8             bNotify1;
 	u8             bNotify2;
@@ -122,16 +130,34 @@  struct usbtmc_file_data {
 	u32            timeout;
 	u8             srq_byte;
 	atomic_t       srq_asserted;
+	atomic_t       closing;
 
 	/* These values are initialized with default values from device_data */
 	u8             TermChar;
 	bool           TermCharEnabled;
 	bool           auto_abort;
 	u8             eom_val;
+
+	spinlock_t     err_lock; /* lock for errors */
+
+	struct usb_anchor submitted;
+
+	/* data for generic_write */
+	struct semaphore limit_write_sem;
+	u64 out_transfer_size;
+	int out_status;
+
+	/* data for generic_read */
+	u64 in_transfer_size;
+	int in_status;
+	int in_urbs_used;
+	struct usb_anchor in_anchor;
+	wait_queue_head_t wait_bulk_in;
 };
 
 /* Forward declarations */
 static struct usb_driver usbtmc_driver;
+static void usbtmc_draw_down(struct usbtmc_file_data *file_data);
 
 static void usbtmc_delete(struct kref *kref)
 {
@@ -160,6 +186,12 @@  static int usbtmc_open(struct inode *inode, struct file *filp)
 
 	pr_debug("%s - called\n", __func__);
 
+	spin_lock_init(&file_data->err_lock);
+	sema_init(&file_data->limit_write_sem, MAX_URBS_IN_FLIGHT);
+	init_usb_anchor(&file_data->submitted);
+	init_usb_anchor(&file_data->in_anchor);
+	init_waitqueue_head(&file_data->wait_bulk_in);
+
 	data = usb_get_intfdata(intf);
 	/* Protect reference to data from file structure until release */
 	kref_get(&data->kref);
@@ -167,6 +199,8 @@  static int usbtmc_open(struct inode *inode, struct file *filp)
 	mutex_lock(&data->io_mutex);
 	file_data->data = data;
 
+	atomic_set(&file_data->closing, 0);
+
 	/* copy default values from device settings */
 	file_data->timeout = USBTMC_TIMEOUT;
 	file_data->TermChar = data->TermChar;
@@ -186,6 +220,41 @@  static int usbtmc_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/*
+ * usbtmc_flush - called before file handle is closed
+ */
+static int usbtmc_flush(struct file *file, fl_owner_t id)
+{
+	struct usbtmc_file_data *file_data;
+	struct usbtmc_device_data *data;
+
+	file_data = file->private_data;
+	if (file_data == NULL)
+		return -ENODEV;
+
+	atomic_set(&file_data->closing, 1);
+	data = file_data->data;
+
+	/* wait for io to stop */
+	mutex_lock(&data->io_mutex);
+
+	usbtmc_draw_down(file_data);
+
+	spin_lock_irq(&file_data->err_lock);
+	file_data->in_status = 0;
+	file_data->in_transfer_size = 0;
+	file_data->in_urbs_used = 0;
+	file_data->out_status = 0;
+	file_data->out_transfer_size = 0;
+	spin_unlock_irq(&file_data->err_lock);
+
+	wake_up_interruptible_all(&data->waitq);
+	pr_debug("%s - called\n", __func__);
+	mutex_unlock(&data->io_mutex);
+
+	return 0;
+}
+
 static int usbtmc_release(struct inode *inode, struct file *file)
 {
 	struct usbtmc_file_data *file_data = file->private_data;
@@ -625,6 +694,555 @@  static int usbtmc488_ioctl_trigger(struct usbtmc_file_data *file_data)
 	return 0;
 }
 
+static struct urb *usbtmc_create_urb(void)
+{
+	const size_t bufsize = USBTMC_BUFSIZE;
+	u8 *dmabuf = NULL;
+	struct urb *urb = usb_alloc_urb(0, GFP_KERNEL);
+
+	if (!urb)
+		return NULL;
+
+	dmabuf = kmalloc(bufsize, GFP_KERNEL);
+	if (!dmabuf) {
+		usb_free_urb(urb);
+		return NULL;
+	}
+
+	urb->transfer_buffer = dmabuf;
+	urb->transfer_buffer_length = bufsize;
+	urb->transfer_flags |= URB_FREE_BUFFER;
+	return urb;
+}
+
+static void usbtmc_read_bulk_cb(struct urb *urb)
+{
+	struct usbtmc_file_data *file_data = urb->context;
+	int status = urb->status;
+
+	/* sync/async unlink faults aren't errors */
+	if (status) {
+		if (!(/* status == -ENOENT || */
+			status == -ECONNRESET ||
+			status == -EREMOTEIO || /* Short packet */
+			status == -ESHUTDOWN))
+			dev_err(&file_data->data->intf->dev,
+			"%s - nonzero read bulk status received: %d\n",
+			__func__, status);
+
+		spin_lock(&file_data->err_lock);
+		if (!file_data->in_status)
+			file_data->in_status = status;
+		spin_unlock(&file_data->err_lock);
+	}
+
+	spin_lock(&file_data->err_lock);
+	file_data->in_transfer_size += urb->actual_length;
+	dev_dbg(&file_data->data->intf->dev,
+		"%s - total size: %llu current: %d status: %d\n",
+		__func__, file_data->in_transfer_size,
+		urb->actual_length, status);
+	spin_unlock(&file_data->err_lock);
+	usb_anchor_urb(urb, &file_data->in_anchor);
+
+	wake_up_interruptible(&file_data->wait_bulk_in);
+	wake_up_interruptible(&file_data->data->waitq);
+}
+
+static inline bool usbtmc_do_transfer(struct usbtmc_file_data *file_data)
+{
+	bool data_or_error;
+
+	spin_lock_irq(&file_data->err_lock);
+	data_or_error = !usb_anchor_empty(&file_data->in_anchor)
+			|| file_data->in_status;
+	spin_unlock_irq(&file_data->err_lock);
+	dev_dbg(&file_data->data->intf->dev, "%s: returns %d\n", __func__,
+		data_or_error);
+	return data_or_error;
+}
+
+static ssize_t usbtmc_generic_read(struct usbtmc_file_data *file_data,
+				   void __user *user_buffer,
+				   u64 transfer_size,
+				   u64 *transferred,
+				   u32 flags)
+{
+	struct usbtmc_device_data *data = file_data->data;
+	struct device *dev = &data->intf->dev;
+	u64 done = 0;
+	u64 remaining;
+	const size_t bufsize = USBTMC_BUFSIZE;
+	int retval = 0;
+	u64 max_transfer_size;
+	unsigned long expire;
+	int bufcount = 1;
+	int again = 0;
+
+	/* mutex already locked */
+
+	*transferred = done;
+
+	max_transfer_size = transfer_size;
+
+	if (flags & USBTMC_FLAG_IGNORE_TRAILER) {
+		/* The device may send extra alignment bytes (up to
+		 * wMaxPacketSize – 1) to avoid sending a zero-length
+		 * packet
+		 */
+		remaining = transfer_size;
+		if (((u32)max_transfer_size % data->wMaxPacketSize) == 0)
+			max_transfer_size += (data->wMaxPacketSize - 1);
+	} else {
+		/* round down to bufsize to avoid truncated data left */
+		if (max_transfer_size > bufsize) {
+			max_transfer_size =
+				roundup(max_transfer_size + 1 - bufsize,
+					bufsize);
+		}
+		remaining = max_transfer_size;
+	}
+
+	spin_lock_irq(&file_data->err_lock);
+
+	if (file_data->in_status) {
+		/* return the very first error */
+		retval = file_data->in_status;
+		spin_unlock_irq(&file_data->err_lock);
+		goto error;
+	}
+
+	if (flags & USBTMC_FLAG_ASYNC) {
+		if (usb_anchor_empty(&file_data->in_anchor))
+			again = 1;
+
+		if (file_data->in_urbs_used == 0) {
+			file_data->in_transfer_size = 0;
+			file_data->in_status = 0;
+		}
+	} else {
+		file_data->in_transfer_size = 0;
+		file_data->in_status = 0;
+	}
+
+	if (max_transfer_size == 0) {
+		bufcount = 0;
+	} else {
+		bufcount = roundup(max_transfer_size, bufsize) / bufsize;
+		if (bufcount > file_data->in_urbs_used)
+			bufcount -= file_data->in_urbs_used;
+		else
+			bufcount = 0;
+
+		if (bufcount + file_data->in_urbs_used > MAX_URBS_IN_FLIGHT) {
+			bufcount = MAX_URBS_IN_FLIGHT -
+					file_data->in_urbs_used;
+		}
+	}
+	spin_unlock_irq(&file_data->err_lock);
+
+	dev_dbg(dev, "%s: requested=%llu flags=0x%X size=%llu bufs=%d used=%d\n",
+		__func__, (u64)transfer_size, (unsigned int)flags,
+		max_transfer_size, bufcount, file_data->in_urbs_used);
+
+	while (bufcount > 0) {
+		u8 *dmabuf = NULL;
+		struct urb *urb = usbtmc_create_urb();
+
+		if (!urb) {
+			retval = -ENOMEM;
+			goto error;
+		}
+
+		dmabuf = urb->transfer_buffer;
+
+		usb_fill_bulk_urb(urb, data->usb_dev,
+			usb_rcvbulkpipe(data->usb_dev, data->bulk_in),
+			dmabuf, bufsize,
+			usbtmc_read_bulk_cb, file_data);
+
+		usb_anchor_urb(urb, &file_data->submitted);
+		retval = usb_submit_urb(urb, GFP_KERNEL);
+		/* urb is anchored. We can release our reference. */
+		usb_free_urb(urb);
+		if (unlikely(retval)) {
+			usb_unanchor_urb(urb);
+			goto error;
+		}
+		file_data->in_urbs_used++;
+		bufcount--;
+	}
+
+	if (again) {
+		dev_dbg(dev, "%s: ret=again\n", __func__);
+		return -EAGAIN;
+	}
+
+	if (user_buffer == NULL)
+		return -EINVAL;
+
+	expire = msecs_to_jiffies(file_data->timeout);
+
+	while (max_transfer_size > 0) {
+		size_t this_part;
+		struct urb *urb = NULL;
+
+		if (!(flags & USBTMC_FLAG_ASYNC)) {
+			dev_dbg(dev, "%s: before wait time %lu\n",
+				__func__, expire);
+			retval = wait_event_interruptible_timeout(
+				file_data->wait_bulk_in,
+				usbtmc_do_transfer(file_data),
+				expire);
+
+			dev_dbg(dev, "%s: wait returned %d\n",
+				__func__, retval);
+
+			if (retval <= 0) {
+				if (retval == 0)
+					retval = -ETIMEDOUT;
+				goto error;
+			}
+		}
+
+		urb = usb_get_from_anchor(&file_data->in_anchor);
+		if (!urb) {
+			if (!(flags & USBTMC_FLAG_ASYNC)) {
+				/* synchronous case: must not happen */
+				retval = -EFAULT;
+				goto error;
+			}
+
+			/* asynchronous case: ready, do not block or wait */
+			*transferred = done;
+			dev_dbg(dev, "%s: (async) done=%llu ret=0\n",
+				__func__, done);
+			return 0;
+		}
+
+		file_data->in_urbs_used--;
+
+		if (max_transfer_size > urb->actual_length)
+			max_transfer_size -= urb->actual_length;
+		else
+			max_transfer_size = 0;
+
+		if (remaining > urb->actual_length)
+			this_part = urb->actual_length;
+		else
+			this_part = remaining;
+
+		print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, 16, 1,
+			urb->transfer_buffer, urb->actual_length, true);
+
+		if (copy_to_user(user_buffer + done,
+				 urb->transfer_buffer, this_part)) {
+			usb_free_urb(urb);
+			retval = -EFAULT;
+			goto error;
+		}
+
+		remaining -= this_part;
+		done += this_part;
+
+		spin_lock_irq(&file_data->err_lock);
+		if (urb->status) {
+			/* return the very first error */
+			retval = file_data->in_status;
+			spin_unlock_irq(&file_data->err_lock);
+			usb_free_urb(urb);
+			goto error;
+		}
+		spin_unlock_irq(&file_data->err_lock);
+
+		if (urb->actual_length < bufsize) {
+			/* short packet or ZLP received => ready */
+			usb_free_urb(urb);
+			retval = 1;
+			break;
+		}
+
+		if (!(flags & USBTMC_FLAG_ASYNC) &&
+		    max_transfer_size > (bufsize * file_data->in_urbs_used)) {
+			/* resubmit, since other buffers still not enough */
+			usb_anchor_urb(urb, &file_data->submitted);
+			retval = usb_submit_urb(urb, GFP_KERNEL);
+			if (unlikely(retval)) {
+				usb_unanchor_urb(urb);
+				usb_free_urb(urb);
+				goto error;
+			}
+			file_data->in_urbs_used++;
+		}
+		usb_free_urb(urb);
+		retval = 0;
+	}
+
+error:
+	*transferred = done;
+
+	dev_dbg(dev, "%s: before kill\n", __func__);
+	/* Attention: killing urbs can take long time (2 ms) */
+	usb_kill_anchored_urbs(&file_data->submitted);
+	dev_dbg(dev, "%s: after kill\n", __func__);
+	usb_scuttle_anchored_urbs(&file_data->in_anchor);
+	file_data->in_urbs_used = 0;
+	file_data->in_status = 0; /* no spinlock needed here */
+	dev_dbg(dev, "%s: done=%llu ret=%d\n", __func__, done, retval);
+
+	return retval;
+}
+
+static ssize_t usbtmc_ioctl_generic_read(struct usbtmc_file_data *file_data,
+					  void __user *arg)
+{
+	struct usbtmc_message msg;
+	ssize_t retval = 0;
+
+	/* mutex already locked */
+
+	if (copy_from_user(&msg, arg, sizeof(struct usbtmc_message)))
+		return -EFAULT;
+
+	retval = usbtmc_generic_read(file_data, msg.message,
+				      msg.transfer_size, &msg.transferred,
+				      msg.flags);
+
+	if (put_user(msg.transferred,
+		     &((struct usbtmc_message __user *)arg)->transferred))
+		return -EFAULT;
+
+	return retval;
+}
+
+static void usbtmc_write_bulk_cb(struct urb *urb)
+{
+	struct usbtmc_file_data *file_data = urb->context;
+	int wakeup = 0;
+
+	spin_lock(&file_data->err_lock);
+	file_data->out_transfer_size += urb->actual_length;
+
+	/* sync/async unlink faults aren't errors */
+	if (urb->status) {
+		if (!(urb->status == -ENOENT ||
+			urb->status == -ECONNRESET ||
+			urb->status == -ESHUTDOWN))
+			dev_err(&file_data->data->intf->dev,
+				"%s - nonzero write bulk status received: %d\n",
+				__func__, urb->status);
+
+		if (!file_data->out_status) {
+			file_data->out_status = urb->status;
+			wakeup = 1;
+		}
+	}
+	spin_unlock(&file_data->err_lock);
+
+	dev_dbg(&file_data->data->intf->dev,
+		"%s - write bulk total size: %llu\n",
+		__func__, file_data->out_transfer_size);
+
+	up(&file_data->limit_write_sem);
+	if (usb_anchor_empty(&file_data->submitted) || wakeup)
+		wake_up_interruptible(&file_data->data->waitq);
+}
+
+static ssize_t usbtmc_generic_write(struct usbtmc_file_data *file_data,
+				    const void __user *user_buffer,
+				    u64 transfer_size,
+				    u64 *transferred,
+				    u32 flags)
+{
+	struct usbtmc_device_data *data = file_data->data;
+	struct device *dev;
+	u64 done = 0;
+	u64 remaining;
+	unsigned long expire;
+	const size_t bufsize = USBTMC_BUFSIZE;
+	struct urb *urb = NULL;
+	int retval = 0;
+	u32 timeout;
+
+	*transferred = 0;
+
+	/* Get pointer to private data structure */
+	dev = &data->intf->dev;
+
+	dev_dbg(dev, "%s: size=%llu flags=0x%X sema=%u\n",
+		__func__, transfer_size, (unsigned int)flags,
+		file_data->limit_write_sem.count);
+
+	if (flags & USBTMC_FLAG_APPEND) {
+		spin_lock_irq(&file_data->err_lock);
+		retval = file_data->out_status;
+		spin_unlock_irq(&file_data->err_lock);
+		if (retval < 0)
+			return retval;
+	} else {
+		spin_lock_irq(&file_data->err_lock);
+		file_data->out_transfer_size = 0;
+		file_data->out_status = 0;
+		spin_unlock_irq(&file_data->err_lock);
+	}
+
+	remaining = transfer_size;
+
+	timeout = file_data->timeout;
+	expire = msecs_to_jiffies(timeout);
+
+	while (remaining > 0) {
+		size_t this_part, aligned;
+		u8 *buffer = NULL;
+
+		if (flags & USBTMC_FLAG_ASYNC) {
+			if (down_trylock(&file_data->limit_write_sem)) {
+				retval = (done)?(0):(-EAGAIN);
+				goto exit;
+			}
+		} else {
+			retval = down_timeout(&file_data->limit_write_sem,
+					      expire);
+			if (retval < 0) {
+				retval = -ETIMEDOUT;
+				goto error;
+			}
+		}
+
+		spin_lock_irq(&file_data->err_lock);
+		retval = file_data->out_status;
+		spin_unlock_irq(&file_data->err_lock);
+		if (retval < 0) {
+			up(&file_data->limit_write_sem);
+			goto error;
+		}
+
+		/* prepare next urb to send */
+		urb = usbtmc_create_urb();
+		if (!urb) {
+			retval = -ENOMEM;
+			up(&file_data->limit_write_sem);
+			goto error;
+		}
+		buffer = urb->transfer_buffer;
+
+		if (remaining > bufsize)
+			this_part = bufsize;
+		else
+			this_part = remaining;
+
+		if (copy_from_user(buffer, user_buffer + done, this_part)) {
+			retval = -EFAULT;
+			up(&file_data->limit_write_sem);
+			goto error;
+		}
+
+		print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE,
+			16, 1, buffer, this_part, true);
+
+		/* fill bulk with 32 bit alignment to meet USBTMC specification
+		 * (size + 3 & ~3) rounds up and simplifies user code
+		 */
+		aligned = (this_part + 3) & ~3;
+		dev_dbg(dev, "write(size:%u align:%u done:%u)\n",
+			(unsigned int)this_part,
+			(unsigned int)aligned,
+			(unsigned int)done);
+
+		usb_fill_bulk_urb(urb, data->usb_dev,
+			usb_sndbulkpipe(data->usb_dev, data->bulk_out),
+			urb->transfer_buffer, aligned,
+			usbtmc_write_bulk_cb, file_data);
+
+		usb_anchor_urb(urb, &file_data->submitted);
+		retval = usb_submit_urb(urb, GFP_KERNEL);
+		if (unlikely(retval)) {
+			usb_unanchor_urb(urb);
+			up(&file_data->limit_write_sem);
+			goto error;
+		}
+
+		usb_free_urb(urb);
+		urb = NULL; /* urb will be finally released by usb driver */
+
+		remaining -= this_part;
+		done += this_part;
+	}
+
+	/* All urbs are on the fly */
+	if (!(flags & USBTMC_FLAG_ASYNC)) {
+		if (!usb_wait_anchor_empty_timeout(&file_data->submitted,
+						   timeout)) {
+			retval = -ETIMEDOUT;
+			goto error;
+		}
+	}
+
+	retval = 0;
+	goto exit;
+
+error:
+	usb_kill_anchored_urbs(&file_data->submitted);
+exit:
+	usb_free_urb(urb);
+
+	spin_lock_irq(&file_data->err_lock);
+	if (!(flags & USBTMC_FLAG_ASYNC))
+		done = file_data->out_transfer_size;
+	if (!retval && file_data->out_status)
+		retval = file_data->out_status;
+	spin_unlock_irq(&file_data->err_lock);
+
+	*transferred = done;
+
+	dev_dbg(dev, "%s: done=%llu, retval=%d, urbstat=%d\n",
+		__func__, done, retval, file_data->out_status);
+
+	return retval;
+}
+
+static ssize_t usbtmc_ioctl_generic_write(struct usbtmc_file_data *file_data,
+					  void __user *arg)
+{
+	struct usbtmc_message msg;
+	ssize_t retval = 0;
+
+	/* mutex already locked */
+
+	if (copy_from_user(&msg, arg, sizeof(struct usbtmc_message)))
+		return -EFAULT;
+
+	retval = usbtmc_generic_write(file_data, msg.message,
+				      msg.transfer_size, &msg.transferred,
+				      msg.flags);
+
+	if (put_user(msg.transferred,
+		     &((struct usbtmc_message __user *)arg)->transferred))
+		return -EFAULT;
+
+	return retval;
+}
+
+/*
+ * Get the generic write result
+ */
+static ssize_t usbtmc_ioctl_write_result(struct usbtmc_file_data *file_data,
+				void __user *arg)
+{
+	__u64 transferred;
+	int retval;
+
+	spin_lock_irq(&file_data->err_lock);
+	transferred = file_data->out_transfer_size;
+	retval = file_data->out_status;
+	spin_unlock_irq(&file_data->err_lock);
+
+	if (put_user(transferred, (__u64 __user *)arg))
+		return -EFAULT;
+
+	return retval;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -1091,6 +1709,33 @@  static int usbtmc_ioctl_clear_in_halt(struct usbtmc_device_data *data)
 	return 0;
 }
 
+static int usbtmc_ioctl_cancel_io(struct usbtmc_file_data *file_data)
+{
+	dev_dbg(&file_data->data->intf->dev, "%s - called: %d\n", __func__, 0);
+	spin_lock_irq(&file_data->err_lock);
+	file_data->in_status = -ECANCELED;
+	file_data->out_status = -ECANCELED;
+	spin_unlock_irq(&file_data->err_lock);
+	usb_kill_anchored_urbs(&file_data->submitted);
+	return 0;
+}
+
+static int usbtmc_ioctl_cleanup_io(struct usbtmc_file_data *file_data)
+{
+	dev_dbg(&file_data->data->intf->dev, "%s - called: %d\n", __func__, 0);
+	usb_kill_anchored_urbs(&file_data->submitted);
+	usb_scuttle_anchored_urbs(&file_data->in_anchor);
+	spin_lock_irq(&file_data->err_lock);
+	file_data->in_status = 0;
+	file_data->in_transfer_size = 0;
+	file_data->out_status = 0;
+	file_data->out_transfer_size = 0;
+	spin_unlock_irq(&file_data->err_lock);
+
+	file_data->in_urbs_used = 0;
+	return 0;
+}
+
 static int get_capabilities(struct usbtmc_device_data *data)
 {
 	struct device *dev = &data->usb_dev->dev;
@@ -1460,6 +2105,21 @@  static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 						   (void __user *)arg);
 		break;
 
+	case USBTMC_IOCTL_WRITE:
+		retval = usbtmc_ioctl_generic_write(file_data,
+						    (void __user *)arg);
+		break;
+
+	case USBTMC_IOCTL_READ:
+		retval = usbtmc_ioctl_generic_read(file_data,
+						   (void __user *)arg);
+		break;
+
+	case USBTMC_IOCTL_WRITE_RESULT:
+		retval = usbtmc_ioctl_write_result(file_data,
+						   (void __user *)arg);
+		break;
+
 	case USBTMC488_IOCTL_GET_CAPS:
 		retval = put_user(data->usb488_caps,
 				  (unsigned char __user *)arg);
@@ -1488,6 +2148,14 @@  static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case USBTMC488_IOCTL_TRIGGER:
 		retval = usbtmc488_ioctl_trigger(file_data);
 		break;
+
+	case USBTMC_IOCTL_CANCEL_IO:
+		retval = usbtmc_ioctl_cancel_io(file_data);
+		break;
+
+	case USBTMC_IOCTL_CLEANUP_IO:
+		retval = usbtmc_ioctl_cleanup_io(file_data);
+		break;
 	}
 
 skip_io_on_zombie:
@@ -1517,7 +2185,28 @@  static __poll_t usbtmc_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &data->waitq, wait);
 
-	mask = (atomic_read(&file_data->srq_asserted)) ? EPOLLPRI : 0;
+	/* Note that EPOLLPRI is now assigned to SRQ, and
+	 * EPOLLIN|EPOLLRDNORM to normal read data.
+	 */
+	mask = 0;
+	if (atomic_read(&file_data->srq_asserted))
+		mask |= EPOLLPRI;
+
+	/* Note that the anchor submitted includes all urbs for BULK IN
+	 * and OUT. So EPOLLOUT is signaled when BULK OUT is empty and
+	 * all BULK IN urbs are completed and moved to in_anchor.
+	 */
+	if (usb_anchor_empty(&file_data->submitted))
+		mask |= (EPOLLOUT | EPOLLWRNORM);
+	if (!usb_anchor_empty(&file_data->in_anchor))
+		mask |= (EPOLLIN | EPOLLRDNORM);
+
+	spin_lock_irq(&file_data->err_lock);
+	if (file_data->in_status || file_data->out_status)
+		mask |= EPOLLERR;
+	spin_unlock_irq(&file_data->err_lock);
+
+	dev_dbg(&data->intf->dev, "poll mask = %x\n", (unsigned int)mask);
 
 no_poll:
 	mutex_unlock(&data->io_mutex);
@@ -1530,6 +2219,7 @@  static const struct file_operations fops = {
 	.write		= usbtmc_write,
 	.open		= usbtmc_open,
 	.release	= usbtmc_release,
+	.flush		= usbtmc_flush,
 	.unlocked_ioctl	= usbtmc_ioctl,
 	.fasync         = usbtmc_fasync,
 	.poll           = usbtmc_poll,
@@ -1669,6 +2359,7 @@  static int usbtmc_probe(struct usb_interface *intf,
 	}
 
 	data->bulk_in = bulk_in->bEndpointAddress;
+	data->wMaxPacketSize = usb_endpoint_maxp(bulk_in);
 	dev_dbg(&intf->dev, "Found bulk in endpoint at %u\n", data->bulk_in);
 
 	data->bulk_out = bulk_out->bEndpointAddress;
@@ -1750,6 +2441,7 @@  static int usbtmc_probe(struct usb_interface *intf,
 static void usbtmc_disconnect(struct usb_interface *intf)
 {
 	struct usbtmc_device_data *data  = usb_get_intfdata(intf);
+	struct list_head *elem;
 
 	dev_dbg(&intf->dev, "%s - called\n", __func__);
 
@@ -1759,19 +2451,99 @@  static void usbtmc_disconnect(struct usb_interface *intf)
 	mutex_lock(&data->io_mutex);
 	data->zombie = 1;
 	wake_up_interruptible_all(&data->waitq);
+	list_for_each(elem, &data->file_list) {
+		struct usbtmc_file_data *file_data;
+
+		file_data = list_entry(elem,
+				       struct usbtmc_file_data,
+				       file_elem);
+		usb_kill_anchored_urbs(&file_data->submitted);
+		usb_scuttle_anchored_urbs(&file_data->in_anchor);
+	}
 	mutex_unlock(&data->io_mutex);
 	usbtmc_free_int(data);
 	kref_put(&data->kref, usbtmc_delete);
 }
 
+static void usbtmc_draw_down(struct usbtmc_file_data *file_data)
+{
+	int time;
+
+	time = usb_wait_anchor_empty_timeout(&file_data->submitted, 1000);
+	if (!time)
+		usb_kill_anchored_urbs(&file_data->submitted);
+	usb_scuttle_anchored_urbs(&file_data->in_anchor);
+}
+
 static int usbtmc_suspend(struct usb_interface *intf, pm_message_t message)
 {
-	/* this driver does not have pending URBs */
+	struct usbtmc_device_data *data = usb_get_intfdata(intf);
+	struct list_head *elem;
+
+	dev_dbg(&intf->dev, "%s - called\n", __func__);
+
+	if (!data)
+		return 0;
+
+	mutex_lock(&data->io_mutex);
+	list_for_each(elem, &data->file_list) {
+		struct usbtmc_file_data *file_data;
+
+		file_data = list_entry(elem,
+				       struct usbtmc_file_data,
+				       file_elem);
+		usbtmc_draw_down(file_data);
+	}
+
+	if (data->iin_ep_present && data->iin_urb)
+		usb_kill_urb(data->iin_urb);
+
+	mutex_unlock(&data->io_mutex);
 	return 0;
 }
 
 static int usbtmc_resume(struct usb_interface *intf)
 {
+	struct usbtmc_device_data *data = usb_get_intfdata(intf);
+	int retcode = 0;
+
+	dev_dbg(&intf->dev, "%s - called\n", __func__);
+	if (data->iin_ep_present && data->iin_urb)
+		retcode = usb_submit_urb(data->iin_urb, GFP_KERNEL);
+	if (retcode)
+		dev_err(&intf->dev, "Failed to submit iin_urb\n");
+
+	return retcode;
+}
+
+static int usbtmc_pre_reset(struct usb_interface *intf)
+{
+	struct usbtmc_device_data *data  = usb_get_intfdata(intf);
+	struct list_head *elem;
+
+	if (!data)
+		return 0;
+
+	mutex_lock(&data->io_mutex);
+
+	list_for_each(elem, &data->file_list) {
+		struct usbtmc_file_data *file_data;
+
+		file_data = list_entry(elem,
+				       struct usbtmc_file_data,
+				       file_elem);
+		usbtmc_ioctl_cancel_io(file_data);
+	}
+
+	return 0;
+}
+
+static int usbtmc_post_reset(struct usb_interface *intf)
+{
+	struct usbtmc_device_data *data  = usb_get_intfdata(intf);
+
+	mutex_unlock(&data->io_mutex);
+
 	return 0;
 }
 
@@ -1782,6 +2554,8 @@  static struct usb_driver usbtmc_driver = {
 	.disconnect	= usbtmc_disconnect,
 	.suspend	= usbtmc_suspend,
 	.resume		= usbtmc_resume,
+	.pre_reset	= usbtmc_pre_reset,
+	.post_reset	= usbtmc_post_reset,
 };
 
 module_usb_driver(usbtmc_driver);
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index c1acec9ad834..1540716de293 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -59,6 +59,20 @@  struct usbtmc_termchar {
 	__u8 term_char_enabled; // bool
 } __attribute__ ((packed));
 
+/*
+ * usbtmc_message->flags:
+ */
+#define USBTMC_FLAG_ASYNC		0x0001
+#define USBTMC_FLAG_APPEND		0x0002
+#define USBTMC_FLAG_IGNORE_TRAILER	0x0004
+
+struct usbtmc_message {
+	void __user *message; /* pointer to header and data */
+	__u64 transfer_size; /* size of bytes to transfer */
+	__u64 transferred; /* size of received/written bytes */
+	__u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */
+} __attribute__ ((packed));
+
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91
 #define USBTMC_IOCTL_INDICATOR_PULSE	_IO(USBTMC_IOC_NR, 1)
@@ -72,6 +86,9 @@  struct usbtmc_termchar {
 #define USBTMC_IOCTL_SET_TIMEOUT	_IOW(USBTMC_IOC_NR, 10, unsigned int)
 #define USBTMC_IOCTL_EOM_ENABLE	        _IOW(USBTMC_IOC_NR, 11, __u8)
 #define USBTMC_IOCTL_CONFIG_TERMCHAR	_IOW(USBTMC_IOC_NR, 12, struct usbtmc_termchar)
+#define USBTMC_IOCTL_WRITE		_IOWR(USBTMC_IOC_NR, 13, struct usbtmc_message)
+#define USBTMC_IOCTL_READ		_IOWR(USBTMC_IOC_NR, 14, struct usbtmc_message)
+#define USBTMC_IOCTL_WRITE_RESULT	_IOWR(USBTMC_IOC_NR, 15, __u64)
 
 #define USBTMC488_IOCTL_GET_CAPS	_IOR(USBTMC_IOC_NR, 17, unsigned char)
 #define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
@@ -80,6 +97,10 @@  struct usbtmc_termchar {
 #define USBTMC488_IOCTL_LOCAL_LOCKOUT	_IO(USBTMC_IOC_NR, 21)
 #define USBTMC488_IOCTL_TRIGGER		_IO(USBTMC_IOC_NR, 22)
 
+/* Cancel and cleanup asynchronous calls */
+#define USBTMC_IOCTL_CANCEL_IO		_IO(USBTMC_IOC_NR, 35)
+#define USBTMC_IOCTL_CLEANUP_IO		_IO(USBTMC_IOC_NR, 36)
+
 /* Driver encoded usb488 capabilities */
 #define USBTMC488_CAPABILITY_TRIGGER         1
 #define USBTMC488_CAPABILITY_SIMPLE          2