Hello,
This patch set contains few small bugfixes found in usb gadget functions
and UDC drivers. The most important is the [1] as it fixes bug causing
BUG_ON() in f_fs driver. Remaining patches contain minor fixes.
Best regards,
Robert Baldyga
[1] usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails
Robert Baldyga (5):
usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails
usb: gadget: midi: avoid redundant f_midi_set_alt() call
usb: isp1760: udc: add missing usb_ep_set_maxpacket_limit()
staging: emxx_udc: add missing usb_ep_set_maxpacket_limit()
usb: gadget: atmel_usba_udc: add missing ret value check
drivers/staging/emxx_udc/emxx_udc.c | 3 ++-
drivers/usb/gadget/function/f_fs.c | 10 +++++++++-
drivers/usb/gadget/function/f_midi.c | 4 ++++
drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++++
drivers/usb/isp1760/isp1760-udc.c | 4 ++--
5 files changed, 21 insertions(+), 4 deletions(-)
--
1.9.1
Function ffs_do_functionfs_bind() calls functionfs_bind() which allocates
usb request and increments refcounts. These things needs to be cleaned
up by if further steps of initialization fail by calling functionfs_unbind().
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/usb/gadget/function/f_fs.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 6e7be91..966b214 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2897,11 +2897,19 @@ static int ffs_func_bind(struct usb_configuration *c,
struct usb_function *f)
{
struct f_fs_opts *ffs_opts = ffs_do_functionfs_bind(f, c);
+ struct ffs_function *func = ffs_func_from_usb(f);
+ int ret;
if (IS_ERR(ffs_opts))
return PTR_ERR(ffs_opts);
- return _ffs_func_bind(c, f);
+ ret = _ffs_func_bind(c, f);
+ if (ret) {
+ ffs_opts->refcnt--;
+ functionfs_unbind(func->ffs);
+ }
+
+ return ret;
}
--
1.9.1
Function midi registers two interfaces with single set_alt() function
which means that f_midi_set_alt() is called twice when configuration
is set. That means that endpoint initialization and ep request allocation
is done two times. To avoid this problem we do such things only once,
for interface number 1 (MIDI Streaming interface).
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 6316aa5..4cef222 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -329,6 +329,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
unsigned i;
int err;
+ /* For Control Device interface we do nothing */
+ if (intf == 0)
+ return 0;
+
err = f_midi_start_ep(midi, f, midi->in_ep);
if (err)
return err;
--
1.9.1
Should use usb_ep_set_maxpacket_limit instead of setting
maxpacket value manually.
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/usb/isp1760/isp1760-udc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
index 18ebf5b1..3699962 100644
--- a/drivers/usb/isp1760/isp1760-udc.c
+++ b/drivers/usb/isp1760/isp1760-udc.c
@@ -1382,11 +1382,11 @@ static void isp1760_udc_init_eps(struct isp1760_udc *udc)
* This fits in the 8kB FIFO without double-buffering.
*/
if (ep_num == 0) {
- ep->ep.maxpacket = 64;
+ usb_ep_set_maxpacket_limit(&ep->ep, 64);
ep->maxpacket = 64;
udc->gadget.ep0 = &ep->ep;
} else {
- ep->ep.maxpacket = 512;
+ usb_ep_set_maxpacket_limit(&ep->ep, 512);
ep->maxpacket = 0;
list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
}
--
1.9.1
Should use usb_ep_set_maxpacket_limit instead of setting
maxpacket value manually.
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/staging/emxx_udc/emxx_udc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
index 4178d96..f50bf5d 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -3203,7 +3203,8 @@ static void __init nbu2ss_drv_ep_init(struct nbu2ss_udc *udc)
ep->ep.name = gp_ep_name[i];
ep->ep.ops = &nbu2ss_ep_ops;
- ep->ep.maxpacket = (i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE);
+ usb_ep_set_maxpacket_limit(&ep->ep,
+ (i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE));
list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
INIT_LIST_HEAD(&ep->queue);
--
1.9.1
Add missing return value check. In case of error print debug message
and return error code.
Signed-off-by: Robert Baldyga <[email protected]>
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 4095cce0..37d414e 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1989,6 +1989,10 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
ret = of_property_read_string(pp, "name", &name);
+ if (ret) {
+ dev_err(&pdev->dev, "of_probe: name error(%d)\n", ret);
+ goto err;
+ }
ep->ep.name = name;
ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
--
1.9.1
Hello.
On 7/7/2015 5:02 PM, Robert Baldyga wrote:
> Should use usb_ep_set_maxpacket_limit instead of setting
> maxpacket value manually.
> Signed-off-by: Robert Baldyga <[email protected]>
> ---
> drivers/staging/emxx_udc/emxx_udc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> index 4178d96..f50bf5d 100644
> --- a/drivers/staging/emxx_udc/emxx_udc.c
> +++ b/drivers/staging/emxx_udc/emxx_udc.c
> @@ -3203,7 +3203,8 @@ static void __init nbu2ss_drv_ep_init(struct nbu2ss_udc *udc)
> ep->ep.name = gp_ep_name[i];
> ep->ep.ops = &nbu2ss_ep_ops;
>
> - ep->ep.maxpacket = (i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE);
> + usb_ep_set_maxpacket_limit(&ep->ep,
> + (i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE));
Inner () not needed.
[...]
WBR, Sergei
On Tue, Jul 07, 2015 at 04:02:49PM +0200, Robert Baldyga wrote:
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 6e7be91..966b214 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2897,11 +2897,19 @@ static int ffs_func_bind(struct usb_configuration *c,
> struct usb_function *f)
> {
> struct f_fs_opts *ffs_opts = ffs_do_functionfs_bind(f, c);
> + struct ffs_function *func = ffs_func_from_usb(f);
> + int ret;
>
> if (IS_ERR(ffs_opts))
> return PTR_ERR(ffs_opts);
>
> - return _ffs_func_bind(c, f);
> + ret = _ffs_func_bind(c, f);
> + if (ret) {
> + ffs_opts->refcnt--;
Wait, why are we decrementing here? ffs_func_unbind() already has a
decrement so this looks like a bug to me. Add a comment if it's really
needed.
> + functionfs_unbind(func->ffs);
regards,
dan carpenter
On Tue, Jul 07, 2015 at 04:02:51PM +0200, Robert Baldyga wrote:
> Should use usb_ep_set_maxpacket_limit instead of setting
> maxpacket value manually.
>
This is a behavior change, right, since now we're setting
ep->ep.maxpacket and ep->ep.maxpacket_limit where before we only set
the first. I don't understand. This patch description is not clear.
regards,
dan carpenter
> Signed-off-by: Robert Baldyga <[email protected]>
> ---
> drivers/usb/isp1760/isp1760-udc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
> index 18ebf5b1..3699962 100644
> --- a/drivers/usb/isp1760/isp1760-udc.c
> +++ b/drivers/usb/isp1760/isp1760-udc.c
> @@ -1382,11 +1382,11 @@ static void isp1760_udc_init_eps(struct isp1760_udc *udc)
> * This fits in the 8kB FIFO without double-buffering.
> */
> if (ep_num == 0) {
> - ep->ep.maxpacket = 64;
> + usb_ep_set_maxpacket_limit(&ep->ep, 64);
> ep->maxpacket = 64;
> udc->gadget.ep0 = &ep->ep;
> } else {
> - ep->ep.maxpacket = 512;
> + usb_ep_set_maxpacket_limit(&ep->ep, 512);
> ep->maxpacket = 0;
> list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
> }
On Tue, Jul 07, 2015 at 04:02:52PM +0200, Robert Baldyga wrote:
> Should use usb_ep_set_maxpacket_limit instead of setting
> maxpacket value manually.
>
Same question.
regards,
dan carpenter
On 07/07/2015 04:53 PM, Dan Carpenter wrote:
> On Tue, Jul 07, 2015 at 04:02:49PM +0200, Robert Baldyga wrote:
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 6e7be91..966b214 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -2897,11 +2897,19 @@ static int ffs_func_bind(struct usb_configuration *c,
>> struct usb_function *f)
>> {
>> struct f_fs_opts *ffs_opts = ffs_do_functionfs_bind(f, c);
>> + struct ffs_function *func = ffs_func_from_usb(f);
>> + int ret;
>>
>> if (IS_ERR(ffs_opts))
>> return PTR_ERR(ffs_opts);
>>
>> - return _ffs_func_bind(c, f);
>> + ret = _ffs_func_bind(c, f);
>> + if (ret) {
>> + ffs_opts->refcnt--;
>
> Wait, why are we decrementing here? ffs_func_unbind() already has a
> decrement so this looks like a bug to me. Add a comment if it's really
> needed.
Decrement is done in ffs_func_unbind() which is not called in this error
path. But after all I see another problem here, because we shouldn't
call functionfs_unbind() if refcnt after decrement is not equal zero. I
will fix it.
>
>> + functionfs_unbind(func->ffs);
Thanks,
Robert Baldyga
On 07/07/2015 05:01 PM, Dan Carpenter wrote:
> On Tue, Jul 07, 2015 at 04:02:51PM +0200, Robert Baldyga wrote:
>> Should use usb_ep_set_maxpacket_limit instead of setting
>> maxpacket value manually.
>>
>
> This is a behavior change, right, since now we're setting
> ep->ep.maxpacket and ep->ep.maxpacket_limit where before we only set
> the first. I don't understand. This patch description is not clear.
Since maxpacket_limit was introduced all UDC drivers should set it, as
it is used by epautoconf. The ep.maxpacket_limit contains actual maximum
maxpacket value supported by hardware, ep.maxpacket is set only for
backward compatibility. Hence setting maxpacket manually instead of
using usb_ep_set_maxpacket_limit() is actually a bug.
I will add better description to commit message.
>
>> Signed-off-by: Robert Baldyga <[email protected]>
>> ---
>> drivers/usb/isp1760/isp1760-udc.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
>> index 18ebf5b1..3699962 100644
>> --- a/drivers/usb/isp1760/isp1760-udc.c
>> +++ b/drivers/usb/isp1760/isp1760-udc.c
>> @@ -1382,11 +1382,11 @@ static void isp1760_udc_init_eps(struct isp1760_udc *udc)
>> * This fits in the 8kB FIFO without double-buffering.
>> */
>> if (ep_num == 0) {
>> - ep->ep.maxpacket = 64;
>> + usb_ep_set_maxpacket_limit(&ep->ep, 64);
>> ep->maxpacket = 64;
>> udc->gadget.ep0 = &ep->ep;
>> } else {
>> - ep->ep.maxpacket = 512;
>> + usb_ep_set_maxpacket_limit(&ep->ep, 512);
>> ep->maxpacket = 0;
>> list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>> }
Thanks,
Robert Baldyga
On Tue, Jul 07, 2015 at 06:00:27PM +0200, Robert Baldyga wrote:
> Decrement is done in ffs_func_unbind() which is not called in this
> error path.
Oh. Duh. I got functionfs_unbind() and ffs_func_unbind() mixed up.
Sorry.
regards,
dan carpenter
Le 07/07/2015 16:02, Robert Baldyga a ?crit :
> Add missing return value check. In case of error print debug message
> and return error code.
>
> Signed-off-by: Robert Baldyga <[email protected]>
Yes, it's indeed missing:
Acked-by: Nicolas Ferre <[email protected]>
Thanks, bye.
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 4095cce0..37d414e 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1989,6 +1989,10 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
> ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
>
> ret = of_property_read_string(pp, "name", &name);
> + if (ret) {
> + dev_err(&pdev->dev, "of_probe: name error(%d)\n", ret);
> + goto err;
> + }
> ep->ep.name = name;
>
> ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
>
--
Nicolas Ferre