Re: [PATCH 2/4] usb-serial: chunk data to wMaxPacketSize

[Samuel Thibault]

Jason Andryuk, le jeu. 12 mars 2020 08:55:21 -0400, a ecrit:
> usb-serial has issues with xHCI controllers where data is lost in the
> VM.  Inspecting the URBs in the guest, EHCI starts every 64 byte boundary
> (wMaxPacketSize) with a header.  EHCI hands packets into
> usb_serial_token_in() with size 64, so these cannot cross the 64 byte
> boundary.  The xHCI controller has packets of 512 bytes and the usb-serial
> will just write through the 64 byte boundary.  In the guest, this means
> data bytes are interpreted as header, so data bytes don't make it out
> the serial interface.
> 
> Re-work usb_serial_token_in to chunk data into 64 byte units - 2 byte
> header and 62 bytes data.  The Linux driver reads wMaxPacketSize to find
> the chunk size, so we match that.
> 
> Real hardware was observed to pass in 512 byte URBs (496 bytes data +
> 8 * 2 byte headers).  Since usb-serial only buffers 384 bytes of data,
> usb-serial will pass in 6 64 byte blocks and 1 12 byte partial block for
> 462 bytes max.
> 
> Signed-off-by: Jason Andryuk <address@hidden>

Reviewed-by: Samuel Thibault <address@hidden>

> ---
>  hw/usb/dev-serial.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 71fa786bd8..96b6c34202 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -360,15 +360,16 @@ static void usb_serial_handle_control(USBDevice *dev, 
> USBPacket *p,
>  
>  static void usb_serial_token_in(USBSerialState *s, USBPacket *p)
>  {
> -    int first_len, len;
> +    const int max_packet_size = desc_iface0.eps[0].wMaxPacketSize;
> +    int packet_len;
>      uint8_t header[2];
>  
> -    first_len = RECV_BUF - s->recv_ptr;
> -    len = p->iov.size;
> -    if (len <= 2) {
> +    packet_len = p->iov.size;
> +    if (packet_len <= 2) {
>          p->status = USB_RET_NAK;
>          return;
>      }
> +
>      header[0] = usb_get_modem_lines(s) | 1;
>      /* We do not have the uart details */
>      /* handle serial break */
> @@ -380,21 +381,31 @@ static void usb_serial_token_in(USBSerialState *s, 
> USBPacket *p)
>      } else {
>          header[1] = 0;
>      }
> -    len -= 2;
> -    if (len > s->recv_used)
> -        len = s->recv_used;
> -    if (!len) {
> +
> +    if (!s->recv_used) {
>          p->status = USB_RET_NAK;
>          return;
>      }
> -    if (first_len > len)
> -        first_len = len;
> -    usb_packet_copy(p, header, 2);
> -    usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
> -    if (len > first_len)
> -        usb_packet_copy(p, s->recv_buf, len - first_len);
> -    s->recv_used -= len;
> -    s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
> +
> +    while (s->recv_used && packet_len > 2) {
> +        int first_len, len;
> +
> +        len = MIN(packet_len, max_packet_size);
> +        len -= 2;
> +        if (len > s->recv_used)
> +            len = s->recv_used;
> +
> +        first_len = RECV_BUF - s->recv_ptr;
> +        if (first_len > len)
> +            first_len = len;
> +        usb_packet_copy(p, header, 2);
> +        usb_packet_copy(p, s->recv_buf + s->recv_ptr, first_len);
> +        if (len > first_len)
> +            usb_packet_copy(p, s->recv_buf, len - first_len);
> +        s->recv_used -= len;
> +        s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;
> +        packet_len -= len + 2;
> +    }
>  
>      return;
>  }
> -- 
> 2.24.1
> 

-- 
Samuel
We are Pentium of Borg. Division is futile. You will be approximated.
(seen in someone's .signature)