From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33991 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pl2Qw-00028u-Kt for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:55:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pl2Ln-0005qx-FM for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:48:09 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:40213) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pl2Ln-0005qo-7v for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:48:07 -0500 Received: by iye19 with SMTP id 19so1324836iye.4 for ; Thu, 03 Feb 2011 08:48:06 -0800 (PST) Message-ID: <4D4ADC43.90709@codemonkey.ws> Date: Thu, 03 Feb 2011 10:48:03 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 06/20] usb-ccid: review fixes (v15-v16) References: <1296678500-19497-1-git-send-email-alevy@redhat.com> <1296678500-19497-7-git-send-email-alevy@redhat.com> In-Reply-To: <1296678500-19497-7-git-send-email-alevy@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: qemu-devel@nongnu.org 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 > --- > 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 > * 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); >