Patchwork [v4,5/6] usb: musb: gadget: implement optional explicit status stage

login
register
mail settings
Submitter Paul Elder
Date Jan. 6, 2019, 4:02 p.m.
Message ID <20190106160221.4480-6-paul.elder@ideasonboard.com>
Download mbox | patch
Permalink /patch/693473/
State New
Headers show

Comments

Paul Elder - Jan. 6, 2019, 4:02 p.m.
Implement the mechanism for optional explicit status stage for the MUSB
driver. This allows a function driver to specify what to reply for the
status stage. The functionality for an implicit status stage is
retained.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
v1 Acked-by: Bin Liu <b-liu@ti.com>
---
No change from v3

Changes from v2:
- update call to usb_gadget_control_complete to include status
- since sending STALL from the function driver is now done with
  usb_ep_set_halt, there is no need for the internal ep0_send_response to
  take a stall/ack parameter; remove the parameter and make the function
  only send ack, and remove checking for the status reply in the
  usb_request for the status stage

Changes from v1:
- obvious change to implement v2 mechanism laid out by 4/6 of this
  series (send_response, and musb_g_ep0_send_response function has
  been removed, call to usb_gadget_control_complete has been added)
- ep0_send_response's ack argument has been changed from stall
- last_packet flag in ep0_rxstate has been removed, since it is equal to
  req != NULL

 drivers/usb/musb/musb_gadget.c     |  2 ++
 drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)
Alan Stern - Jan. 6, 2019, 8:03 p.m.
On Sun, 6 Jan 2019, Paul Elder wrote:

> Implement the mechanism for optional explicit status stage for the MUSB
> driver. This allows a function driver to specify what to reply for the
> status stage. The functionality for an implicit status stage is
> retained.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> v1 Acked-by: Bin Liu <b-liu@ti.com>
> ---
> No change from v3
> 
> Changes from v2:
> - update call to usb_gadget_control_complete to include status
> - since sending STALL from the function driver is now done with
>   usb_ep_set_halt, there is no need for the internal ep0_send_response to
>   take a stall/ack parameter; remove the parameter and make the function
>   only send ack, and remove checking for the status reply in the
>   usb_request for the status stage
> 
> Changes from v1:
> - obvious change to implement v2 mechanism laid out by 4/6 of this
>   series (send_response, and musb_g_ep0_send_response function has
>   been removed, call to usb_gadget_control_complete has been added)
> - ep0_send_response's ack argument has been changed from stall
> - last_packet flag in ep0_rxstate has been removed, since it is equal to
>   req != NULL
> 
>  drivers/usb/musb/musb_gadget.c     |  2 ++
>  drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index d3f33f449445..a7a992ab0c9d 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
>  
>  	trace_musb_req_gb(req);
>  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
> +	usb_gadget_control_complete(&musb->g, request->explicit_status,
> +			request->status);

I haven't paid much attention to this part of the patch series, not 
knowing much about musb.  Still, it's clear that 
usb_gadget_control_complete should be called only for transfers on 
ep0.  You need to test the endpoint value.

Another problem: the completion handler may deallocate the request.  
Dereferencing request->expicit_status and request->status would then 
cause errors.  Would it be preferable to call 
usb_gadget_control_complete before usb_gadget_giveback_request?  If 
it gets done that way then the arguments could be simplified: we could 
pass a pointer to the request instead of the separate explicit_status 
and status values.

