From d04467620bf0ab930b1983c741320cb29be77151 Mon Sep 17 00:00:00 2001 From: patacongo Date: Sun, 9 Jan 2011 02:15:30 +0000 Subject: [PATCH] Fix misc race conditions and use of stale pointer git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@3234 42af7a65-404d-4744-a932-0658087f49c3 --- arch/arm/src/lpc17xx/lpc17_usbhost.c | 125 +++++++-- drivers/usbhost/usbhost_storage.c | 404 ++++++++++++++++++--------- 2 files changed, 375 insertions(+), 154 deletions(-) diff --git a/arch/arm/src/lpc17xx/lpc17_usbhost.c b/arch/arm/src/lpc17xx/lpc17_usbhost.c index 49719673c0..b9882c34c6 100755 --- a/arch/arm/src/lpc17xx/lpc17_usbhost.c +++ b/arch/arm/src/lpc17xx/lpc17_usbhost.c @@ -158,8 +158,10 @@ struct lpc17_usbhost_s volatile uint8_t tdstatus; /* TD control status bits from last Writeback Done Head event */ volatile bool connected; /* Connected to device */ - sem_t rhssem; /* Semaphore to wait Root Hub Status change */ - sem_t wdhsem; /* Semaphore used to wait for Writeback Done Head event */ + volatile bool rhswait; /* TRUE: Thread is waiting for Root Hub Status change */ + volatile bool wdhwait; /* TRUE: Thread is waiting for WDH interrupt */ + sem_t rhssem; /* Semaphore to wait Writeback Done Head event */ + sem_t wdhsem; /* Semaphore used to wait for Writeback Done Head event */ }; /* The following are used to manage lists of free EDs and TD buffers*/ @@ -602,6 +604,38 @@ static void lpc17_enqueuetd(volatile struct ohci_ed_s *ed, uint32_t dirpid, ed->nexted = 0; } +/******************************************************************************* + * Name: lpc17_wdhwait + * + * Description: + * Set the request for the Writeback Done Head event well BEFORE enabling the + * transfer (as soon as we are absolutely committed to the to avoid transfer). + * We do this to minimize race conditions. This logic would have to be expanded + * if we want to have more than one packet in flight at a time! + * + *******************************************************************************/ + +static int lpc17_wdhwait(struct lpc17_usbhost_s *priv) +{ + irqstate_t flags = irqsave(); + int ret = -ENODEV; + + /* Is the device still connected? */ + + if (priv->connected) + { + /* Yes.. then set wdhwait to indicate that we expect to be informed when + * either (1) the device is disconnected, or (2) the transfer completed. + */ + + priv->wdhwait = true; + ret = OK; + } + + irqrestore(flags); + return ret; +} + /******************************************************************************* * Name: lpc17_ctrltd * @@ -621,6 +655,20 @@ static int lpc17_ctrltd(struct lpc17_usbhost_s *priv, uint32_t dirpid, { uint32_t toggle; uint32_t regval; + int ret; + + /* Set the request for the Writeback Done Head event well BEFORE enabling the + * transfer. + */ + + ret = lpc17_wdhwait(priv); + if (ret != OK) + { + udbg("ERROR: Device disconnected\n"); + return ret; + } + + /* Configure the toggle field in the TD */ if (dirpid == GTD_STATUS_DP_SETUP) { @@ -633,7 +681,7 @@ static int lpc17_ctrltd(struct lpc17_usbhost_s *priv, uint32_t dirpid, /* Then enqueue the transfer */ - priv->tdstatus = 0; + priv->tdstatus = TD_CC_NOERROR; lpc17_enqueuetd(EDCTRL, dirpid, toggle, buffer, buflen); /* Set the head of the control list to the EP0 EDCTRL (this would have to @@ -666,7 +714,7 @@ static int lpc17_ctrltd(struct lpc17_usbhost_s *priv, uint32_t dirpid, /* Check the TDHEAD completion status bits */ - if (priv->tdstatus == 0) + if (priv->tdstatus == TD_CC_NOERROR) { return OK; } @@ -733,12 +781,16 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) /* Yes.. connected. */ ullvdbg("Connected\n"); - priv->tdstatus = 0; + priv->tdstatus = TD_CC_NOERROR; + priv->connected = true; /* Notify any waiters */ - priv->connected = true; - lpc17_givesem(&priv->rhssem); + if (priv->rhswait) + { + lpc17_givesem(&priv->rhssem); + priv->rhswait = false; + } } else { @@ -762,11 +814,29 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) /* Yes.. Disconnect the class */ CLASS_DISCONNECTED(priv->class); + priv->class = NULL; } - /* Notify any waiters */ + /* Notify any waiters for the Root Hub Status change event */ - lpc17_givesem(&priv->rhssem); + if (priv->rhswait) + { + lpc17_givesem(&priv->rhssem); + priv->rhswait = false; + } + + /* Wake up the thread waiting for the WDH event. In my + * experience, the WHD event will occur later after the + * disconnection, but it is safer to make sure that + * there are no waiters. + */ + + if (priv->wdhwait) + { + priv->tdstatus = TD_CC_DEVNOTRESPONDING; + lpc17_givesem(&priv->wdhsem); + priv->wdhwait = false; + } } else { @@ -802,7 +872,7 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) /* Since there is only one TD, we can disable further TD processing. */ - regval = lpc17_getreg(LPC17_USBHOST_CTRL); + regval = lpc17_getreg(LPC17_USBHOST_CTRL); regval &= ~(OHCI_CTRL_PLE|OHCI_CTRL_IE|OHCI_CTRL_CLE|OHCI_CTRL_BLE); lpc17_putreg(regval, LPC17_USBHOST_CTRL); @@ -811,7 +881,7 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) priv->tdstatus = (TDHEAD->ctrl & GTD_STATUS_CC_MASK) >> GTD_STATUS_CC_SHIFT; #ifdef CONFIG_DEBUG_USB - if (priv->tdstatus != 0) + if (priv->tdstatus != TD_CC_NOERROR) { /* The transfer failed for some reason... dump some diagnostic info. */ @@ -823,8 +893,11 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) /* And wake up the thread waiting for the WDH event */ - DEBUGASSERT(priv->wdhsem.semcount <= 0); - lpc17_givesem(&priv->wdhsem); + if (priv->wdhwait) + { + lpc17_givesem(&priv->wdhsem); + priv->wdhwait = false; + } } #ifdef CONFIG_DEBUG_USB @@ -873,15 +946,19 @@ static int lpc17_usbinterrupt(int irq, FAR void *context) static int lpc17_wait(FAR struct usbhost_driver_s *drvr, bool connected) { struct lpc17_usbhost_s *priv = (struct lpc17_usbhost_s *)drvr; + irqstate_t flags; /* Are we already connected? */ + flags = irqsave(); while (priv->connected == connected) { /* No... wait for the connection/disconnection */ + priv->rhswait = true; lpc17_takesem(&priv->rhssem); } + irqrestore(flags); udbg("Connected:%s\n", priv->connected ? "YES" : "NO"); return OK; @@ -1202,7 +1279,7 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, #ifdef CONFIG_UBHOST_AHBIOBUFFERS uint8_t *origbuf = NULL; #endif - int ret = -ENOMEM; + int ret; DEBUGASSERT(drvr && ep && buffer && buflen > 0); uvdbg("EP%d %s maxpacket:%d buflen:%d\n", @@ -1220,6 +1297,7 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, { uvdbg("buflen (%d) > IO buffer size (%d)\n", buflen, CONFIG_USBHOST_IOBUFSIZE); + ret = -ENOMEM; goto errout; } @@ -1230,6 +1308,7 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, if (!buffer) { uvdbg("IO buffer allocation failed\n"); + ret = -ENOMEM; goto errout; } @@ -1246,12 +1325,24 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, } #endif + /* Set the request for the Writeback Done Head event well BEFORE enabling the + * transfer. + */ + + ret = lpc17_wdhwait(priv); + if (ret != OK) + { + udbg("ERROR: Device disconnected\n"); + goto errout; + } + /* Allocate an ED */ ed = lpc17_edalloc(priv); if (!ed) { udbg("ED allocation failed\n"); + ret = -ENOMEM; goto errout; } @@ -1277,7 +1368,7 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, /* Then enqueue the transfer */ - priv->tdstatus = 0; + priv->tdstatus = TD_CC_NOERROR; lpc17_enqueuetd(ed, dirpid, GTD_STATUS_T_TOGGLE, buffer, buflen); /* Set the head of the bulk list to the EP descriptor (this would have to @@ -1310,7 +1401,7 @@ static int lpc17_transfer(FAR struct usbhost_driver_s *drvr, /* Check the TDHEAD completion status bits */ - if (priv->tdstatus == 0) + if (priv->tdstatus == TD_CC_NOERROR) { ret = OK; } @@ -1332,7 +1423,7 @@ errout: * way around this copy. */ - if (ep->in != 0) + if (ep->in != 0 && ret == OK) { memcpy(origbuf, buffer, buflen); } diff --git a/drivers/usbhost/usbhost_storage.c b/drivers/usbhost/usbhost_storage.c index 680e215690..7aee9d2ea0 100644 --- a/drivers/usbhost/usbhost_storage.c +++ b/drivers/usbhost/usbhost_storage.c @@ -120,16 +120,17 @@ struct usbhost_state_s /* The remainder of the fields are provide o the mass storage class */ - char sdchar; /* Character identifying the /dev/sd[n] device */ - int16_t crefs; /* Reference count on the driver instance */ - uint16_t blocksize; /* Block size of USB mass storage device */ - uint32_t nblocks; /* Number of blocks on the USB mass storage device */ - sem_t exclsem; /* Used to maintain mutual exclusive access */ - struct work_s work; /* For interacting with the worker thread */ - FAR uint8_t *tdbuffer; /* The allocated transfer descriptor buffer */ - size_t tdbuflen; /* Size of the allocated transfer buffer */ - struct usbhost_epdesc_s bulkin; /* Bulk IN endpoint */ - struct usbhost_epdesc_s bulkout; /* Bulk OUT endpoint */ + char sdchar; /* Character identifying the /dev/sd[n] device */ + volatile bool disconnected; /* TRUE: Device has been disconnected */ + int16_t crefs; /* Reference count on the driver instance */ + uint16_t blocksize; /* Block size of USB mass storage device */ + uint32_t nblocks; /* Number of blocks on the USB mass storage device */ + sem_t exclsem; /* Used to maintain mutual exclusive access */ + struct work_s work; /* For interacting with the worker thread */ + FAR uint8_t *tdbuffer; /* The allocated transfer descriptor buffer */ + size_t tdbuflen; /* Size of the allocated transfer buffer */ + struct usbhost_epdesc_s bulkin; /* Bulk IN endpoint */ + struct usbhost_epdesc_s bulkout; /* Bulk OUT endpoint */ }; /**************************************************************************** @@ -143,8 +144,8 @@ static void usbhost_takesem(sem_t *sem); /* Memory allocation services */ -static inline struct usbhost_state_s *usbhost_allocclass(void); -static inline void usbhost_freeclass(struct usbhost_state_s *class); +static inline FAR struct usbhost_state_s *usbhost_allocclass(void); +static inline void usbhost_freeclass(FAR struct usbhost_state_s *class); /* Device name management */ @@ -342,7 +343,7 @@ static void usbhost_takesem(sem_t *sem) ****************************************************************************/ #if CONFIG_USBHOST_NPREALLOC > 0 -static inline struct usbhost_state_s *usbhost_allocclass(void) +static inline FAR struct usbhost_state_s *usbhost_allocclass(void) { struct usbhost_state_s *priv; irqstate_t flags; @@ -359,17 +360,22 @@ static inline struct usbhost_state_s *usbhost_allocclass(void) priv->class.flink = NULL; } irqrestore(flags); + ullvdbg("Allocated: %p\n", priv);; return priv; } #else -static inline struct usbhost_state_s *usbhost_allocclass(void) +static inline FAR struct usbhost_state_s *usbhost_allocclass(void) { + FAR struct usbhost_state_s *priv; + /* We are not executing from an interrupt handler so we can just call * malloc() to get memory for the class instance. */ DEBUGASSERT(!up_interrupt_context()); - return (struct usbhost_state_s *)malloc(sizeof(struct usbhost_state_s)); + priv = (FAR struct usbhost_state_s *)malloc(sizeof(struct usbhost_state_s)); + uvdbg("Allocated: %p\n", priv);; + return priv; } #endif @@ -388,11 +394,13 @@ static inline struct usbhost_state_s *usbhost_allocclass(void) ****************************************************************************/ #if CONFIG_USBHOST_NPREALLOC > 0 -static inline void usbhost_freeclass(struct usbhost_state_s *class) +static inline void usbhost_freeclass(FAR struct usbhost_state_s *class) { irqstate_t flags; DEBUGASSERT(class != NULL); + ullvdbg("Freeing: %p\n", class);; + /* Just put the pre-allocated class structure back on the freelist */ flags = irqsave(); @@ -401,7 +409,7 @@ static inline void usbhost_freeclass(struct usbhost_state_s *class) irqrestore(flags); } #else -static inline void usbhost_freeclass(struct usbhost_state_s *class) +static inline void usbhost_freeclass(FAR struct usbhost_state_s *class) { DEBUGASSERT(class != NULL); @@ -409,6 +417,7 @@ static inline void usbhost_freeclass(struct usbhost_state_s *class) * from an interrupt handler. */ + uvdbg("Freeing: %p\n", class);; free(class); } #endif @@ -651,34 +660,101 @@ static inline int usbhost_maxlunreq(FAR struct usbhost_state_s *priv) FAR struct usb_ctrlreq_s *req = (FAR struct usb_ctrlreq_s *)priv->tdbuffer; DEBUGASSERT(priv && priv->tdbuffer); - /* Request maximum logical unit number. NOTE: On an IN transaction, The - * req and buffer pointers passed to DRVR_CTRLIN may refer to the same - * allocated memory. - */ + /* Make sure that the device is still connected. */ - uvdbg("Request maximum logical unit number\n"); - memset(req, 0, sizeof(struct usb_ctrlreq_s)); - req->type = USB_DIR_IN|USB_REQ_TYPE_CLASS|USB_REQ_RECIPIENT_INTERFACE; - req->req = USBSTRG_REQ_GETMAXLUN; - usbhost_putle16(req->len, 1); - return DRVR_CTRLIN(priv->drvr, req, priv->tdbuffer); + if (!priv->disconnected) + { + /* Request maximum logical unit number. NOTE: On an IN transaction, The + * req and buffer pointers passed to DRVR_CTRLIN may refer to the same + * allocated memory. + */ + + uvdbg("Request maximum logical unit number\n"); + memset(req, 0, sizeof(struct usb_ctrlreq_s)); + req->type = USB_DIR_IN|USB_REQ_TYPE_CLASS|USB_REQ_RECIPIENT_INTERFACE; + req->req = USBSTRG_REQ_GETMAXLUN; + usbhost_putle16(req->len, 1); + return DRVR_CTRLIN(priv->drvr, req, priv->tdbuffer); + } + + udbg("ERROR: No longer connected\n"); + return -ENODEV; } static inline int usbhost_testunitready(FAR struct usbhost_state_s *priv) { FAR struct usbstrg_cbw_s *cbw; - int result = -ENOMEM; + int result; + + /* Make sure that the device is still connected */ + + if (priv->disconnected) + { + udbg("ERROR: No longer connected\n"); + return -ENODEV; + } /* Initialize a CBW (re-using the allocated transfer buffer) */ cbw = usbhost_cbwalloc(priv); - if (cbw) + if (!cbw) { - /* Construct and send the CBW */ + udbg("ERROR: Failed to create CBW\n"); + return -ENOMEM; + } + + /* Construct and send the CBW */ - usbhost_testunitreadycbw(cbw); - result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, - (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + usbhost_testunitreadycbw(cbw); + result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, + (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + if (result == OK) + { + /* Receive the CSW */ + + result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, + priv->tdbuffer, USBSTRG_CSW_SIZEOF); + if (result == OK) + { + usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); + } + } + return result; +} + +static inline int usbhost_requestsense(FAR struct usbhost_state_s *priv) +{ + FAR struct usbstrg_cbw_s *cbw; + int result; + + /* Make sure that the device is still connected */ + + if (priv->disconnected) + { + udbg("ERROR: No longer connected\n"); + return -ENODEV; + } + + /* Initialize a CBW (re-using the allocated transfer buffer) */ + + cbw = usbhost_cbwalloc(priv); + if (!cbw) + { + udbg("ERROR: Failed to create CBW\n"); + return -ENOMEM; + } + + /* Construct and send the CBW */ + + usbhost_requestsensecbw(cbw); + result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, + (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + if (result == OK) + { + /* Receive the sense data response */ + + result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, + priv->tdbuffer, SCSIRESP_FIXEDSENSEDATA_SIZEOF); if (result == OK) { /* Receive the CSW */ @@ -691,43 +767,7 @@ static inline int usbhost_testunitready(FAR struct usbhost_state_s *priv) } } } - return result; -} -static inline int usbhost_requestsense(FAR struct usbhost_state_s *priv) -{ - FAR struct usbstrg_cbw_s *cbw; - int result = -ENOMEM; - - /* Initialize a CBW (re-using the allocated transfer buffer) */ - - cbw = usbhost_cbwalloc(priv); - if (cbw) - { - /* Construct and send the CBW */ - - usbhost_requestsensecbw(cbw); - result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, - (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); - if (result == OK) - { - /* Receive the sense data response */ - - result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, SCSIRESP_FIXEDSENSEDATA_SIZEOF); - if (result == OK) - { - /* Receive the CSW */ - - result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, USBSTRG_CSW_SIZEOF); - if (result == OK) - { - usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); - } - } - } - } return result; } @@ -735,43 +775,55 @@ static inline int usbhost_readcapacity(FAR struct usbhost_state_s *priv) { FAR struct usbstrg_cbw_s *cbw; FAR struct scsiresp_readcapacity10_s *resp; - int result = -ENOMEM; + int result; + + /* Make sure that the device is still connected */ + + if (priv->disconnected) + { + udbg("ERROR: No longer connected\n"); + return -ENODEV; + } /* Initialize a CBW (re-using the allocated transfer buffer) */ cbw = usbhost_cbwalloc(priv); - if (cbw) + if (!cbw) { - /* Construct and send the CBW */ + udbg("ERROR: Failed to create CBW\n"); + return -ENOMEM; + } + + /* Construct and send the CBW */ - usbhost_readcapacitycbw(cbw); - result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, - (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + usbhost_readcapacitycbw(cbw); + result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, + (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + if (result == OK) + { + /* Receive the read capacity CBW IN response */ + + result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, + priv->tdbuffer, SCSIRESP_READCAPACITY10_SIZEOF); if (result == OK) { - /* Receive the read capacity CBW IN response */ + /* Save the capacity information */ + + resp = (FAR struct scsiresp_readcapacity10_s *)priv->tdbuffer; + priv->nblocks = usbhost_getbe32(resp->lba); + priv->blocksize = usbhost_getbe32(resp->blklen); + + /* Receive the CSW */ result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, SCSIRESP_READCAPACITY10_SIZEOF); + priv->tdbuffer, USBSTRG_CSW_SIZEOF); if (result == OK) { - /* Save the capacity information */ - - resp = (FAR struct scsiresp_readcapacity10_s *)priv->tdbuffer; - priv->nblocks = usbhost_getbe32(resp->lba); - priv->blocksize = usbhost_getbe32(resp->blklen); - - /* Receive the CSW */ - - result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, USBSTRG_CSW_SIZEOF); - if (result == OK) - { - usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); - } + usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); } } } + return result; } @@ -779,41 +831,53 @@ static inline int usbhost_inquiry(FAR struct usbhost_state_s *priv) { FAR struct usbstrg_cbw_s *cbw; FAR struct scsiresp_inquiry_s *resp; - int result = -ENOMEM; + int result; + + /* Make sure that the device is still connected */ + + if (priv->disconnected) + { + udbg("ERROR: No longer connected\n"); + return -ENODEV; + } /* Initialize a CBW (re-using the allocated transfer buffer) */ cbw = usbhost_cbwalloc(priv); - if (cbw) + if (!cbw) { - /* Construct and send the CBW */ + udbg("ERROR: Failed to create CBW\n"); + return -ENOMEM; + } + + /* Construct and send the CBW */ - usbhost_inquirycbw(cbw); - result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, - (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + usbhost_inquirycbw(cbw); + result = DRVR_TRANSFER(priv->drvr, &priv->bulkout, + (uint8_t*)cbw, USBSTRG_CBW_SIZEOF); + if (result == OK) + { + /* Receive the CBW IN response */ + + result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, + priv->tdbuffer, SCSIRESP_INQUIRY_SIZEOF); if (result == OK) { - /* Receive the CBW IN response */ + /* TODO: If USB debug is enabled, dump the response data here */ + + resp = (FAR struct scsiresp_inquiry_s *)priv->tdbuffer; + + /* Receive the CSW */ result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, SCSIRESP_INQUIRY_SIZEOF); + priv->tdbuffer, USBSTRG_CSW_SIZEOF); if (result == OK) { - /* TODO: If USB debug is enabled, dump the response data here */ - - resp = (FAR struct scsiresp_inquiry_s *)priv->tdbuffer; - - /* Receive the CSW */ - - result = DRVR_TRANSFER(priv->drvr, &priv->bulkin, - priv->tdbuffer, USBSTRG_CSW_SIZEOF); - if (result == OK) - { - usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); - } + usbhost_dumpcsw((FAR struct usbstrg_csw_s *)priv->tdbuffer); } } } + return result; } @@ -839,6 +903,7 @@ static void usbhost_destroy(FAR void *arg) char devname[DEV_NAMELEN]; DEBUGASSERT(priv != NULL); + uvdbg("crefs: %d\n", priv->crefs); /* Unregister the block driver */ @@ -857,8 +922,14 @@ static void usbhost_destroy(FAR void *arg) sem_destroy(&priv->exclsem); + /* Disconnect the USB host device */ + + DRVR_DISCONNECT(priv->drvr); + /* And free the class instance. Hmmm.. this may execute on the worker - * thread and the work structure is part of what is getting freed. + * thread and the work structure is part of what is getting freed. That + * should be okay because once the work contained is removed from the + * queue, it should not longer be accessed by the worker thread. */ usbhost_freeclass(priv); @@ -898,10 +969,17 @@ static void usbhost_initvolume(FAR void *arg) result = usbhost_tdalloc(priv); if (result != OK) { - udbg("Failed to allocate transfer buffer\n"); + udbg("ERROR: Failed to allocate transfer buffer\n"); return; } + /* Increment the reference count. This will prevent usbhost_destroy() from + * being called asynchronously if the device is removed. + */ + + priv->crefs++; + DEBUGASSERT(priv->crefs == 2); + /* Request the maximum logical unit number */ uvdbg("Get max LUN\n"); @@ -959,7 +1037,7 @@ static void usbhost_initvolume(FAR void *arg) csw = (FAR struct usbstrg_csw_s *)priv->tdbuffer; if (csw->status != 0) { - udbg("CSW status error: %d\n", csw->status); + udbg("ERROR: CSW status error: %d\n", csw->status); result = -ENODEV; } } @@ -980,7 +1058,7 @@ static void usbhost_initvolume(FAR void *arg) csw = (FAR struct usbstrg_csw_s *)priv->tdbuffer; if (csw->status != 0) { - udbg("CSW status error: %d\n", csw->status); + udbg("ERROR: CSW status error: %d\n", csw->status); result = -ENODEV; } } @@ -997,18 +1075,45 @@ static void usbhost_initvolume(FAR void *arg) result = register_blockdriver(devname, &g_bops, 0, priv); } - /* Check if we successfully initialized */ + /* Check if we successfully initialized. We now have to be concerned + * about asynchronous modification of crefs because the block + * driver has been registerd. + */ if (result == OK) { - /* Ready for normal operation as a block device driver */ + usbhost_takesem(&priv->exclsem); + DEBUGASSERT(priv->crefs >= 2); - uvdbg("Successfully initialized\n"); + /* Handle a corner case where (1) open() has been called so the + * reference count is > 2, but the device has been disconnected. + * In this case, the class instance needs to persist until close() + * is called. + */ + + if (priv->crefs <= 2 && priv->disconnected) + { + /* We don't have to give the semaphore because it will be + * destroyed when usb_destroy is called. + */ + + result = -ENODEV; + } + else + { + /* Ready for normal operation as a block device driver */ + + uvdbg("Successfully initialized\n"); + priv->crefs--; + usbhost_givesem(&priv->exclsem); + } } - else + + /* Disconnect on any errors detected during volume initialization */ + + if (result != OK) { udbg("ERROR! Aborting: %d\n", result); - DRVR_DISCONNECT(priv->drvr); usbhost_destroy(priv); } } @@ -1040,6 +1145,8 @@ static void usbhost_work(FAR struct usbhost_state_s *priv, worker_t worker) * prevent us from over-running the work structure. */ + uvdbg("Queuing work: worker %p->%p\n", priv->work.worker, worker); + DEBUGASSERT(priv->work.worker == NULL); (void)work_queue(&priv->work, worker, priv, 0); } else @@ -1258,12 +1365,16 @@ static inline int usbhost_tdalloc(FAR struct usbhost_state_s *priv) static inline int usbhost_tdfree(FAR struct usbhost_state_s *priv) { - int result; - DEBUGASSERT(priv && priv->tdbuffer != NULL); + int result = OK; + DEBUGASSERT(priv); - result = DRVR_FREE(priv->drvr, priv->tdbuffer); - priv->tdbuffer = NULL; - priv->tdbuflen = 0; + if (priv->tdbuffer) + { + DEBUGASSERT(priv->drvr); + result = DRVR_FREE(priv->drvr, priv->tdbuffer); + priv->tdbuffer = NULL; + priv->tdbuflen = 0; + } return result; } @@ -1594,8 +1705,8 @@ static int usbhost_disconnected(struct usbhost_class_s *class) * of the mass storage device that the device is no longer available. */ - flags = irqsave(); - priv->drvr = NULL; + flags = irqsave(); + priv->disconnected = true; /* Now check the number of references on the class instance. If it is one, * then we can free the class instance now. Otherwise, we will have to @@ -1603,6 +1714,7 @@ static int usbhost_disconnected(struct usbhost_class_s *class) * block driver. */ + ullvdbg("crefs: %d\n", priv->crefs); if (priv->crefs == 1) { /* Destroy the class instance */ @@ -1626,15 +1738,25 @@ static int usbhost_disconnected(struct usbhost_class_s *class) static int usbhost_open(FAR struct inode *inode) { FAR struct usbhost_state_s *priv; + irqstate_t flags; int ret; uvdbg("Entry\n"); DEBUGASSERT(inode && inode->i_private); priv = (FAR struct usbhost_state_s *)inode->i_private; - /* Check if the mass storage device is still connected */ + /* Make sure that we have exclusive access to the private data structure */ - if (!priv->drvr) + DEBUGASSERT(priv->crefs > 0 && priv->crefs < USBHOST_MAX_CREFS); + usbhost_takesem(&priv->exclsem); + + /* Check if the mass storage device is still connected. We need to disable + * interrupts momentarily to assure that there are no asynchronous disconnect + * events. + */ + + flags = irqsave(); + if (priv->disconnected) { /* No... the block driver is no longer bound to the class. That means that * the USB storage device is no longer connected. Refuse any further @@ -1647,13 +1769,12 @@ static int usbhost_open(FAR struct inode *inode) { /* Otherwise, just increment the reference count on the driver */ - DEBUGASSERT(priv->crefs > 0 && priv->crefs < USBHOST_MAX_CREFS); - usbhost_takesem(&priv->exclsem); priv->crefs++; - usbhost_givesem(&priv->exclsem); ret = OK; } + irqrestore(flags); + usbhost_givesem(&priv->exclsem); return ret; } @@ -1667,6 +1788,7 @@ static int usbhost_open(FAR struct inode *inode) static int usbhost_close(FAR struct inode *inode) { FAR struct usbhost_state_s *priv; + irqstate_t flags; uvdbg("Entry\n"); DEBUGASSERT(inode && inode->i_private); @@ -1685,19 +1807,27 @@ static int usbhost_close(FAR struct inode *inode) usbhost_givesem(&priv->exclsem); + /* We need to disable interrupts momentarily to assure that there are + * no asynchronous disconnect events. + */ + + flags = irqsave(); + /* Check if the USB mass storage device is still connected. If the * storage device is not connected and the reference count just * decremented to one, then unregister the block driver and free * the class instance. */ - if (priv->crefs <= 1 && priv->drvr == NULL) + if (priv->crefs <= 1 && priv->disconnected) { /* Destroy the class instance */ DEBUGASSERT(priv->crefs == 1); usbhost_destroy(priv); } + + irqrestore(flags); return OK; } @@ -1724,7 +1854,7 @@ static ssize_t usbhost_read(FAR struct inode *inode, unsigned char *buffer, /* Check if the mass storage device is still connected */ - if (!priv->drvr) + if (priv->disconnected) { /* No... the block driver is no longer bound to the class. That means that * the USB storage device is no longer connected. Refuse any attempt to read @@ -1816,7 +1946,7 @@ static ssize_t usbhost_write(FAR struct inode *inode, const unsigned char *buffe /* Check if the mass storage device is still connected */ - if (!priv->drvr) + if (priv->disconnected) { /* No... the block driver is no longer bound to the class. That means that * the USB storage device is no longer connected. Refuse any attempt to @@ -1904,7 +2034,7 @@ static int usbhost_geometry(FAR struct inode *inode, struct geometry *geometry) /* Check if the mass storage device is still connected */ priv = (FAR struct usbhost_state_s *)inode->i_private; - if (!priv->drvr) + if (priv->disconnected) { /* No... the block driver is no longer bound to the class. That means that * the USB storage device is no longer connected. Refuse to return any @@ -1957,7 +2087,7 @@ static int usbhost_ioctl(FAR struct inode *inode, int cmd, unsigned long arg) /* Check if the mass storage device is still connected */ - if (!priv->drvr) + if (priv->disconnected) { /* No... the block driver is no longer bound to the class. That means that * the USB storage device is no longer connected. Refuse to process any