[PATCH 0/5] fixes for f_mass_storage

[PATCH 0/5] fixes for f_mass_storage

[Roger Quadros]

Hi,

These patches contain some fixes and improvements for f_mass_storage.c

cheers,
-roger

---
Roger Quadros (5):
  usb: gadget: f_mass_storage: Fix Bulk-only RESET handling
  usb: gadget: f_mass_storage: If 'ro'/'cdrom' specified, open file as
    read-only
  usb: gadget: f_mass_storage: Prevent NULL pointer dereference
  usb: gadget: f_mass_storage: Fix potential memory leak
  usb: gadget: f_mass_storage: remove unnecessary initialization

 drivers/usb/gadget/f_mass_storage.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

[PATCH 1/5] usb: gadget: f_mass_storage: Fix Bulk-only RESET handling

[Roger Quadros]

The ep0 request tag was not recorded thus resulting in phase
problems while sending status/response in handle_execption() handler.
This was resulting in MSC compliance test failures with USBCV tool.

With this patch, the Bulk-Only Mass storage RESET request is
handled correctly and the MSC compliance tests pass.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index ef6c65a..90472af 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -613,6 +613,11 @@ static int fsg_setup(struct usb_function *f,
 	if (!fsg_is_set(fsg->common))
 		return -EOPNOTSUPP;
 
+	++fsg->common->ep0_req_tag;	/* Record arrival of a new request */
+	req->context = NULL;
+	req->length = 0;
+	dump_msg(fsg, "ep0-setup", (u8 *) ctrl, sizeof(*ctrl));
+
 	switch (ctrl->bRequest) {
 
 	case USB_BULK_RESET_REQUEST:
-- 
1.6.0.4

[PATCH 2/5] usb: gadget: f_mass_storage: If 'ro'/'cdrom' specified, open file as read-only

[Roger Quadros]

If we don't need Write access then attempt to open backing file in Read Only
mode instead of bailing out too soon.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 90472af..5d7de93 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2810,6 +2810,7 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 	for (i = 0, lcfg = cfg->luns; i < nluns; ++i, ++curlun, ++lcfg) {
 		curlun->cdrom = !!lcfg->cdrom;
 		curlun->ro = lcfg->cdrom || lcfg->ro;
+		curlun->initially_ro = curlun->ro;
 		curlun->removable = lcfg->removable;
 		curlun->dev.release = fsg_lun_release;
 		curlun->dev.parent = &gadget->dev;
-- 
1.6.0.4

[PATCH 3/5] usb: gadget: f_mass_storage: Prevent NULL pointer dereference

[Roger Quadros]

Prevent a NULL pointer dereference in fsg_config_from_params() if
'file' parameter is not specified.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 5d7de93..f6bd001 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -3177,7 +3177,7 @@ fsg_config_from_params(struct fsg_config *cfg,
 		lun->removable = /* Removable by default */
 			params->removable_count <= i || params->removable[i];
 		lun->filename =
-			params->file_count > i && params->file[i][0]
+			params->file_count > i && params->file[i]
 			? params->file[i]
 			: 0;
 	}
-- 
1.6.0.4

[PATCH 4/5] usb: gadget: f_mass_storage: Fix potential memory leak

[Roger Quadros]

If allocation of multiple buffers would fail then we would leak memory.
Fix that.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index f6bd001..60b4df9 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2863,6 +2863,12 @@ buffhds_first_it:
 		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
 		if (unlikely(!bh->buf)) {
 			rc = -ENOMEM;
+			/* clean up */
+			while (i < FSG_NUM_BUFFERS) {
+				bh--;
+				kfree(bh->buf);
+				i++;
+			}
 			goto error_release;
 		}
 	} while (--i);
-- 
1.6.0.4

[PATCH 5/5] usb: gadget: f_mass_storage: remove unnecessary initialization

[Roger Quadros]

memset sets the entire data structure to zero, so no need to
zero initialize its field again.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 60b4df9..d3c00e6 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2775,7 +2775,6 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
 		common->free_storage_on_release = 1;
 	} else {
 		memset(common, 0, sizeof *common);
-		common->free_storage_on_release = 0;
 	}
 
 	common->ops = cfg->ops;
-- 
1.6.0.4

Re: [PATCH 3/5] usb: gadget: f_mass_storage: Prevent NULL pointer dereference

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:40 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> Prevent a NULL pointer dereference in fsg_config_from_params() if
> 'file' parameter is not specified.

Have you observed this behaviour?  I don't see how it could happen with
module parameters and if it appears in some gadget it's a bug in the
gadget.  Not that I'm saying checking for null pointer is a bad idea.

> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
> ---
>  drivers/usb/gadget/f_mass_storage.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c  
> b/drivers/usb/gadget/f_mass_storage.c
> index 5d7de93..f6bd001 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -3177,7 +3177,7 @@ fsg_config_from_params(struct fsg_config *cfg,
>  		lun->removable = /* Removable by default */
>  			params->removable_count <= i || params->removable[i];
>  		lun->filename =
> -			params->file_count > i && params->file[i][0]
> +			params->file_count > i && params->file[i]

You're removing the check if an empty file name has been specified.  It
should read:

+			params->file_count > i && params->file[i] && params->file[i][0]

And since the line is getting pretty long, maybe convert it to a proper
“if”.  I'm sure Greg will like that. ;)

>  			? params->file[i]
>  			: 0;
>  	}

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 4/5] usb: gadget: f_mass_storage: Fix potential memory leak

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:41 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:

