From 9294b7d6fd575877a3050e12c80678ec3eaa4036 Mon Sep 17 00:00:00 2001 From: patacongo Date: Mon, 19 Mar 2012 17:56:15 +0000 Subject: [PATCH] Minor updates for PIC32 USB device driver bugs git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@4497 42af7a65-404d-4744-a932-0658087f49c3 --- ChangeLog | 2 +- TODO | 37 +++++++++++++++++++++++--- arch/mips/src/pic32mx/pic32mx-usbdev.c | 3 ++- configs/pic32-starterkit/README.txt | 26 ++++++++++++++++++ drivers/usbdev/usbmsc_scsi.c | 12 +++++---- include/nuttx/usb/usbdev_trace.h | 4 +-- sched/pthread_condsignal.c | 13 +++++++-- sched/pthread_condwait.c | 4 +-- 8 files changed, 84 insertions(+), 17 deletions(-) diff --git a/ChangeLog b/ChangeLog index 48efd5a467..229326e99d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2578,4 +2578,4 @@ * arch/mips/src/pic32mx/picm32mx-usbdev.c: Several stall-related fixes so that the USB device driver can used the the mass storage class (which does a LOT of stalling as part of its normal protocol). The PIC32 USB Mass Storage - device now seems functional (but has not been tested exhaustively). + device is, however, still non-functional when debug is OFF. diff --git a/TODO b/TODO index d20309ae16..0baac17ab7 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated February 21, 2012) +NuttX TODO List (Last updated March 19, 2012) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -35,7 +35,7 @@ nuttx/ (3) AVR (arch/avr) (0) Intel x86 (arch/x86) (4) 8051 / MCS51 (arch/8051/) - (0) MIPS/PIC32 (arch/mips) + (1) MIPS/PIC32 (arch/mips) (1) Hitachi/Renesas SH-1 (arch/sh/src/sh1) (4) Renesas M16C/26 (arch/sh/src/m16c) (10) z80/z8/ez80 (arch/z80/) @@ -1253,8 +1253,37 @@ o 8051 / MCS51 (arch/8051/) Status: Open Priority: Low -- only because there as so many other issues with 8051 -o MIPS (arch/mips) - ^^^^^^^^^^^^^^^^ +o MIPS/PIC32(arch/mips) + ^^^^^^^^^^^^^^^^^^^^^ + + Title: PIC32 USB DRIVER DOES NOT WORK WITH MASS STORAGE CLASS + Description: The PIC32 USB driver either crashes or hangs when used with + the mass storage class when trying to write files to the target + storage device. This usually works with debug on, but does not + work with debug OFF (implying some race condition?) + + Here are some details of what I see in debugging: + + 1. The USB MSC device completes processing of a read request + and returns the read request to the driver. + 2. Before the MSC device can even begin the wait for the next + driver, many packets come in at interrupt level. The MSC + device goes to sleep (on pthread_cond_wait) with all of the + read buffers ready (16 in my test case). + 3. The pthread_cond_wait() does not wake up. This implies + a problem with pthread_con_wait(?). But in other cases, + the MSC device does wake up, but then immediately crashes + because its stack is bad. + 4. If I force the pthread_cond_wait to wake up (by using + pthread_cond_timedwait instead), then the thread wakes + up and crashes with a bad stack. + + So far, I have no clue why this is failing. + Status: Open + Priority: High + + + o Hitachi/Renesas SH-1 (arch/sh/src/sh1) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/arch/mips/src/pic32mx/pic32mx-usbdev.c b/arch/mips/src/pic32mx/pic32mx-usbdev.c index 9998ca4427..79a5bfaa8d 100644 --- a/arch/mips/src/pic32mx/pic32mx-usbdev.c +++ b/arch/mips/src/pic32mx/pic32mx-usbdev.c @@ -1013,7 +1013,7 @@ static void pic32mx_rqrestart(int argc, uint32_t arg1, ...) privep->stalled = false; privep->txnullpkt = false; - /* Check the request at the head of the endpoint's active request queue */ + /* Check the request at the head of the endpoint's pending request queue */ privreq = pic32mx_rqhead(&privep->pend); if (privreq) @@ -1646,6 +1646,7 @@ static int pic32mx_rdrequest(struct pic32mx_usbdev_s *priv, if (ret == OK) { privreq = pic32mx_remfirst(&privep->pend); + DEBUGASSERT(privreq != NULL); pic32mx_addlast(&privep->active, privreq); } return ret; diff --git a/configs/pic32-starterkit/README.txt b/configs/pic32-starterkit/README.txt index fb693f54e2..db38863430 100644 --- a/configs/pic32-starterkit/README.txt +++ b/configs/pic32-starterkit/README.txt @@ -1171,4 +1171,30 @@ Where is one of the following: little testing I have done with it, it appears functional. But the logic has not been stressed and there could still be lurking issues. + Update. The following was added to the top-level TODO list: + Title: PIC32 USB DRIVER DOES NOT WORK WITH MASS STORAGE CLASS + Description: The PIC32 USB driver either crashes or hangs when used with + the mass storage class when trying to write files to the target + storage device. This usually works with debug on, but does not + work with debug OFF (implying some race condition?) + + Here are some details of what I see in debugging: + + 1. The USB MSC device completes processing of a read request + and returns the read request to the driver. + 2. Before the MSC device can even begin the wait for the next + driver, many packets come in at interrupt level. The MSC + device goes to sleep (on pthread_cond_wait) with all of the + read buffers ready (16 in my test case). + 3. The pthread_cond_wait() does not wake up. This implies + a problem with pthread_con_wait(?). But in other cases, + the MSC device does wake up, but then immediately crashes + because its stack is bad. + 4. If I force the pthread_cond_wait to wake up (by using + pthread_cond_timedwait instead), then the thread wakes + up and crashes with a bad stack. + + So far, I have no clue why this is failing. + Status: Open + Priority: High diff --git a/drivers/usbdev/usbmsc_scsi.c b/drivers/usbdev/usbmsc_scsi.c index fa443497f2..ccc9676189 100644 --- a/drivers/usbdev/usbmsc_scsi.c +++ b/drivers/usbdev/usbmsc_scsi.c @@ -1626,11 +1626,6 @@ static int usbmsc_idlestate(FAR struct usbmsc_dev_s *priv) req->priv = privreq; req->callback = usbmsc_rdcomplete; - if (EP_SUBMIT(priv->epbulkout, req) != OK) - { - usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_IDLERDSUBMIT), (uint16_t)-ret); - } - /* Change to the CMDPARSE state and return success */ usbtrace(TRACE_CLASSSTATE(USBMSC_CLASSSTATE_IDLECMDPARSE), priv->cdb[0]); @@ -1638,6 +1633,13 @@ static int usbmsc_idlestate(FAR struct usbmsc_dev_s *priv) ret = OK; } + /* In any event, return the request to be refilled */ + + if (EP_SUBMIT(priv->epbulkout, req) != OK) + { + usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_IDLERDSUBMIT), (uint16_t)-ret); + } + return ret; } diff --git a/include/nuttx/usb/usbdev_trace.h b/include/nuttx/usb/usbdev_trace.h index 051b4ffa65..ae8e13c3a4 100644 --- a/include/nuttx/usb/usbdev_trace.h +++ b/include/nuttx/usb/usbdev_trace.h @@ -1,8 +1,8 @@ /**************************************************************************** * include/nuttx/usb/usbdev_trace.h * - * Copyright (C) 2008, 2009-2010 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Copyright (C) 2008, 2009-2010, 2012 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions diff --git a/sched/pthread_condsignal.c b/sched/pthread_condsignal.c index a10dda6be4..8f96532df2 100644 --- a/sched/pthread_condsignal.c +++ b/sched/pthread_condsignal.c @@ -1,8 +1,8 @@ /**************************************************************************** * sched/pthread_condsignal.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Copyright (C) 2007-2009, 2012 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -112,6 +112,15 @@ int pthread_cond_signal(FAR pthread_cond_t *cond) else { + /* One of my objectives in this design was to make pthread_cond_signal + * usable from interrupt handlers. However, from interrupt handlers, + * you cannot take the associated mutex before signaling the condition. + * As a result, I think that there could be a race condition with + * the following logic which assumes that the if sval < 0 then the + * thread is waiting. Without the mutex, there is no atomic, protected + * operation that will guarantee this to be so. + */ + sdbg("sval=%d\n", sval); if (sval < 0) { diff --git a/sched/pthread_condwait.c b/sched/pthread_condwait.c index 38e3ddeb9b..9863491371 100644 --- a/sched/pthread_condwait.c +++ b/sched/pthread_condwait.c @@ -1,8 +1,8 @@ /**************************************************************************** * sched/pthread_condwait.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Copyright (C) 2007-2009, 2012 Gregory Nutt. All rights reserved. + * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions