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=140917381611947=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=140917381611947=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
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-usbm=140917381611947w=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
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-usbm=140917381611947w=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
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=140917381611947=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
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 so...@merica.cz
---
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-usbm=140917381611947w=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
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 so...@merica.cz
---
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
