Re: [PATCH v5 1/3] usb: gadget: Refactor request completion
On Tue, 23 Sep 2014, Michal Sojka wrote: > >> +/** > >> + * usb_gadget_giveback_request - give the request back to the gadget layer > >> + * Context: in_interrupt() > >> + * > >> + * This is called by device controller drivers in order to return the > >> + * completed request back to the gadget layer. > >> + */ > >> +void usb_gadget_giveback_request(struct usb_ep *ep, > >> + struct usb_request *req) > >> +{ > >> + if (likely(req->complete)) > >> + req->complete(ep, req); > >> + else > >> + pr_err("%s : req->complete must not be NULL\n", __func__); > > > > let it Oops. We require ->complete to be valid, if there's any gadget > > driver not setting ->complete, it deserves to oops so we can the > > error. > > The Oops was there before, but I removed it because greg k-h didn't want > it. See http://marc.info/?l=linux-usb&m=140917381611947&w=2. Do you > still want the oops here? Greg didn't want you to add a BUG() statement, whereas Felipe wants you to leave out the "if" test and pr_err(). Just omit both, and everyone will be satisfied. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] usb: gadget: Refactor request completion
Hi, On Tue, Sep 23, 2014 at 10:09:22AM +0200, Michal Sojka wrote: > >> +/** > >> + * usb_gadget_giveback_request - give the request back to the gadget layer > >> + * Context: in_interrupt() > >> + * > >> + * This is called by device controller drivers in order to return the > >> + * completed request back to the gadget layer. > >> + */ > >> +void usb_gadget_giveback_request(struct usb_ep *ep, > >> + struct usb_request *req) > >> +{ > >> + if (likely(req->complete)) > >> + req->complete(ep, req); > >> + else > >> + pr_err("%s : req->complete must not be NULL\n", __func__); > > > > let it Oops. We require ->complete to be valid, if there's any gadget > > driver not setting ->complete, it deserves to oops so we can the > > error. > > The Oops was there before, but I removed it because greg k-h didn't want > it. See http://marc.info/?l=linux-usb&m=140917381611947&w=2. Do you > still want the oops here? you don't need a BUG_ON(), just call req->complete() directly without any checks, if it's not valid, it _will_ oops. ->complete() is mandatory and anybody sending requests without complete deserve to oops. -- balbi signature.asc Description: Digital signature
Re: [PATCH v5 1/3] usb: gadget: Refactor request completion
Dear Felipe, On Wed, Sep 17 2014, Felipe Balbi wrote: > On Wed, Sep 17, 2014 at 09:21:11AM +0200, Michal Sojka wrote: >> All USB peripheral controller drivers called completion routines >> directly. This patch moves the completion call from drivers to >> usb_gadget_giveback_request(), in order to have a place where common >> functionality can be added. >> >> All places in drivers/usb/ matching "[-.]complete(" were replaced with a >> call to usb_gadget_giveback_request(). This was compile-tested with all >> ARM drivers enabled and runtime-tested for musb. >> >> Signed-off-by: Michal Sojka >> --- >> drivers/usb/chipidea/udc.c | 6 +++--- >> drivers/usb/dwc2/gadget.c | 6 +++--- >> drivers/usb/dwc3/gadget.c | 2 +- >> drivers/usb/gadget/udc/amd5536udc.c | 2 +- >> drivers/usb/gadget/udc/at91_udc.c | 2 +- >> drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++-- >> drivers/usb/gadget/udc/bcm63xx_udc.c| 2 +- >> drivers/usb/gadget/udc/dummy_hcd.c | 10 +- >> drivers/usb/gadget/udc/fotg210-udc.c| 2 +- >> drivers/usb/gadget/udc/fsl_qe_udc.c | 6 +- >> drivers/usb/gadget/udc/fsl_udc_core.c | 6 ++ >> drivers/usb/gadget/udc/fusb300_udc.c| 2 +- >> drivers/usb/gadget/udc/goku_udc.c | 2 +- >> drivers/usb/gadget/udc/gr_udc.c | 2 +- >> drivers/usb/gadget/udc/lpc32xx_udc.c| 2 +- >> drivers/usb/gadget/udc/m66592-udc.c | 2 +- >> drivers/usb/gadget/udc/mv_u3d_core.c| 8 ++-- >> drivers/usb/gadget/udc/mv_udc_core.c| 8 ++-- >> drivers/usb/gadget/udc/net2272.c| 2 +- >> drivers/usb/gadget/udc/net2280.c| 2 +- >> drivers/usb/gadget/udc/omap_udc.c | 2 +- >> drivers/usb/gadget/udc/pch_udc.c| 2 +- >> drivers/usb/gadget/udc/pxa25x_udc.c | 2 +- >> drivers/usb/gadget/udc/pxa27x_udc.c | 2 +- >> drivers/usb/gadget/udc/r8a66597-udc.c | 2 +- >> drivers/usb/gadget/udc/s3c-hsudc.c | 3 +-- >> drivers/usb/gadget/udc/s3c2410_udc.c| 2 +- >> drivers/usb/gadget/udc/udc-core.c | 19 +++ >> drivers/usb/musb/musb_gadget.c | 2 +- >> drivers/usb/renesas_usbhs/mod_gadget.c | 2 +- >> include/linux/usb/gadget.h | 8 > > I would rather split this into several patches, btw. With the > introduction of usb_gadget_giveback_request() being the first one in the > series. It's easier to review that way. This would be no problem. >> diff --git a/drivers/usb/gadget/udc/udc-core.c >> b/drivers/usb/gadget/udc/udc-core.c >> index b0d9817..29789f1 100644 >> --- a/drivers/usb/gadget/udc/udc-core.c >> +++ b/drivers/usb/gadget/udc/udc-core.c >> @@ -106,6 +106,25 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request); >> >> /* >> - */ >> >> +/** >> + * usb_gadget_giveback_request - give the request back to the gadget layer >> + * Context: in_interrupt() >> + * >> + * This is called by device controller drivers in order to return the >> + * completed request back to the gadget layer. >> + */ >> +void usb_gadget_giveback_request(struct usb_ep *ep, >> +struct usb_request *req) >> +{ >> +if (likely(req->complete)) >> +req->complete(ep, req); >> +else >> +pr_err("%s : req->complete must not be NULL\n", __func__); > > let it Oops. We require ->complete to be valid, if there's any gadget > driver not setting ->complete, it deserves to oops so we can the > error. The Oops was there before, but I removed it because greg k-h didn't want it. See http://marc.info/?l=linux-usb&m=140917381611947&w=2. Do you still want the oops here? -Michal -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] usb: gadget: Refactor request completion
On Wed, Sep 17, 2014 at 09:21:11AM +0200, Michal Sojka wrote: > All USB peripheral controller drivers called completion routines > directly. This patch moves the completion call from drivers to > usb_gadget_giveback_request(), in order to have a place where common > functionality can be added. > > All places in drivers/usb/ matching "[-.]complete(" were replaced with a > call to usb_gadget_giveback_request(). This was compile-tested with all > ARM drivers enabled and runtime-tested for musb. > > Signed-off-by: Michal Sojka > --- > drivers/usb/chipidea/udc.c | 6 +++--- > drivers/usb/dwc2/gadget.c | 6 +++--- > drivers/usb/dwc3/gadget.c | 2 +- > drivers/usb/gadget/udc/amd5536udc.c | 2 +- > drivers/usb/gadget/udc/at91_udc.c | 2 +- > drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++-- > drivers/usb/gadget/udc/bcm63xx_udc.c| 2 +- > drivers/usb/gadget/udc/dummy_hcd.c | 10 +- > drivers/usb/gadget/udc/fotg210-udc.c| 2 +- > drivers/usb/gadget/udc/fsl_qe_udc.c | 6 +- > drivers/usb/gadget/udc/fsl_udc_core.c | 6 ++ > drivers/usb/gadget/udc/fusb300_udc.c| 2 +- > drivers/usb/gadget/udc/goku_udc.c | 2 +- > drivers/usb/gadget/udc/gr_udc.c | 2 +- > drivers/usb/gadget/udc/lpc32xx_udc.c| 2 +- > drivers/usb/gadget/udc/m66592-udc.c | 2 +- > drivers/usb/gadget/udc/mv_u3d_core.c| 8 ++-- > drivers/usb/gadget/udc/mv_udc_core.c| 8 ++-- > drivers/usb/gadget/udc/net2272.c| 2 +- > drivers/usb/gadget/udc/net2280.c| 2 +- > drivers/usb/gadget/udc/omap_udc.c | 2 +- > drivers/usb/gadget/udc/pch_udc.c| 2 +- > drivers/usb/gadget/udc/pxa25x_udc.c | 2 +- > drivers/usb/gadget/udc/pxa27x_udc.c | 2 +- > drivers/usb/gadget/udc/r8a66597-udc.c | 2 +- > drivers/usb/gadget/udc/s3c-hsudc.c | 3 +-- > drivers/usb/gadget/udc/s3c2410_udc.c| 2 +- > drivers/usb/gadget/udc/udc-core.c | 19 +++ > drivers/usb/musb/musb_gadget.c | 2 +- > drivers/usb/renesas_usbhs/mod_gadget.c | 2 +- > include/linux/usb/gadget.h | 8 I would rather split this into several patches, btw. With the introduction of usb_gadget_giveback_request() being the first one in the series. It's easier to review that way. > diff --git a/drivers/usb/gadget/udc/udc-core.c > b/drivers/usb/gadget/udc/udc-core.c > index b0d9817..29789f1 100644 > --- a/drivers/usb/gadget/udc/udc-core.c > +++ b/drivers/usb/gadget/udc/udc-core.c > @@ -106,6 +106,25 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request); > > /* - > */ > > +/** > + * usb_gadget_giveback_request - give the request back to the gadget layer > + * Context: in_interrupt() > + * > + * This is called by device controller drivers in order to return the > + * completed request back to the gadget layer. > + */ > +void usb_gadget_giveback_request(struct usb_ep *ep, > + struct usb_request *req) > +{ > + if (likely(req->complete)) > + req->complete(ep, req); > + else > + pr_err("%s : req->complete must not be NULL\n", __func__); let it Oops. We require ->complete to be valid, if there's any gadget driver not setting ->complete, it deserves to oops so we can the error. -- balbi signature.asc Description: Digital signature
[PATCH v5 1/3] usb: gadget: Refactor request completion
All USB peripheral controller drivers called completion routines directly. This patch moves the completion call from drivers to usb_gadget_giveback_request(), in order to have a place where common functionality can be added. All places in drivers/usb/ matching "[-.]complete(" were replaced with a call to usb_gadget_giveback_request(). This was compile-tested with all ARM drivers enabled and runtime-tested for musb. Signed-off-by: Michal Sojka --- drivers/usb/chipidea/udc.c | 6 +++--- drivers/usb/dwc2/gadget.c | 6 +++--- drivers/usb/dwc3/gadget.c | 2 +- drivers/usb/gadget/udc/amd5536udc.c | 2 +- drivers/usb/gadget/udc/at91_udc.c | 2 +- drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++-- drivers/usb/gadget/udc/bcm63xx_udc.c| 2 +- drivers/usb/gadget/udc/dummy_hcd.c | 10 +- drivers/usb/gadget/udc/fotg210-udc.c| 2 +- drivers/usb/gadget/udc/fsl_qe_udc.c | 6 +- drivers/usb/gadget/udc/fsl_udc_core.c | 6 ++ drivers/usb/gadget/udc/fusb300_udc.c| 2 +- drivers/usb/gadget/udc/goku_udc.c | 2 +- drivers/usb/gadget/udc/gr_udc.c | 2 +- drivers/usb/gadget/udc/lpc32xx_udc.c| 2 +- drivers/usb/gadget/udc/m66592-udc.c | 2 +- drivers/usb/gadget/udc/mv_u3d_core.c| 8 ++-- drivers/usb/gadget/udc/mv_udc_core.c| 8 ++-- drivers/usb/gadget/udc/net2272.c| 2 +- drivers/usb/gadget/udc/net2280.c| 2 +- drivers/usb/gadget/udc/omap_udc.c | 2 +- drivers/usb/gadget/udc/pch_udc.c| 2 +- drivers/usb/gadget/udc/pxa25x_udc.c | 2 +- drivers/usb/gadget/udc/pxa27x_udc.c | 2 +- drivers/usb/gadget/udc/r8a66597-udc.c | 2 +- drivers/usb/gadget/udc/s3c-hsudc.c | 3 +-- drivers/usb/gadget/udc/s3c2410_udc.c| 2 +- drivers/usb/gadget/udc/udc-core.c | 19 +++ drivers/usb/musb/musb_gadget.c | 2 +- drivers/usb/renesas_usbhs/mod_gadget.c | 2 +- include/linux/usb/gadget.h | 8 31 files changed, 68 insertions(+), 56 deletions(-) diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index b8125aa..0444d3f 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -627,7 +627,7 @@ __acquires(hwep->lock) if (hwreq->req.complete != NULL) { spin_unlock(hwep->lock); - hwreq->req.complete(&hwep->ep, &hwreq->req); + usb_gadget_giveback_request(&hwep->ep, &hwreq->req); spin_lock(hwep->lock); } } @@ -922,7 +922,7 @@ __acquires(hwep->lock) if ((hwep->type == USB_ENDPOINT_XFER_CONTROL) && hwreq->req.length) hweptemp = hwep->ci->ep0in; - hwreq->req.complete(&hweptemp->ep, &hwreq->req); + usb_gadget_giveback_request(&hweptemp->ep, &hwreq->req); spin_lock(hwep->lock); } } @@ -1347,7 +1347,7 @@ static int ep_dequeue(struct usb_ep *ep, struct usb_request *req) if (hwreq->req.complete != NULL) { spin_unlock(hwep->lock); - hwreq->req.complete(&hwep->ep, &hwreq->req); + usb_gadget_giveback_request(&hwep->ep, &hwreq->req); spin_lock(hwep->lock); } diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 0ba9c33..5a524a6 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -987,8 +987,8 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg, hs_req = ep->req; ep->req = NULL; list_del_init(&hs_req->queue); - hs_req->req.complete(&ep->ep, -&hs_req->req); + usb_gadget_giveback_request(&ep->ep, + &hs_req->req); } /* If we have pending request, then start it */ @@ -1245,7 +1245,7 @@ static void s3c_hsotg_complete_request(struct s3c_hsotg *hsotg, if (hs_req->req.complete) { spin_unlock(&hsotg->lock); - hs_req->req.complete(&hs_ep->ep, &hs_req->req); + usb_gadget_giveback_request(&hs_ep->ep, &hs_req->req); spin_lock(&hsotg->lock); } diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 349cacc..b4b7a6b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -268,7 +268,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req, req->request.length, status); spin_unlock(&dwc->lock); -
[PATCH v5 1/3] usb: gadget: Refactor request completion
All USB peripheral controller drivers called completion routines directly. This patch moves the completion call from drivers to usb_gadget_giveback_request(), in order to have a place where common functionality can be added. All places in drivers/usb/ matching "[-.]complete(" were replaced with a call to usb_gadget_giveback_request(). This was compile-tested with all ARM drivers enabled and runtime-tested for musb. Signed-off-by: Michal Sojka --- drivers/usb/chipidea/udc.c | 6 +++--- drivers/usb/dwc2/gadget.c | 6 +++--- drivers/usb/dwc3/gadget.c | 2 +- drivers/usb/gadget/udc/amd5536udc.c | 2 +- drivers/usb/gadget/udc/at91_udc.c | 2 +- drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++-- drivers/usb/gadget/udc/bcm63xx_udc.c| 2 +- drivers/usb/gadget/udc/dummy_hcd.c | 10 +- drivers/usb/gadget/udc/fotg210-udc.c| 2 +- drivers/usb/gadget/udc/fsl_qe_udc.c | 6 +- drivers/usb/gadget/udc/fsl_udc_core.c | 6 ++ drivers/usb/gadget/udc/fusb300_udc.c| 2 +- drivers/usb/gadget/udc/goku_udc.c | 2 +- drivers/usb/gadget/udc/gr_udc.c | 2 +- drivers/usb/gadget/udc/lpc32xx_udc.c| 2 +- drivers/usb/gadget/udc/m66592-udc.c | 2 +- drivers/usb/gadget/udc/mv_u3d_core.c| 8 ++-- drivers/usb/gadget/udc/mv_udc_core.c| 8 ++-- drivers/usb/gadget/udc/net2272.c| 2 +- drivers/usb/gadget/udc/net2280.c| 2 +- drivers/usb/gadget/udc/omap_udc.c | 2 +- drivers/usb/gadget/udc/pch_udc.c| 2 +- drivers/usb/gadget/udc/pxa25x_udc.c | 2 +- drivers/usb/gadget/udc/pxa27x_udc.c | 2 +- drivers/usb/gadget/udc/r8a66597-udc.c | 2 +- drivers/usb/gadget/udc/s3c-hsudc.c | 3 +-- drivers/usb/gadget/udc/s3c2410_udc.c| 2 +- drivers/usb/gadget/udc/udc-core.c | 19 +++ drivers/usb/musb/musb_gadget.c | 2 +- drivers/usb/renesas_usbhs/mod_gadget.c | 2 +- include/linux/usb/gadget.h | 8 31 files changed, 68 insertions(+), 56 deletions(-) diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index b8125aa..0444d3f 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -627,7 +627,7 @@ __acquires(hwep->lock) if (hwreq->req.complete != NULL) { spin_unlock(hwep->lock); - hwreq->req.complete(&hwep->ep, &hwreq->req); + usb_gadget_giveback_request(&hwep->ep, &hwreq->req); spin_lock(hwep->lock); } } @@ -922,7 +922,7 @@ __acquires(hwep->lock) if ((hwep->type == USB_ENDPOINT_XFER_CONTROL) && hwreq->req.length) hweptemp = hwep->ci->ep0in; - hwreq->req.complete(&hweptemp->ep, &hwreq->req); + usb_gadget_giveback_request(&hweptemp->ep, &hwreq->req); spin_lock(hwep->lock); } } @@ -1347,7 +1347,7 @@ static int ep_dequeue(struct usb_ep *ep, struct usb_request *req) if (hwreq->req.complete != NULL) { spin_unlock(hwep->lock); - hwreq->req.complete(&hwep->ep, &hwreq->req); + usb_gadget_giveback_request(&hwep->ep, &hwreq->req); spin_lock(hwep->lock); } diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 0ba9c33..5a524a6 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -987,8 +987,8 @@ static int s3c_hsotg_process_req_feature(struct s3c_hsotg *hsotg, hs_req = ep->req; ep->req = NULL; list_del_init(&hs_req->queue); - hs_req->req.complete(&ep->ep, -&hs_req->req); + usb_gadget_giveback_request(&ep->ep, + &hs_req->req); } /* If we have pending request, then start it */ @@ -1245,7 +1245,7 @@ static void s3c_hsotg_complete_request(struct s3c_hsotg *hsotg, if (hs_req->req.complete) { spin_unlock(&hsotg->lock); - hs_req->req.complete(&hs_ep->ep, &hs_req->req); + usb_gadget_giveback_request(&hs_ep->ep, &hs_req->req); spin_lock(&hsotg->lock); } diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 349cacc..b4b7a6b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -268,7 +268,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req, req->request.length, status); spin_unlock(&dwc->lock); -