> If allocation of multiple buffers would fail then we would leak memory.
> Fix that.
>
> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
> ---
>  drivers/usb/gadget/f_mass_storage.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c  
> b/drivers/usb/gadget/f_mass_storage.c
> index f6bd001..60b4df9 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2863,6 +2863,12 @@ buffhds_first_it:
>  		bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
>  		if (unlikely(!bh->buf)) {
>  			rc = -ENOMEM;
> +			/* clean up */
> +			while (i < FSG_NUM_BUFFERS) {
> +				bh--;
> +				kfree(bh->buf);
> +				i++;
> +			}
>  			goto error_release;
>  		}
>  	} while (--i);

This is handled in fsg_common_release(), isn't it?  Feel free to
send a patch with a comment explaining that.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 5/5] usb: gadget: f_mass_storage: remove unnecessary initialization

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:42 +0200, Roger Quadros wrote:
> memset sets the entire data structure to zero, so no need to
> zero initialize its field again.
>
> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
> ---
>  drivers/usb/gadget/f_mass_storage.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c  
> b/drivers/usb/gadget/f_mass_storage.c
> index 60b4df9..d3c00e6 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2775,7 +2775,6 @@ static struct fsg_common *fsg_common_init(struct  
> fsg_common *common,
>  		common->free_storage_on_release = 1;
>  	} else {
>  		memset(common, 0, sizeof *common);
> -		common->free_storage_on_release = 0;
>  	}
> 	common->ops = cfg->ops;

