[PATCH 0/5] usb: gadget: miscellaneous fixes

[PATCH 0/5] usb: gadget: miscellaneous fixes

[Robert Baldyga]

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

[PATCH 1/5] usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails

[Robert Baldyga]

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 <r.baldyga@samsung.com>
---
 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

[PATCH 1/5] usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails

[Dan Carpenter]

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

[PATCH 1/5] usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails

[Robert Baldyga]

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

[PATCH 1/5] usb: gadget: ffs: call functionfs_unbind() if _ffs_func_bind() fails

[Dan Carpenter]

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

[PATCH 2/5] usb: gadget: midi: avoid redundant f_midi_set_alt() call

[Robert Baldyga]

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 <r.baldyga@samsung.com>
---
 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

[PATCH 3/5] usb: isp1760: udc: add missing usb_ep_set_maxpacket_limit()

[Robert Baldyga]

Should use usb_ep_set_maxpacket_limit instead of setting
maxpacket value manually.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 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

[PATCH 3/5] usb: isp1760: udc: add missing usb_ep_set_maxpacket_limit()

[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 <r.baldyga@samsung.com>
> ---
>  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);
>  		}

[PATCH 3/5] usb: isp1760: udc: add missing usb_ep_set_maxpacket_limit()

[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 <r.baldyga@samsung.com>
>> ---
>>   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

[PATCH 4/5] staging: emxx_udc: add missing usb_ep_set_maxpacket_limit()

[Robert Baldyga]

Should use usb_ep_set_maxpacket_limit instead of setting
maxpacket value manually.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 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

[PATCH 4/5] staging: emxx_udc: add missing usb_ep_set_maxpacket_limit()

[Sergei Shtylyov]

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 <r.baldyga@samsung.com>
> ---
>   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

[PATCH 4/5] staging: emxx_udc: add missing usb_ep_set_maxpacket_limit()

[Dan Carpenter]

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

[PATCH 5/5] usb: gadget: atmel_usba_udc: add missing ret value check

[Robert Baldyga]

Add missing return value check. In case of error print debug message
and return error code.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 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

[PATCH 5/5] usb: gadget: atmel_usba_udc: add missing ret value check

[Nicolas Ferre]

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 <r.baldyga@samsung.com>

Yes, it's indeed missing:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

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