Re: [Qemu-devel] [PATCH 06/20] usb-ccid: review fixes (v15-v16)

[Alon Levy <alevy@redhat.com>]

On Thu, Feb 03, 2011 at 10:48:03AM -0600, Anthony Liguori wrote:
> On 02/02/2011 02:28 PM, Alon Levy wrote:
> >I'll fold it before submitting the version to be applied, but
> >I hope keeping it as a separate patch will make reviewing easier.
> 
> Hrm, can you just send out the new patches?   It's actually harder
> to review like this.

Yes.

> 
> Regards,
> 
> Anthony Liguori
> 
> >Behavioral changes:
> >  * fix abort on client answer after card remove
> >  * enable migration
> >  * remove side affect code from asserts
> >  * return consistent self-powered state
> >  * mask out reserved bits in ccid_set_parameters
> >  * add missing abRFU in SetParameters (no affect on linux guest)
> >
> >whitefixes / comments / consts defines:
> >  * remove stale comment
> >  * remove ccid_print_pending_answers if no DEBUG_CCID
> >  * replace printf's with DPRINTF, remove DEBUG_CCID, add verbosity defines
> >  * use error_report
> >  * update copyright (most of the code is not original)
> >  * reword known bug comment
> >  * add missing closing quote in comment
> >  * add missing whitespace on one line
> >  * s/CCID_SetParameter/CCID_SetParameters/
> >  * add comments
> >  * use define for max packet size
> >
> >Comment for "return consistent self-powered state":
> >
> >the Configuration Descriptor bmAttributes claims we are self powered,
> >but we were returning not self powered to USB_REQ_GET_STATUS control message.
> >
> >In practice, this message is not sent by a linux 2.6.35.10-74.fc14.x86_64
> >guest (not tested on other guests), unless you issue lsusb -v as root (for
> >example).
> >
> >Signed-off-by: Alon Levy<alevy@redhat.com>
> >---
> >  hw/ccid.h     |    7 +++
> >  hw/usb-ccid.c |  115 ++++++++++++++++++++++++++++-----------------------------
> >  2 files changed, 63 insertions(+), 59 deletions(-)
> >
> >diff --git a/hw/ccid.h b/hw/ccid.h
> >index af59070..f5dcfae 100644
> >--- a/hw/ccid.h
> >+++ b/hw/ccid.h
> >@@ -6,11 +6,16 @@
> >  typedef struct CCIDCardState CCIDCardState;
> >  typedef struct CCIDCardInfo CCIDCardInfo;
> >
> >+/* state of the CCID Card device (i.e. hw/ccid-card-*.c)
> >+ */
> >  struct CCIDCardState {
> >      DeviceState qdev;
> >      uint32_t    slot; // For future use with multiple slot reader.
> >  };
> >
> >+/* callbacks to be used by the CCID device (hw/usb-ccid.c) to call
> >+ * into the smartcard device (hw/ccid-card-*.c)
> >+ */
> >  struct CCIDCardInfo {
> >      DeviceInfo qdev;
> >      void (*print)(Monitor *mon, CCIDCardState *card, int indent);
> >@@ -20,6 +25,8 @@ struct CCIDCardInfo {
> >      int (*initfn)(CCIDCardState *card);
> >  };
> >
> >+/* API for smartcard calling the CCID device (used by hw/ccid-card-*.c)
> >+ */
> >  void ccid_card_send_apdu_to_guest(CCIDCardState *card, uint8_t* apdu, uint32_t len);
> >  void ccid_card_card_removed(CCIDCardState *card);
> >  void ccid_card_card_inserted(CCIDCardState *card);
> >diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
> >index 58f69a6..09e39ac 100644
> >--- a/hw/usb-ccid.c
> >+++ b/hw/usb-ccid.c
> >@@ -1,15 +1,18 @@
> >  /*
> >   * CCID Device emulation
> >   *
> >- * Based on usb-serial.c:
> >+ * Written by Alon Levy, with contributions from Robert Relyea.
> >+ *
> >+ * Based on usb-serial.c, see it's copyright and attributions below.
> >+ *
> >+ * This code is licenced under the GNU LGPL, version 2 or later.
> >+ *
> >+ * -------
> >+ *
> >+ * usb-serial.c copyright and attribution:
> >   * Copyright (c) 2006 CodeSourcery.
> >   * Copyright (c) 2008 Samuel Thibault<samuel.thibault@ens-lyon.org>
> >   * Written by Paul Brook, reused for FTDI by Samuel Thibault,
> >- * Reused for CCID by Alon Levy.
> >- * Contributed to by Robert Relyea
> >- * Copyright (c) 2010 Red Hat.
> >- *
> >- * This code is licenced under the LGPL.
> >   */
> >
> >  /* References:
> >@@ -19,22 +22,16 @@
> >   *  Specification for Integrated Circuit(s) Cards Interface Devices
> >   *
> >   * Endianess note: from the spec (1.3)
> >- *  "Fields that are larger than a byte are stored in little endian
> >+ *  "Fields that are larger than a byte are stored in little endian"
> >   *
> >   * KNOWN BUGS
> >   * 1. remove/insert can sometimes result in removed state instead of inserted.
> >   * This is a result of the following:
> >- *  symptom: dmesg shows ERMOTEIO (-121), pcscd shows -99. This happens
> >- *  when we send a too short packet, seen in uhci-usb.c, resulting from
> >- *  a urb requesting SPD and us returning a smaller packet.
> >+ *  symptom: dmesg shows ERMOTEIO (-121), pcscd shows -99. This can happen
> >+ *  when a short packet is sent, as seen in uhci-usb.c, resulting from a urb
> >+ *  from the guest requesting SPD and us returning a smaller packet.
> >   *  Not sure which messages trigger this.
> >   *
> >- * Migration note:
> >- *
> >- * All the VMStateDescription's are left here for future use, but
> >- * not enabled right now since there is no support for USB migration.
> >- *
> >- * To enable define ENABLE_MIGRATION
> >   */
> >
> >  #include "qemu-common.h"
> >@@ -44,11 +41,14 @@
> >
> >  #include "hw/ccid.h"
> >
> >-//#define DEBUG_CCID
> >-
> >  #define DPRINTF(s, lvl, fmt, ...) \
> >  do { if (lvl<= s->debug) { printf("usb-ccid: " fmt , ## __VA_ARGS__); } } while (0)
> >
> >+#define D_WARN 1
> >+#define D_INFO 2
> >+#define D_MORE_INFO 3
> >+#define D_VERBOSE 4
> >+
> >  #define CCID_DEV_NAME "usb-ccid"
> >
> >  /* The two options for variable sized buffers:
> >@@ -64,6 +64,8 @@ do { if (lvl<= s->debug) { printf("usb-ccid: " fmt , ## __VA_ARGS__); } } while
> >  #define InterfaceOutClass    ((USB_DIR_OUT|USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
> >  #define InterfaceInClass     ((USB_DIR_IN |USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
> >
> >+#define CCID_MAX_PACKET_SIZE                64
> >+
> >  #define CCID_CONTROL_ABORT                  0x1
> >  #define CCID_CONTROL_GET_CLOCK_FREQUENCIES  0x2
> >  #define CCID_CONTROL_GET_DATA_RATES         0x3
> >@@ -209,11 +211,12 @@ typedef struct __attribute__ ((__packed__)) CCID_IccPowerOff {
> >      uint16_t    abRFU;
> >  } CCID_IccPowerOff;
> >
> >-typedef struct __attribute__ ((__packed__)) CCID_SetParameter {
> >+typedef struct __attribute__ ((__packed__)) CCID_SetParameters {
> >      CCID_Header hdr;
> >      uint8_t     bProtocolNum;
> >+    uint16_t   abRFU;
> >      uint8_t    abProtocolDataStructure[0];
> >-} CCID_SetParameter;
> >+} CCID_SetParameters;
> >
> >  typedef struct CCID_Notify_Slot_Change {
> >      uint8_t     bMessageType; /* CCID_MESSAGE_TYPE_RDR_to_PC_NotifySlotChange */
> >@@ -278,10 +281,6 @@ struct USBCCIDState {
> >      uint16_t migration_target_port;
> >  };
> >
> >-/* Slot specific variables. We emulate a single slot card reader.
> >- */
> >-
> >-
> >  /* CCID Spec chapter 4: CCID uses a standard device descriptor per Chapter 9,
> >   * "USB Device Framework", section 9.6.1, in the Universal Serial Bus
> >   * Specification.
> >@@ -416,7 +415,8 @@ static const uint8_t qemu_ccid_config_descriptor[] = {
> >                      /*  u8  ep_bEndpointAddress; IN Endpoint 1 */
> >          0x80 | CCID_INT_IN_EP,
> >          0x03,       /*  u8  ep_bmAttributes; Interrupt */
> >-        0x40, 0x00, /*  u16 ep_wMaxPacketSize; */
> >+                    /*  u16 ep_wMaxPacketSize; */
> >+        CCID_MAX_PACKET_SIZE&  0xff, (CCID_MAX_PACKET_SIZE>>  8),
> >          0xff,       /*  u8  ep_bInterval; */
> >
> >          /* Bulk-In endpoint */
> >@@ -455,32 +455,31 @@ static void ccid_clear_pending_answers(USBCCIDState *s)
> >
> >  static void ccid_print_pending_answers(USBCCIDState *s)
> >  {
> >-#ifdef DEBUG_CCID
> >      Answer *answer;
> >      int i, count;
> >
> >-    printf("usb-ccid: pending answers:");
> >+    DPRINTF(s, D_VERBOSE, "usb-ccid: pending answers:");
> >      if (!ccid_has_pending_answers(s)) {
> >-        printf(" empty\n");
> >+        DPRINTF(s, D_VERBOSE, " empty\n");
> >          return;
> >      }
> >      for (i = s->pending_answers_start, count=s->pending_answers_num ;
> >           count>  0; count--, i++) {
> >          answer =&s->pending_answers[i % PENDING_ANSWERS_NUM];
> >          if (count == 1) {
> >-            printf("%d:%d\n", answer->slot, answer->seq);
> >+            DPRINTF(s, D_VERBOSE, "%d:%d\n", answer->slot, answer->seq);
> >          } else {
> >-            printf("%d:%d,", answer->slot, answer->seq);
> >+            DPRINTF(s, D_VERBOSE, "%d:%d,", answer->slot, answer->seq);
> >          }
> >      }
> >-#endif
> >  }
> >
> >  static void ccid_add_pending_answer(USBCCIDState *s, CCID_Header *hdr)
> >  {
> >      Answer* answer;
> >
> >-    assert(s->pending_answers_num++<  PENDING_ANSWERS_NUM);
> >+    assert(s->pending_answers_num<  PENDING_ANSWERS_NUM);
> >+    s->pending_answers_num++;
> >      answer =&s->pending_answers[(s->pending_answers_end++) % PENDING_ANSWERS_NUM];
> >      answer->slot = hdr->bSlot;
> >      answer->seq = hdr->bSeq;
> >@@ -492,7 +491,8 @@ static void ccid_remove_pending_answer(USBCCIDState *s,
> >  {
> >      Answer *answer;
> >
> >-    assert(s->pending_answers_num-->  0);
> >+    assert(s->pending_answers_num>  0);
> >+    s->pending_answers_num--;
> >      answer =&s->pending_answers[(s->pending_answers_start++) % PENDING_ANSWERS_NUM];
> >      *slot = answer->slot;
> >      *seq = answer->seq;
> >@@ -528,16 +528,16 @@ static void* ccid_reserve_recv_buf(USBCCIDState* s, uint16_t len)
> >  {
> >      BulkIn* bulk_in;
> >
> >-    DPRINTF(s, 4, "%s: QUEUE: reserve %d bytes\n", __func__, len);
> >+    DPRINTF(s, D_VERBOSE, "%s: QUEUE: reserve %d bytes\n", __func__, len);
> >
> >      /* look for an existing element */
> >      if (len>  BULK_IN_BUF_SIZE) {
> >-        printf("usb-ccid.c: %s: len larger then max (%d>%d). discarding message.\n",
> >+        DPRINTF(s, D_WARN, "usb-ccid.c: %s: len larger then max (%d>%d). discarding message.\n",
> >              __func__, len, BULK_IN_BUF_SIZE);
> >          return NULL;
> >      }
> >      if (s->bulk_in_pending_num>= BULK_IN_PENDING_NUM) {
> >-        printf("usb-ccid.c: %s: No free bulk_in buffers. discarding message.\n",
> >+        DPRINTF(s, D_WARN, "usb-ccid.c: %s: No free bulk_in buffers. discarding message.\n",
> >                  __func__);
> >          return NULL;
> >      }
> >@@ -576,7 +576,7 @@ static int ccid_handle_control(USBDevice *dev, int request, int value,
> >      DPRINTF(s, 1, "got control %x, value %x\n",request, value);
> >      switch (request) {
> >      case DeviceRequest | USB_REQ_GET_STATUS:
> >-        data[0] = (0<<  USB_DEVICE_SELF_POWERED) |
> >+        data[0] = (1<<  USB_DEVICE_SELF_POWERED) |
> >              (dev->remote_wakeup<<  USB_DEVICE_REMOTE_WAKEUP);
> >          data[1] = 0x00;
> >          ret = 2;
> >@@ -709,7 +709,7 @@ static uint8_t ccid_calc_status(USBCCIDState *s)
> >         bmCommandStatus
> >       */
> >      uint8_t ret = ccid_card_status(s) | (s->bmCommandStatus<<  6);
> >-    DPRINTF(s, 4, "status = %d\n", ret);
> >+    DPRINTF(s, D_VERBOSE, "status = %d\n", ret);
> >      return ret;
> >  }
> >
> >@@ -770,11 +770,9 @@ static void ccid_write_data_block(
> >      p->b.hdr.bSeq = seq;
> >      p->b.bStatus = ccid_calc_status(s);
> >      p->b.bError = s->bError;
> >-#ifdef DEBUG_CCID
> >      if (p->b.bError) {
> >-        DPRINTF(s, 4, "error %d", p->b.bError);
> >+        DPRINTF(s, D_VERBOSE, "error %d", p->b.bError);
> >      }
> >-#endif
> >      memcpy(p->abData, data, len);
> >      ccid_reset_error_status(s);
> >  }
> >@@ -805,12 +803,12 @@ static void ccid_write_data_block_atr(USBCCIDState* s, CCID_Header* recv)
> >
> >  static void ccid_set_parameters(USBCCIDState *s, CCID_Header *recv)
> >  {
> >-    CCID_SetParameter *ph = (CCID_SetParameter *) recv;
> >+    CCID_SetParameters *ph = (CCID_SetParameters *) recv;
> >      uint32_t len = 0;
> >-    if (ph->bProtocolNum == 0) {
> >+    if ((ph->bProtocolNum&  3) == 0) {
> >          len = 5;
> >      }
> >-    if (ph->bProtocolNum == 1) {
> >+    if ((ph->bProtocolNum&  3) == 1) {
> >          len = 7;
> >      }
> >      if (len == 0) {
> >@@ -884,7 +882,7 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, CCID_XferBlock *recv)
> >      if (s->card) {
> >          s->cardinfo->apdu_from_guest(s->card, recv->abData, len);
> >      } else {
> >-        printf("warning: discarded apdu\n");
> >+        DPRINTF(s, D_WARN, "warning: discarded apdu\n");
> >      }
> >  }
> >
> >@@ -901,15 +899,15 @@ static int ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
> >      ccid_header = (CCID_Header*)s->bulk_out_data;
> >      memcpy(s->bulk_out_data + s->bulk_out_pos, p->data, p->len);
> >      s->bulk_out_pos += p->len;
> >-    if (p->len == 64) {
> >-        DPRINTF(s, 4, "usb-ccid: bulk_in: expecting more packets (%d/%d)\n",
> >+    if (p->len == CCID_MAX_PACKET_SIZE) {
> >+        DPRINTF(s, D_VERBOSE, "usb-ccid: bulk_in: expecting more packets (%d/%d)\n",
> >              p->len, ccid_header->dwLength);
> >          return 0;
> >      }
> >      if (s->bulk_out_pos<  10) {
> >          DPRINTF(s, 1, "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n", __func__);
> >      } else {
> >-        DPRINTF(s, 3, "%s %x\n", __func__, ccid_header->bMessageType);
> >+        DPRINTF(s, D_MORE_INFO, "%s %x\n", __func__, ccid_header->bMessageType);
> >          switch (ccid_header->bMessageType) {
> >              case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus:
> >                  ccid_write_slot_status(s, ccid_header);
> >@@ -965,7 +963,7 @@ static int ccid_bulk_in_copy_to_guest(USBCCIDState *s, uint8_t *data, int len)
> >  {
> >      int ret = 0;
> >
> >-    assert(len>0);
> >+    assert(len>  0);
> >      ccid_bulk_in_get(s);
> >      if (s->current_bulk_in != NULL) {
> >          ret = MIN(s->current_bulk_in->len - s->current_bulk_in->pos, len);
> >@@ -978,7 +976,7 @@ static int ccid_bulk_in_copy_to_guest(USBCCIDState *s, uint8_t *data, int len)
> >          ret = USB_RET_NAK; /* return when device has no data - usb 2.0 spec Table 8-4 */
> >      }
> >      if (ret>  0) {
> >-        DPRINTF(s, 3, "%s: %d/%d req/act to guest (BULK_IN)\n", __func__, len, ret);
> >+        DPRINTF(s, D_MORE_INFO, "%s: %d/%d req/act to guest (BULK_IN)\n", __func__, len, ret);
> >      }
> >      if (ret != USB_RET_NAK&&  ret<  len) {
> >          DPRINTF(s, 1, "%s: returning short (EREMOTEIO) %d<  %d\n", __func__, ret, len);
> >@@ -1015,7 +1013,7 @@ static int ccid_handle_data(USBDevice *dev, USBPacket *p)
> >                      ret = 2;
> >                      s->notify_slot_change = false;
> >                      s->bmSlotICCState&= ~SLOT_0_CHANGED_MASK;
> >-                    DPRINTF(s, 2, "handle_data: int_in: notify_slot_change %X, requested len %d\n",
> >+                    DPRINTF(s, D_INFO, "handle_data: int_in: notify_slot_change %X, requested len %d\n",
> >                              s->bmSlotICCState, len);
> >                  }
> >                  break;
> >@@ -1146,7 +1144,11 @@ void ccid_card_card_error(CCIDCardState *card, uint64_t error)
> >      DPRINTF(s, 1, "VSC_Error: %lX\n", s->last_answer_error);
> >      /* TODO: these error's should be more verbose and propogated to the guest.
> >       * */
> >-    ccid_write_data_block_answer(s, NULL, 0);
> >+    /* we flush all pending answers on CardRemove message in ccid-card-passthru,
> >+     * so check that first to not trigger abort */
> >+    if (ccid_has_pending_answers(s)) {
> >+        ccid_write_data_block_answer(s, NULL, 0);
> >+    }
> >  }
> >
> >  void ccid_card_card_inserted(CCIDCardState *card)
> >@@ -1184,12 +1186,12 @@ static int ccid_card_init(DeviceState *qdev, DeviceInfo *base)
> >      int ret = 0;
> >
> >      if (card->slot != 0) {
> >-        fprintf(stderr, "Warning: usb-ccid supports one slot, can't add %d",
> >+        error_report("Warning: usb-ccid supports one slot, can't add %d",
> >                  card->slot);
> >          return -1;
> >      }
> >      if (s->card != NULL) {
> >-        fprintf(stderr, "Warning: usb-ccid card already full, not adding\n");
> >+        error_report("Warning: usb-ccid card already full, not adding\n");
> >          return -1;
> >      }
> >      ret = info->initfn ? info->initfn(card) : ret;
> >@@ -1233,7 +1235,6 @@ static int ccid_initfn(USBDevice *dev)
> >      return 0;
> >  }
> >
> >-#ifdef ENABLE_MIGRATION
> >  static int ccid_post_load(void *opaque, int version_id)
> >  {
> >      USBCCIDState *s = opaque;
> >@@ -1325,7 +1326,6 @@ static VMStateDescription ccid_vmstate = {
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> >-#endif // ENABLE_MIGRATION
> >
> >  static struct USBDeviceInfo ccid_info = {
> >      .product_desc   = "QEMU USB CCID",
> >@@ -1342,12 +1342,9 @@ static struct USBDeviceInfo ccid_info = {
> >          DEFINE_PROP_UINT8("debug", USBCCIDState, debug, 0),
> >          DEFINE_PROP_END_OF_LIST(),
> >      },
> >-#ifdef ENABLE_MIGRATION
> >      .qdev.vmsd      =&ccid_vmstate,
> >-#endif
> >  };
> >
> >-
> >  static void ccid_register_devices(void)
> >  {
> >      usb_qdev_register(&ccid_info);
>