Zitat von Greg KH <gre...@linuxfoundation.org>:

On Fri, May 18, 2018 at 02:48:11PM +0000, gu...@kiener-muenchen.de wrote:

Zitat von Greg KH <gre...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to