| Message ID | 20181010024903.1633-6-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request | expand |
Hi, On Tue, Oct 09, 2018 at 10:49:02PM -0400, Paul Elder wrote: > This patch implements a mechanism to signal the MUSB driver to reply to > a control OUT request with STALL or ACK. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> The patch looks good to me, here is my Acked-by: Acked-by: Bin Liu <b-liu@ti.com> but I am unable to test this patch set - the current Greg's usb-next tree gives deadlock error between composite_disconnect() and usb_function_deactivate() when loading g_webcam.ko on BeagleboneBlack. The error happens before applying this patch set. Regards, -Bin.
Hi Bin, On Thu, Oct 11, 2018 at 11:07:46AM -0500, Bin Liu wrote: > Hi, > > On Tue, Oct 09, 2018 at 10:49:02PM -0400, Paul Elder wrote: > > This patch implements a mechanism to signal the MUSB driver to reply to > > a control OUT request with STALL or ACK. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The patch looks good to me, here is my Acked-by: > > Acked-by: Bin Liu <b-liu@ti.com> > > but I am unable to test this patch set - the current Greg's usb-next > tree gives deadlock error between composite_disconnect() and > usb_function_deactivate() when loading g_webcam.ko on BeagleboneBlack. > The error happens before applying this patch set. We don't use g_webcam anymore, because it doesn't offer the flexibility that configfs does (for example, only one function can be configured with g_webcam). There are no features that g_webcam offers that configfs doesn't. I was unable to reproduce the deadlock that you mention on Greg's usb-next tree. Which commit were you on? I did, however, get the deadlock that you mention upon *killing* the userspace application providing the stream, not when loading g_webcam.ko. Here is a sample script for setting up a UVC gadget through configfs (I haven't tested this exact script; I extracted the functional components): CONFIGFS="/sys/kernel/config" GADGET="$CONFIGFS/usb_gadget" VID="0x0525" PID="0xa4a2" SERIAL="0123456789" MANUF=$(hostname) PRODUCT="UVC Gadget" UDC=`ls /sys/class/udc` cd $GADGET/g1 echo $VID > idVendor echo $PID > idProduct mkdir -p strings/0x409 echo $SERIAL > strings/0x409/serialnumber echo $MANUF > strings/0x409/manufacturer echo $PRODUCT > strings/0x409/product mkdir configs/c.1 mkdir configs/c.1/strings/0x409 # create uvc CONFIG="configs/c.1" FUNCTION="uvc.0" mkdir functions/$FUNCTION # create frame 640x360 uncompressed WIDTH=640 HEIGHT=360 wdir=functions/$FUNCTION/streaming/uncompressed/u/${HEIGHT}p mkdir $wdir echo $WIDTH > $wdir/wWidth echo $HEIGHT > $wdir/wHeight echo $(( $WIDTH * $HEIGHT * 2 )) > $wdir/dwMaxVideoFrameBufferSize cat <<EOF > $wdir/dwFrameInterval 666666 100000 5000000 EOF # end create frame mkdir functions/$FUNCTION/streaming/header/h cd functions/$FUNCTION/streaming/header/h ln -s ../../uncompressed/u cd ../../class/fs ln -s ../../header/h cd ../../class/hs ln -s ../../header/h cd ../../../control mkdir header/h ln -s header/h class/fs ln -s header/h class/ss cd ../../../ # Set the packet size: uvc gadget max size is 3k... echo 3072 > functions/$FUNCTION/streaming_maxpacket echo 2048 > functions/$FUNCTION/streaming_maxpacket echo 1024 > functions/$FUNCTION/streaming_maxpacket ln -s functions/$FUNCTION configs/c.1 # end create uvc echo $UDC > UDC Thanks, Paul
diff --git a/drivers/usb/musb/musb_gadget_ep0.c b/drivers/usb/musb/musb_gadget_ep0.c index 91a5027b5c1f..f0ed1f7472a3 100644 --- a/drivers/usb/musb/musb_gadget_ep0.c +++ b/drivers/usb/musb/musb_gadget_ep0.c @@ -458,6 +458,25 @@ __acquires(musb->lock) return handled; } +static int ep0_send_response(struct musb *musb, bool stall) +{ + void __iomem *regs = musb->control_ep->regs; + u16 ackpend; + + if (musb->ep0_state != MUSB_EP0_STAGE_RX && + musb->ep0_state != MUSB_EP0_STAGE_STATUSIN) + return -EINVAL; + + ackpend = MUSB_CSR0_P_DATAEND + | MUSB_CSR0_P_SVDRXPKTRDY + | (stall ? MUSB_CSR0_P_SENDSTALL : 0); + + musb_ep_select(musb->mregs, 0); + musb_writew(regs, MUSB_CSR0, ackpend); + + return 0; +} + /* we have an ep0out data packet * Context: caller holds controller lock */ @@ -466,10 +485,13 @@ static void ep0_rxstate(struct musb *musb) void __iomem *regs = musb->control_ep->regs; struct musb_request *request; struct usb_request *req; + struct usb_ep *ep; u16 count, csr; + bool last_packet = false; request = next_ep0_request(musb); req = &request->request; + ep = &request->ep->end_point; /* read packet and ack; or stall because of gadget driver bug: * should have provided the rx buffer before setup() returned. @@ -492,6 +514,7 @@ static void ep0_rxstate(struct musb *musb) if (count < 64 || req->actual == req->length) { musb->ep0_state = MUSB_EP0_STAGE_STATUSIN; csr |= MUSB_CSR0_P_DATAEND; + last_packet = true; } else req = NULL; } else @@ -508,6 +531,10 @@ static void ep0_rxstate(struct musb *musb) return; musb->ackpend = 0; } + + if (last_packet && ep->delayed_status) + return; + musb_ep_select(musb->mregs, 0); musb_writew(regs, MUSB_CSR0, csr); } @@ -991,6 +1018,19 @@ static int musb_g_ep0_dequeue(struct usb_ep *ep, struct usb_request *req) return -EINVAL; } +static int musb_g_ep0_send_response(struct usb_ep *e, bool stall) +{ + struct musb_ep *ep = to_musb_ep(e); + struct musb *musb = ep->musb; + unsigned long flags; + int ret; + + spin_lock_irqsave(&musb->lock, flags); + ret = ep0_send_response(musb, stall); + spin_unlock_irqrestore(&musb->lock, flags); + return ret; +} + static int musb_g_ep0_halt(struct usb_ep *e, int value) { struct musb_ep *ep; @@ -1059,4 +1099,5 @@ const struct usb_ep_ops musb_g_ep0_ops = { .queue = musb_g_ep0_queue, .dequeue = musb_g_ep0_dequeue, .set_halt = musb_g_ep0_halt, + .send_response = musb_g_ep0_send_response, };