scd: Improve watching USB device removal.

* scd/apdu.c(struct reader_table_s): Add require_get_status.
(apdu_connect): Change return value meaning.  Call apdu_reset here.
* scd/app.c (app_new_register): Add require_get_status.
(select_application): Use the return value of apdu_connect.
(scd_update_reader_status_file): Call update_fdset_for_usb with
checking all_have_intr_endp.
(app_list_start, app_list_finish): Remove.
* scd/ccid-driver.c (struct ccid_driver_s): Add transfer.
(intr_cb): Don't call libusb_transfer in this callback.
(ccid_require_get_status): New.
(do_close_reader): Call libusb_transfer here.
* scd/scdaemon.c (update_fdset_for_usb): Remove the first argument.

--

With Gnuk Token, it works fine as expected.  With Gemalto reader,
intr_cb is not called when card is removed.  So, the macro
LIBUSB_WORKS_EXPECTED_FOR_INTERRUPT_ENDP is not defined yet.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>

8 files changed, 127 insertions, 106 deletions

diff --git a/scd/apdu.c b/scd/apdu.c
index f88d97035..0037c476c 100644
--- a/scd/apdu.c
+++ b/scd/apdu.c
@@ -141,11 +141,12 @@ struct reader_table_s {
   } rapdu;
 #endif /*USE_G10CODE_RAPDU*/
   char *rdrname;     /* Name of the connected reader or NULL if unknown. */
-  int is_t0;         /* True if we know that we are running T=0. */
-  int is_spr532;     /* True if we know that the reader is a SPR532.  */
-  int pinpad_varlen_supported;  /* True if we know that the reader
-                                   supports variable length pinpad
-                                   input.  */
+  unsigned int is_t0:1;     /* True if we know that we are running T=0. */
+  unsigned int is_spr532:1; /* True if we know that the reader is a SPR532.  */
+  unsigned int pinpad_varlen_supported:1;  /* True if we know that the reader
+                                              supports variable length pinpad
+                                              input.  */
+  unsigned int require_get_status:1;
   unsigned char atr[33];
   size_t atrlen;           /* A zero length indicates that the ATR has
                               not yet been read; i.e. the card is not
@@ -470,6 +471,7 @@ new_reader_slot (void)
   reader_table[reader].is_t0 = 1;
   reader_table[reader].is_spr532 = 0;
   reader_table[reader].pinpad_varlen_supported = 0;
+  reader_table[reader].require_get_status = 1;
 #ifdef NEED_PCSC_WRAPPER
   reader_table[reader].pcsc.req_fd = -1;
   reader_table[reader].pcsc.rsp_fd = -1;
@@ -2572,6 +2574,7 @@ open_ccid_reader (struct dev_list *dl)
 {
   int err;
   int slot;
+  int require_get_status;
   reader_table_t slotp;
 
   slot = new_reader_slot ();
@@ -2596,6 +2599,8 @@ open_ccid_reader (struct dev_list *dl)
       err = 0;
     }
 
+  require_get_status = ccid_require_get_status (slotp->ccid.handle);
+
   reader_table[slot].close_reader = close_ccid_reader;
   reader_table[slot].reset_reader = reset_ccid_reader;
   reader_table[slot].get_status_reader = get_status_ccid;
@@ -2608,6 +2613,7 @@ open_ccid_reader (struct dev_list *dl)
   /* Our CCID reader code does not support T=0 at all, thus reset the
      flag.  */
   reader_table[slot].is_t0 = 0;
+  reader_table[slot].require_get_status = require_get_status;
 
   dump_reader_status (slot);
   unlock_slot (slot);
@@ -2970,22 +2976,15 @@ apdu_dev_list_start (const char *portstr, struct dev_list **l_p)
   return 0;
 }
 
-int
+void
 apdu_dev_list_finish (struct dev_list *dl)
 {
-  int all_have_intr_endp;
-
 #ifdef HAVE_LIBUSB
   if (dl->ccid_table)
-    all_have_intr_endp = ccid_dev_scan_finish (dl->ccid_table, dl->idx_max);
-  else
-    all_have_intr_endp = 0;
-#else
-  all_have_intr_endp = 0;
+    ccid_dev_scan_finish (dl->ccid_table, dl->idx_max);
 #endif
   xfree (dl);
   npth_mutex_unlock (&reader_table_lock);
-  return all_have_intr_endp;
 }
 
 
