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

[Anthony Liguori <anthony@codemonkey.ws>]

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.

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);
>