Re: [PATCH 06/36] usb: serial: ti_usb_3410_5052: Do not use __uX types

From: Johan Hovold
Date: Fri Jul 15 2016 - 07:03:26 EST


On Thu, May 12, 2016 at 10:48:38AM +0200, Mathieu OTHACEHE wrote:
> __uX types should only be used for user-space interactions.
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@xxxxxxxxx>
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 66 ++++++++++++++++++-----------------
> 1 file changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index af4e145..164e07b 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -179,23 +179,23 @@
>
> /* Config struct */
> struct ti_uart_config {
> - __u16 wBaudRate;
> - __u16 wFlags;
> - __u8 bDataBits;
> - __u8 bParity;
> - __u8 bStopBits;
> + __be16 wBaudRate;
> + __be16 wFlags;

While I have nothing against replacing the __u<width>, this makes me
worried. How, if at all, is the endianess of these fields handled in the
code?

Ok, I see now it's using cpu_to_be16s after writing host-endian data
into it. Could you clean that to use cpu_to_be16 instead (you may need
to update an error message too)?

Note that the wBaudRate assignment was still using __u16 after this
patch.

> + u8 bDataBits;
> + u8 bParity;
> + u8 bStopBits;
> char cXon;
> char cXoff;
> - __u8 bUartMode;
> + u8 bUartMode;
> } __packed;

> static int ti_write_byte(struct usb_serial_port *port,
> - struct ti_device *tdev, unsigned long addr,
> - __u8 mask, __u8 byte)
> + struct ti_device *tdev, unsigned long addr,
> + u8 mask, u8 byte)
> {
> int status;
> unsigned int size;
> @@ -1659,10 +1661,10 @@ static int ti_do_download(struct usb_device *dev, int pipe,
> int len;
>
> for (pos = sizeof(struct ti_firmware_header); pos < size; pos++)
> - cs = (__u8)(cs + buffer[pos]);
> + cs = (u8)(cs + buffer[pos]);
>
> header = (struct ti_firmware_header *)buffer;
> - header->wLength = cpu_to_le16((__u16)(size
> + header->wLength = cpu_to_le16((u16)(size
> - sizeof(struct ti_firmware_header)));

Cast not needed.

Thanks,
Johan