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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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);
-