@@ -3347,8 +3346,11 @@ apdu_enum_reader (int slot, int *used)
 
 /* Connect a card.  This is used to power up the card and make sure
    that an ATR is available.  Depending on the reader backend it may
-   return an error for an inactive card or if no card is
-   available.  */
+   return an error for an inactive card or if no card is available.
+   Return -1 on error.  Return 1 if reader requires get_status to
+   watch card removal.  Return 0 if it's a token (always with a card),
+   or it supports INTERRUPT endpoint to watch card removal.
+  */
 int
 apdu_connect (int slot)
 {
@@ -3362,7 +3364,7 @@ apdu_connect (int slot)
     {
       if (DBG_READER)
         log_debug ("leave: apdu_connect => SW_HOST_NO_DRIVER\n");
-      return SW_HOST_NO_DRIVER;
+      return -1;
     }
 
   /* Only if the access method provides a connect function we use it.
@@ -3393,10 +3395,19 @@ apdu_connect (int slot)
   else if ((status & APDU_CARD_PRESENT) && !(status & APDU_CARD_ACTIVE))
     sw = SW_HOST_CARD_INACTIVE;
 
+  if (sw == SW_HOST_CARD_INACTIVE)
+    {
+      /* Try power it up again.  */
+      sw = apdu_reset (slot);
+    }
+
   if (DBG_READER)
     log_debug ("leave: apdu_connect => sw=0x%x\n", sw);
 
-  return sw;
+  if (sw)
+    return -1;
+
+  return reader_table[slot].require_get_status;
 }
 
 
diff --git a/scd/apdu.h b/scd/apdu.h
index d71c8dd5d..473def518 100644
--- a/scd/apdu.h
+++ b/scd/apdu.h
@@ -88,7 +88,7 @@ struct dev_list;
 gpg_error_t apdu_init (void);
 
 gpg_error_t apdu_dev_list_start (const char *portstr, struct dev_list **l_p);
-int apdu_dev_list_finish (struct dev_list *l);
+void apdu_dev_list_finish (struct dev_list *l);
 
 /* Note, that apdu_open_reader returns no status word but -1 on error. */
 int apdu_open_reader (struct dev_list *l);
diff --git a/scd/app-common.h b/scd/app-common.h
index b979f5476..521fcf329 100644
--- a/scd/app-common.h
+++ b/scd/app-common.h
@@ -121,8 +121,6 @@ size_t app_help_read_length_of_cert (int slot, int fid, size_t *r_certoff);
 
 
 /*-- app.c --*/
-app_t app_list_start (void);
-void app_list_finish (void);
 void app_send_card_list (ctrl_t ctrl);
 char *app_get_serialno (app_t app);
 
diff --git a/scd/app.c b/scd/app.c
index daff0b7d7..3cf219cd9 100644
--- a/scd/app.c
+++ b/scd/app.c
@@ -174,7 +174,8 @@ app_reset (app_t app, ctrl_t ctrl, int send_reset)
 }
 
 static gpg_error_t
-app_new_register (int slot, ctrl_t ctrl, const char *name)
+app_new_register (int slot, ctrl_t ctrl, const char *name,
+                  int require_get_status)
 {
   gpg_error_t err = 0;
   app_t app = NULL;
@@ -303,7 +304,7 @@ app_new_register (int slot, ctrl_t ctrl, const char *name)
       return err;
     }
 
-  app->require_get_status = 1;   /* For token, this can be 0.  */
+  app->require_get_status = require_get_status;
 
   npth_mutex_lock (&app_list_lock);
   app->next = app_top;