Alan Stern
Paul Elder - Jan. 8, 2019, 5:49 a.m.
On Sun, Jan 06, 2019 at 03:03:09PM -0500, Alan Stern wrote:
> On Sun, 6 Jan 2019, Paul Elder wrote:
> 
> > Implement the mechanism for optional explicit status stage for the MUSB
> > driver. This allows a function driver to specify what to reply for the
> > status stage. The functionality for an implicit status stage is
> > retained.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > v1 Acked-by: Bin Liu <b-liu@ti.com>
> > ---
> > No change from v3
> > 
> > Changes from v2:
> > - update call to usb_gadget_control_complete to include status
> > - since sending STALL from the function driver is now done with
> >   usb_ep_set_halt, there is no need for the internal ep0_send_response to
> >   take a stall/ack parameter; remove the parameter and make the function
> >   only send ack, and remove checking for the status reply in the
> >   usb_request for the status stage
> > 
> > Changes from v1:
> > - obvious change to implement v2 mechanism laid out by 4/6 of this
> >   series (send_response, and musb_g_ep0_send_response function has
> >   been removed, call to usb_gadget_control_complete has been added)
> > - ep0_send_response's ack argument has been changed from stall
> > - last_packet flag in ep0_rxstate has been removed, since it is equal to
> >   req != NULL
> > 
> >  drivers/usb/musb/musb_gadget.c     |  2 ++
> >  drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index d3f33f449445..a7a992ab0c9d 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
> >  
> >  	trace_musb_req_gb(req);
> >  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
> > +	usb_gadget_control_complete(&musb->g, request->explicit_status,
> > +			request->status);
> 
> I haven't paid much attention to this part of the patch series, not 
> knowing much about musb.  Still, it's clear that 
> usb_gadget_control_complete should be called only for transfers on 
> ep0.  You need to test the endpoint value.

Oh oops, yeah, you're right.

> Another problem: the completion handler may deallocate the request.  
> Dereferencing request->expicit_status and request->status would then 
> cause errors.  Would it be preferable to call 
> usb_gadget_control_complete before usb_gadget_giveback_request?  If 
> it gets done that way then the arguments could be simplified: we could 
> pass a pointer to the request instead of the separate explicit_status 
> and status values.

I thought that usb_gadget_control_complete needs to check the status of
the request that was given back. Doesn't that mean that
usb_gadget_giveback_request must be called first?

I was thinking that maybe we could save explicit_status before calling
usb_gadget_giveback_request, and if request is still valid then we can
pull status from it otherwise use 0, but then would that be considered
adding complexity to UDCs that want to implement optional status stage
delay? Or add a wrapper function?

On the other hand, if we do put usb_gadget_control_complete before
usb_gadget_giveback_request, then the control transfer would complete
before the function driver has a chance to complete, but if the function
driver wanted to intervene/determine the status stage then it would have
gone through the new mechanism that we're making here. So it could be
fine to switch the order. My tests for it work too, so I guess we'll go
with usb_gadget_control_complete before usb_gadget_giveback_request
then. In that case usb_gadget_control_complete doesn't need to check the
status of the request, since there isn't any, right?


Thanks,

Paul Elder
Alan Stern - Jan. 8, 2019, 3:32 p.m.
On Tue, 8 Jan 2019, Paul Elder wrote:

> On Sun, Jan 06, 2019 at 03:03:09PM -0500, Alan Stern wrote:
> > On Sun, 6 Jan 2019, Paul Elder wrote:
> > 
> > > Implement the mechanism for optional explicit status stage for the MUSB
> > > driver. This allows a function driver to specify what to reply for the
> > > status stage. The functionality for an implicit status stage is
> > > retained.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > v1 Acked-by: Bin Liu <b-liu@ti.com>
> > > ---
> > > No change from v3
> > > 
> > > Changes from v2:
> > > - update call to usb_gadget_control_complete to include status
> > > - since sending STALL from the function driver is now done with
> > >   usb_ep_set_halt, there is no need for the internal ep0_send_response to
> > >   take a stall/ack parameter; remove the parameter and make the function
> > >   only send ack, and remove checking for the status reply in the
> > >   usb_request for the status stage
> > > 
> > > Changes from v1:
> > > - obvious change to implement v2 mechanism laid out by 4/6 of this
> > >   series (send_response, and musb_g_ep0_send_response function has
> > >   been removed, call to usb_gadget_control_complete has been added)
> > > - ep0_send_response's ack argument has been changed from stall
> > > - last_packet flag in ep0_rxstate has been removed, since it is equal to
> > >   req != NULL
> > > 
> > >  drivers/usb/musb/musb_gadget.c     |  2 ++
> > >  drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++
> > >  2 files changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > > index d3f33f449445..a7a992ab0c9d 100644
> > > --- a/drivers/usb/musb/musb_gadget.c
> > > +++ b/drivers/usb/musb/musb_gadget.c
> > > @@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
> > >  
> > >  	trace_musb_req_gb(req);
> > >  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
> > > +	usb_gadget_control_complete(&musb->g, request->explicit_status,
> > > +			request->status);
> > 
> > I haven't paid much attention to this part of the patch series, not 
> > knowing much about musb.  Still, it's clear that 
> > usb_gadget_control_complete should be called only for transfers on 
> > ep0.  You need to test the endpoint value.
> 
> Oh oops, yeah, you're right.
> 
> > Another problem: the completion handler may deallocate the request.  
> > Dereferencing request->expicit_status and request->status would then 
> > cause errors.  Would it be preferable to call 
> > usb_gadget_control_complete before usb_gadget_giveback_request?  If 
> > it gets done that way then the arguments could be simplified: we could 
> > pass a pointer to the request instead of the separate explicit_status 
> > and status values.
> 
> I thought that usb_gadget_control_complete needs to check the status of
> the request that was given back. Doesn't that mean that
> usb_gadget_giveback_request must be called first?

