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

2011-02-03 Thread Anthony Liguori 

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 Levyal...@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_tslot; // 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 Thibaultsamuel.thiba...@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__);

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

2011-02-03 Thread Alon Levy 
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 Levyal...@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_tslot; // 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 Thibaultsamuel.thiba...@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

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

2011-02-02 Thread Alon Levy 
I'll fold it before submitting the version to be applied, but
I hope keeping it as a separate patch will make reviewing easier.

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 al...@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_tslot; // 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.thiba...@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