@@ -330,7 +331,7 @@ select_application (ctrl_t ctrl, const char *name, app_t *r_app,
   if (scan || !app_top)
     {
       struct dev_list *l;
-      int all_have_intr_endp;
+      int all_have_intr_endp = 1;
 
       err = apdu_dev_list_start (opt.reader_port, &l);
       if (err)
@@ -339,39 +340,31 @@ select_application (ctrl_t ctrl, const char *name, app_t *r_app,
       while (1)
         {
           int slot;
-          int sw;
+          int require_get_status;
 
           slot = apdu_open_reader (l);
           if (slot < 0)
             break;
 
-          err = 0;
-          sw = apdu_connect (slot);
-
-          if (sw == SW_HOST_CARD_INACTIVE)
+          require_get_status = apdu_connect (slot);
+          if (require_get_status < 0)
             {
-              /* Try again.  */
-              sw = apdu_reset (slot);
+              /* We close a reader with no card.  */
+              err = gpg_error (GPG_ERR_ENODEV);
             }
-
-          if (!sw || sw == SW_HOST_ALREADY_CONNECTED)
-            err = 0;
-          else if (sw == SW_HOST_NO_CARD)
-            err = gpg_error (GPG_ERR_CARD_NOT_PRESENT);
-          else
-            err = gpg_error (GPG_ERR_ENODEV);
-
-          if (!err)
-            err = app_new_register (slot, ctrl, name);
           else
             {
-              /* We close a reader with no card.  */
-              apdu_close_reader (slot);
+              err = app_new_register (slot, ctrl, name, require_get_status);
+              if (require_get_status)
+                all_have_intr_endp = 0;
             }
+
+          if (err)
+            apdu_close_reader (slot);
         }
 
-      all_have_intr_endp = apdu_dev_list_finish (l);
-      update_fdset_for_usb (1, all_have_intr_endp);
+      apdu_dev_list_finish (l);
+      update_fdset_for_usb (all_have_intr_endp);
     }
 
   npth_mutex_lock (&app_list_lock);
@@ -1021,46 +1014,60 @@ void
 scd_update_reader_status_file (void)
 {
   app_t a, app_next;
+  int all_have_intr_endp = 1;
+  int removal_detected = 0;
 
   npth_mutex_lock (&app_list_lock);
   for (a = app_top; a; a = app_next)
     {
+      int sw;
+      unsigned int status;
+
+      sw = apdu_get_status (a->slot, 0, &status);
       app_next = a->next;
-      if (a->require_get_status)
+
+      if (sw == SW_HOST_NO_READER)
+        {
+          /* Most likely the _reader_ has been unplugged.  */
+          status = 0;
+        }
+      else if (sw)
+        {
+          /* Get status failed.  Ignore that.  */
+          if (a->require_get_status)
+            all_have_intr_endp = 0;
+          continue;
+        }
+
+      if (a->card_status != status)
         {
-          int sw;
-          unsigned int status;
-          sw = apdu_get_status (a->slot, 0, &status);
+          report_change (a->slot, a->card_status, status);
+          send_client_notifications (a, status == 0);
 
-          if (sw == SW_HOST_NO_READER)
-            {
-              /* Most likely the _reader_ has been unplugged.  */
-              status = 0;
-            }
-          else if (sw)
+          if (status == 0)
             {
-              /* Get status failed.  Ignore that.  */
-              continue;
+              log_debug ("Removal of a card: %d\n", a->slot);
+              apdu_close_reader (a->slot);
+              deallocate_app (a);
+              removal_detected = 1;
             }
-
-          if (a->card_status != status)
+          else
             {
-              report_change (a->slot, a->card_status, status);
-              send_client_notifications (a, status == 0);
-
-              if (status == 0)
-                {
-                  log_debug ("Removal of a card: %d\n", a->slot);
-                  apdu_close_reader (a->slot);
-                  deallocate_app (a);
-                  update_fdset_for_usb (0, 0);
-                }
-              else
-                a->card_status = status;
+              a->card_status = status;
+              if (a->require_get_status)
+                all_have_intr_endp = 0;
             }
         }
+      else
+        {
+          if (a->require_get_status)
+            all_have_intr_endp = 0;
+        }
     }
   npth_mutex_unlock (&app_list_lock);
+
+  if (removal_detected)
+    update_fdset_for_usb (all_have_intr_endp);
 }
 
 /* This function must be called once to initialize this module.  This
@@ -1082,19 +1089,6 @@ initialize_module_command (void)
   return apdu_init ();
 }
 
-app_t
-app_list_start (void)
-{
-  npth_mutex_lock (&app_list_lock);
-  return app_top;
-}
-
-void
-app_list_finish (void)
-{
-  npth_mutex_unlock (&app_list_lock);
-}
-
 void
 app_send_card_list (ctrl_t ctrl)
 {
diff --git a/scd/ccid-driver.c b/scd/ccid-driver.c
index 8ca8e9646..1bb0fb267 100644
--- a/scd/ccid-driver.c
+++ b/scd/ccid-driver.c
@@ -274,6 +274,7 @@ struct ccid_driver_s
   void *progress_cb_arg;
 
   unsigned char intr_buf[64];
+  struct libusb_transfer *transfer;
 };
 
 
@@ -1699,17 +1700,13 @@ ccid_dev_scan (int *idx_max_p, struct ccid_dev_table **t_p)
   return err;
 }
 
-int
+void
 ccid_dev_scan_finish (struct ccid_dev_table *tbl, int max)
 {
-  int all_have_intr_endp = 1;
   int i;
 
   for (i = 0; i < max; i++)
     {
-      if (tbl[i].ep_intr == -1)
-        all_have_intr_endp = 0;
-
       free (tbl[i].ifcdesc_extra);
       tbl[i].transport = 0;
       tbl[i].n = 0;
@@ -1723,8 +1720,6 @@ ccid_dev_scan_finish (struct ccid_dev_table *tbl, int max)
     }
   libusb_free_device_list (ccid_usb_dev_list, 1);
   ccid_usb_dev_list = NULL;
-
-  return all_have_intr_endp;
 }
 
 unsigned int
@@ -1767,14 +1762,14 @@ intr_cb (struct libusb_transfer *transfer)
 {
   ccid_driver_t handle = transfer->user_data;
 
-  DEBUGOUT ("CCID: interrupt callback\n");
+  DEBUGOUT_1 ("CCID: interrupt callback %d\n", transfer->status);
 
-  if (transfer->status == LIBUSB_TRANSFER_NO_DEVICE)
+  if (transfer->status == LIBUSB_TRANSFER_NO_DEVICE
+      || transfer->status == LIBUSB_TRANSFER_ERROR)
     {
     device_removed:
       DEBUGOUT ("CCID: device removed\n");
       handle->powered_off = 1;
-      libusb_free_transfer (transfer);
     }
   else if (transfer->status == LIBUSB_TRANSFER_COMPLETED)
     {
@@ -1784,7 +1779,6 @@ intr_cb (struct libusb_transfer *transfer)
         {
           DEBUGOUT ("CCID: card removed\n");
           handle->powered_off = 1;
-          libusb_free_transfer (transfer);
         }
       else
         {
@@ -1795,12 +1789,12 @@ intr_cb (struct libusb_transfer *transfer)
           if (err == LIBUSB_ERROR_NO_DEVICE)
             goto device_removed;
 
-          DEBUGOUT_1 ("CCID submit transfer again %d", err);
+          DEBUGOUT_1 ("CCID submit transfer again %d\n", err);
         }
     }
   else
     {
-      ;
+      DEBUGOUT_1 ("CCID intr_cb: %d\n", transfer->status);
     }
 }
 
@@ -1811,6 +1805,7 @@ ccid_setup_intr  (ccid_driver_t handle)
   int err;
 
   transfer = libusb_alloc_transfer (0);
+  handle->transfer = transfer;
   libusb_fill_interrupt_transfer (transfer, handle->idev, handle->ep_intr,
                                   handle->intr_buf, sizeof (handle->intr_buf),
                                   intr_cb, handle, 0);
@@ -1913,7 +1908,7 @@ ccid_open_usb_reader (const char *spec_reader_name,
         }
     }
 
-  if ((*handle)->ep_intr)
+  if ((*handle)->ep_intr >= 0)
     ccid_setup_intr (*handle);
 
   rc = ccid_vendor_specific_init (*handle);
@@ -2010,6 +2005,26 @@ ccid_open_reader (const char *spec_reader_name, int idx,
 }
 
 
+int
+ccid_require_get_status (ccid_driver_t handle)
+{
+#ifdef LIBUSB_WORKS_EXPECTED_FOR_INTERRUPT_ENDP
+  if (handle->ep_intr >= 0)
+    return 0;
+#endif
+
+  /* Here comes products check for tokens which
+     always have card inserted.  */
+  switch (handle->id_vendor)
+    {
+    case VENDOR_FSIJ:
+      return 0;
+    }
+
+  return 1;
+}
+
+
 static void
 do_close_reader (ccid_driver_t handle)
 {
@@ -2037,6 +2052,7 @@ do_close_reader (ccid_driver_t handle)
     }
   if (handle->idev)
     {
+      libusb_free_transfer (handle->transfer);
       libusb_release_interface (handle->idev, handle->ifc_no);
       libusb_close (handle->idev);
       handle->idev = NULL;
diff --git a/scd/ccid-driver.h b/scd/ccid-driver.h
index 8e9f9e251..cff3f198f 100644
--- a/scd/ccid-driver.h
+++ b/scd/ccid-driver.h
@@ -115,7 +115,7 @@ int ccid_set_debug_level (int level);
 char *ccid_get_reader_list (void);
 
 gpg_error_t ccid_dev_scan (int *idx_max, struct ccid_dev_table **t_p);
-int ccid_dev_scan_finish (struct ccid_dev_table *tbl, int max);
+void ccid_dev_scan_finish (struct ccid_dev_table *tbl, int max);
 unsigned int ccid_get_BAI (int, struct ccid_dev_table *tbl);
 int ccid_compare_BAI (ccid_driver_t handle, unsigned int);
 int ccid_open_reader (const char *spec_reader_name,
@@ -140,7 +140,7 @@ int ccid_transceive_escape (ccid_driver_t handle,
                             const unsigned char *data, size_t datalen,
                             unsigned char *resp, size_t maxresplen,
                             size_t *nresp);
-
+int ccid_require_get_status (ccid_driver_t handle);
 
 
 #endif /*CCID_DRIVER_H*/
diff --git a/scd/scdaemon.c b/scd/scdaemon.c
index 14e0b05f0..4ab0fcf9a 100644
--- a/scd/scdaemon.c
+++ b/scd/scdaemon.c
@@ -1202,15 +1202,14 @@ start_connection_thread (void *arg)
 
 
 void
-update_fdset_for_usb (int scanned, int all_have_intr_endp)
+update_fdset_for_usb (int all_have_intr_endp)
 {
 #ifdef HAVE_LIBUSB
   const struct libusb_pollfd **pfd_array = libusb_get_pollfds (NULL);
   const struct libusb_pollfd **p;
 #endif
 
-  if (scanned)
-    usb_all_have_intr_endp = all_have_intr_endp;
+  usb_all_have_intr_endp = all_have_intr_endp;
 
   FD_ZERO (&fdset);
   nfd = 0;
@@ -1230,6 +1229,7 @@ update_fdset_for_usb (int scanned, int all_have_intr_endp)
       if (nfd < fd)
         nfd = fd;
       p++;
+      log_debug ("USB: add %d to fdset\n", fd);
     }
 
   libusb_free_pollfds (pfd_array);
@@ -1238,8 +1238,7 @@ update_fdset_for_usb (int scanned, int all_have_intr_endp)
   /* Kick the select loop.  */
   write (notify_fd, "", 1);
 
-  log_debug ("update_fdset_for_usb (%d, %d): %d %lx\n",
-             scanned, all_have_intr_endp, nfd, fdset.fds_bits[0]);
+  log_debug ("update_fdset_for_usb (%d): %d\n", all_have_intr_endp, nfd);
 }
 
 static int
@@ -1395,6 +1394,7 @@ handle_connections (void)
           char buf[256];
 
           read (pipe_fd[0], buf, sizeof buf);
+          ret--;
         }
 
       if (listen_fd != -1 && FD_ISSET (listen_fd, &read_fdset))
@@ -1439,6 +1439,8 @@ handle_connections (void)
       if (ret)
         {
           struct timeval tv = {0, 0};
+
+          log_debug ("scd main: USB handle events\n");
           libusb_handle_events_timeout_completed (NULL, &tv, NULL);
         }
 #endif
diff --git a/scd/scdaemon.h b/scd/scdaemon.h
index 9d92ff2c3..9e616f4c9 100644
--- a/scd/scdaemon.h
+++ b/scd/scdaemon.h
@@ -125,7 +125,7 @@ void send_status_info (ctrl_t ctrl, const char *keyword, ...)
 void send_status_direct (ctrl_t ctrl, const char *keyword, const char *args);
 void scd_update_reader_status_file (void);
 void send_client_notifications (app_t app, int removal);
-void update_fdset_for_usb (int scanned, int all_have_intr_endp);
+void update_fdset_for_usb (int all_have_intr_endp);
 
 
 #endif /*SCDAEMON_H*/