Yes, that's correct, but *I* would still leave that for the sake of
symmetry.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 2/5] usb: gadget: f_mass_storage: If 'ro'/'cdrom' specified, open file as read-only

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:39 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> If we don't need Write access then attempt to open backing file in Read  
> Only mode instead of bailing out too soon.
>
> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2810,6 +2810,7 @@ static struct fsg_common *fsg_common_init(struct  
> fsg_common *common,
>  	for (i = 0, lcfg = cfg->luns; i < nluns; ++i, ++curlun, ++lcfg) {
>  		curlun->cdrom = !!lcfg->cdrom;
>  		curlun->ro = lcfg->cdrom || lcfg->ro;
> +		curlun->initially_ro = curlun->ro;
>  		curlun->removable = lcfg->removable;
>  		curlun->dev.release = fsg_lun_release;
>  		curlun->dev.parent = &gadget->dev;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 1/5] usb: gadget: f_mass_storage: Fix Bulk-only RESET handling

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:38 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> The ep0 request tag was not recorded thus resulting in phase
> problems while sending status/response in handle_execption() handler.
> This was resulting in MSC compliance test failures with USBCV tool.
>
> With this patch, the Bulk-Only Mass storage RESET request is
> handled correctly and the MSC compliance tests pass.
>
> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -613,6 +613,11 @@ static int fsg_setup(struct usb_function *f,
>  	if (!fsg_is_set(fsg->common))
>  		return -EOPNOTSUPP;
> +	++fsg->common->ep0_req_tag;	/* Record arrival of a new request */
> +	req->context = NULL;
> +	req->length = 0;
> +	dump_msg(fsg, "ep0-setup", (u8 *) ctrl, sizeof(*ctrl));
> +
>  	switch (ctrl->bRequest) {
> 	case USB_BULK_RESET_REQUEST:

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 0/5] fixes for f_mass_storage

[Michal Nazarewicz]

On Tue, 05 Apr 2011 17:36:37 +0200, Roger Quadros  
<roger.quadros@nokia.com> wrote:
> These patches contain some fixes and improvements for f_mass_storage.c

Thanks for the fixes!  Comments follow each patch.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 3/5] usb: gadget: f_mass_storage: Prevent NULL pointer dereference

[Roger Quadros]

On 04/05/2011 06:56 PM, ext Michal Nazarewicz wrote:
> On Tue, 05 Apr 2011 17:36:40 +0200, Roger Quadros
> <roger.quadros@nokia.com> wrote:
>> Prevent a NULL pointer dereference in fsg_config_from_params() if
>> 'file' parameter is not specified.
> 
> Have you observed this behaviour?  I don't see how it could happen with
> module parameters and if it appears in some gadget it's a bug in the

It can happen if the gadget that uses f_mass_storage specifies
file_count=1 and doesn't specify a file name.

> gadget.  Not that I'm saying checking for null pointer is a bad idea.

OK. let's do that then.

> 
>> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
>> ---
>>  drivers/usb/gadget/f_mass_storage.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index 5d7de93..f6bd001 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -3177,7 +3177,7 @@ fsg_config_from_params(struct fsg_config *cfg,
>>          lun->removable = /* Removable by default */
>>              params->removable_count <= i || params->removable[i];
>>          lun->filename =
>> -            params->file_count > i && params->file[i][0]
>> +            params->file_count > i && params->file[i]
> 
> You're removing the check if an empty file name has been specified.  It
> should read:
> 
> +            params->file_count > i && params->file[i] &&
> params->file[i][0]

Right.

> 
> And since the line is getting pretty long, maybe convert it to a proper
> “if”.  I'm sure Greg will like that. ;)
> 
>>              ? params->file[i]
>>              : 0;
>>      }
> 
ok.

-- 
regards,
-roger

Re: [PATCH 4/5] usb: gadget: f_mass_storage: Fix potential memory leak

[Roger Quadros]

On 04/05/2011 06:59 PM, ext Michal Nazarewicz wrote:
> On Tue, 05 Apr 2011 17:36:41 +0200, Roger Quadros
> <roger.quadros@nokia.com> wrote:
> 
>> If allocation of multiple buffers would fail then we would leak memory.
>> Fix that.
>>
>> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
>> ---
>>  drivers/usb/gadget/f_mass_storage.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index f6bd001..60b4df9 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -2863,6 +2863,12 @@ buffhds_first_it:
>>          bh->buf = kmalloc(FSG_BUFLEN, GFP_KERNEL);
>>          if (unlikely(!bh->buf)) {
>>              rc = -ENOMEM;
>> +            /* clean up */
>> +            while (i < FSG_NUM_BUFFERS) {
>> +                bh--;
>> +                kfree(bh->buf);
>> +                i++;
>> +            }
>>              goto error_release;
>>          }
>>      } while (--i);
> 
> This is handled in fsg_common_release(), isn't it?  Feel free to
> send a patch with a comment explaining that.
> 

Yes it is. This patch is not required.

-- 
regards,
-roger