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(-)
If allocation of multiple buffers would fail then we would leak memory.
Fix that.
Signed-off-by: Roger Quadros <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
memset sets the entire data structure to zero, so no need to
zero initialize its field again.
Signed-off-by: Roger Quadros <[email protected]>
---
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
Prevent a NULL pointer dereference in fsg_config_from_params() if
'file' parameter is not specified.
Signed-off-by: Roger Quadros <[email protected]>
---
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
On Tue, 05 Apr 2011 17:36:40 +0200, Roger Quadros
<[email protected]> 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 <[email protected]>
> ---
> 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: [email protected]>-----ooO--(_)--Ooo--
On Tue, 05 Apr 2011 17:36:41 +0200, Roger Quadros
<[email protected]> wrote:
> If allocation of multiple buffers would fail then we would leak memory.
> Fix that.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> 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: [email protected]>-----ooO--(_)--Ooo--
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 <[email protected]>
> ---
> 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: [email protected]>-----ooO--(_)--Ooo--
On Tue, 05 Apr 2011 17:36:39 +0200, Roger Quadros
<[email protected]> 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 <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
> --- 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: [email protected]>-----ooO--(_)--Ooo--
On Tue, 05 Apr 2011 17:36:38 +0200, Roger Quadros
<[email protected]> 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 <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
> --- 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: [email protected]>-----ooO--(_)--Ooo--
On Tue, 05 Apr 2011 17:36:37 +0200, Roger Quadros
<[email protected]> 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: [email protected]>-----ooO--(_)--Ooo--
On 04/05/2011 06:56 PM, ext Michal Nazarewicz wrote:
> On Tue, 05 Apr 2011 17:36:40 +0200, Roger Quadros
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
On 04/05/2011 06:59 PM, ext Michal Nazarewicz wrote:
> On Tue, 05 Apr 2011 17:36:41 +0200, Roger Quadros
> <[email protected]> wrote:
>
>> If allocation of multiple buffers would fail then we would leak memory.
>> Fix that.
>>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> 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