You misunderstand.  req->status has already been determined by the time 
usb_gadget_giveback_request is called.  It does not get set by that 
call.

> I was thinking that maybe we could save explicit_status before calling
> usb_gadget_giveback_request, and if request is still valid then we can
> pull status from it otherwise use 0, but then would that be considered
> adding complexity to UDCs that want to implement optional status stage
> delay? Or add a wrapper function?
> 
> On the other hand, if we do put usb_gadget_control_complete before
> usb_gadget_giveback_request, then the control transfer would complete
> before the function driver has a chance to complete, but if the function
> driver wanted to intervene/determine the status stage then it would have
> gone through the new mechanism that we're making here. So it could be
> fine to switch the order. My tests for it work too, so I guess we'll go
> with usb_gadget_control_complete before usb_gadget_giveback_request
> then. In that case usb_gadget_control_complete doesn't need to check the
> status of the request, since there isn't any, right?

Wrong -- the status has already been set by that time.

Alan Stern

Patch

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index d3f33f449445..a7a992ab0c9d 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -145,6 +145,8 @@  __acquires(ep->musb->lock)
 
 	trace_musb_req_gb(req);
 	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
+	usb_gadget_control_complete(&musb->g, request->explicit_status,
+			request->status);
 	spin_lock(&musb->lock);
 	ep->busy = busy;
 }
diff --git a/drivers/usb/musb/musb_gadget_ep0.c b/drivers/usb/musb/musb_gadget_ep0.c
index 91a5027b5c1f..bbce8a9d77e4 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -458,6 +458,23 @@  __acquires(musb->lock)
 	return handled;
 }
 
+static int ep0_send_ack(struct musb *musb)
+{
+	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;
+
+	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
  */
@@ -504,10 +521,13 @@  static void ep0_rxstate(struct musb *musb)
 	if (req) {
 		musb->ackpend = csr;
 		musb_g_ep0_giveback(musb, req);
+		if (req->explicit_status)
+			return;
 		if (!musb->ackpend)
 			return;
 		musb->ackpend = 0;
 	}
+
 	musb_ep_select(musb->mregs, 0);
 	musb_writew(regs, MUSB_CSR0, csr);
 }
@@ -939,6 +959,9 @@  musb_g_ep0_queue(struct usb_ep *e, struct usb_request *r, gfp_t gfp_flags)
 	case MUSB_EP0_STAGE_ACKWAIT:	/* zero-length data */
 		status = 0;
 		break;
+	case MUSB_EP0_STAGE_STATUSIN:
+		status = ep0_send_ack(musb);
+		goto cleanup;
 	default:
 		musb_dbg(musb, "ep0 request queued in state %d",
 				musb->ep0_state);