Re: [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue

[Paolo Bonzini]

Il 06/11/2012 15:08, Hans de Goede ha scritto:
> Since we handle interrupt out async, and not buffered like iso-out, there is
> no need for a separate status flag, instead store any reported error status
> into the bufp queue.
> 
> Signed-off-by: Hans de Goede <address@hidden>
> ---
>  hw/usb/redirect.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 9052ef8..b61bb6e 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -58,11 +58,11 @@ struct endp_data {
>      uint8_t type;
>      uint8_t interval;
>      uint8_t interface; /* bInterfaceNumber this ep belongs to */
> +    uint8_t zero; /* Was interrupt_error, now always 0 for migration compat 
> */

No need for this, use VMSTATE_UNUSED.

But if you do this, you need to bump the migration version because the
old version will not be able to retrieve the interrupt_error value.  Can
you somewhat figure out it in the pre_load callback instead?

Hmm... well, migration from usbredir is new in 1.3, so you do have time
to change the protocol.  You do not need zero, you do not need
VMSTATE_UNUSED.  But you'd better do this change as well in a certain
downstream you care about, otherwise you'll have the same problem soon.

Paolo

>      uint16_t max_packet_size; /* In bytes, not wMaxPacketSize format !! */
>      uint8_t iso_started;
>      uint8_t iso_error; /* For reporting iso errors to the HC */
>      uint8_t interrupt_started;
> -    uint8_t interrupt_error;
>      uint8_t bufpq_prefilled;
>      uint8_t bufpq_dropping_packets;
>      QTAILQ_HEAD(, buf_packet) bufpq;
> @@ -626,7 +626,7 @@ static void 
> usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
>      int status, len;
>  
>      if (!dev->endpoint[EP2I(ep)].interrupt_started &&
> -            !dev->endpoint[EP2I(ep)].interrupt_error) {
> +            QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq)) {
>          struct usb_redir_start_interrupt_receiving_header start_int = {
>              .endpoint = ep,
>          };
> @@ -645,14 +645,7 @@ static void 
> usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
>      intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
>      if (intp == NULL) {
>          DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
> -        /* Check interrupt_error for stream errors */
> -        status = dev->endpoint[EP2I(ep)].interrupt_error;
> -        dev->endpoint[EP2I(ep)].interrupt_error = 0;
> -        if (status) {
> -            usbredir_handle_status(dev, p, status);
> -        } else {
> -            p->status = USB_RET_NAK;
> -        }
> +        p->status = USB_RET_NAK;
>          return;
>      }
>      DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
> @@ -708,7 +701,6 @@ static void 
> usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
>          DPRINTF("interrupt recv stopped ep %02X\n", ep);
>          dev->endpoint[EP2I(ep)].interrupt_started = 0;
>      }
> -    dev->endpoint[EP2I(ep)].interrupt_error = 0;
>      usbredir_free_bufpq(dev, ep);
>  }
>  
> @@ -1513,20 +1505,22 @@ static void usbredir_interrupt_receiving_status(void 
> *priv, uint64_t id,
>  {
>      USBRedirDevice *dev = priv;
>      uint8_t ep = interrupt_receiving_status->endpoint;
> +    uint8_t status = interrupt_receiving_status->status;
>  
>      DPRINTF("interrupt recv status %d ep %02X id %"PRIu64"\n",
>              interrupt_receiving_status->status, ep, id);
>  
> -    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started) {
> +    if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started ||
> +            status == usb_redir_success) {
>          return;
>      }
>  
> -    dev->endpoint[EP2I(ep)].interrupt_error =
> -        interrupt_receiving_status->status;
>      if (interrupt_receiving_status->status == usb_redir_stall) {
>          DPRINTF("interrupt receiving stopped by peer ep %02X\n", ep);
>          dev->endpoint[EP2I(ep)].interrupt_started = 0;
> +        status = usb_redir_ioerror;
>      }
> +    bufp_alloc(dev, NULL, 0, status, ep);
>  }
>  
>  static void usbredir_bulk_streams_status(void *priv, uint64_t id,
> @@ -1833,7 +1827,7 @@ static const VMStateDescription usbredir_ep_vmstate = {
>          VMSTATE_UINT8(iso_started, struct endp_data),
>          VMSTATE_UINT8(iso_error, struct endp_data),
>          VMSTATE_UINT8(interrupt_started, struct endp_data),
> -        VMSTATE_UINT8(interrupt_error, struct endp_data),
> +        VMSTATE_UINT8(zero, struct endp_data), /* Was interrupt_error */
>          VMSTATE_UINT8(bufpq_prefilled, struct endp_data),
>          VMSTATE_UINT8(bufpq_dropping_packets, struct endp_data),
>          {
>