[PATCH 0/3] A few mass_storage fixes.

[PATCH 0/3] A few mass_storage fixes.

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Two fixes to the "file" sysfs entry handling and one patch removing
unused code.

Michal Nazarewicz (3):
  usb: gadget: storage_common: remove FSG_BUFFHD_STATIC_BUFFER support
  usb: gadget: mass_storage: fail fsg_store_file() early if colud not
    open file
  usb: gadget: mass_storage: require backing file for non-removable
    LUNs

 drivers/usb/gadget/storage_common.c |   65 +++++++++++++++++------------------
 1 files changed, 32 insertions(+), 33 deletions(-)

-- 
1.7.7.3

[PATCH 1/3] usb: gadget: storage_common: remove FSG_BUFFHD_STATIC_BUFFER support

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Since f_mass_storage stopped using FSG_BUFFHD_STATIC_BUFFER (because it
caused buffers not to be page aligned which did not work well with at
least some UDCs), no code was using it.  Removing not to bloat the code
too much.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/storage_common.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 8081ca3..8a8157f 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -38,12 +38,6 @@
  */
 
 /*
- * When FSG_BUFFHD_STATIC_BUFFER is defined when this file is included
- * the fsg_buffhd structure's buf field will be an array of FSG_BUFLEN
- * characters rather then a pointer to void.
- */
-
-/*
  * When USB_GADGET_DEBUG_FILES is defined the module param num_buffers
  * sets the number of pipeline buffers (length of the fsg_buffhd array).
  * The valid range of num_buffers is: num >= 2 && num <= 4.
@@ -260,11 +254,7 @@ enum fsg_buffer_state {
 };
 
 struct fsg_buffhd {
-#ifdef FSG_BUFFHD_STATIC_BUFFER
-	char				buf[FSG_BUFLEN];
-#else
 	void				*buf;
-#endif
 	enum fsg_buffer_state		state;
 	struct fsg_buffhd		*next;
 
-- 
1.7.7.3

[PATCH 2/3] usb: gadget: mass_storage: fail fsg_store_file() early if colud not open file

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Currently, when a new value is stored to the “file” sysfs entry,
fsg_store_file() will release existing backing file and only then attempt to
open a new one.  If that fails, no new backing file is open.

This commit changes the fsg_lun_open() so that it closes existing backing file
only after the new backing file has been successfully opened.  With that
change, fsg_store_file() may use it to perform an atomic open operation with
guarantee that logical unit will either point to the new backing file or still
to the old one.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/storage_common.c |   52 +++++++++++++++++++---------------
 1 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 8a8157f..e576678 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -617,6 +617,16 @@ static struct usb_gadget_strings	fsg_stringtab = {
  * the caller must own fsg->filesem for writing.
  */
 
+static void fsg_lun_close(struct fsg_lun *curlun)
+{
+	if (curlun->filp) {
+		LDBG(curlun, "close backing file\n");
+		fput(curlun->filp);
+		curlun->filp = NULL;
+	}
+}
+
+
 static int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
 {
 	int				ro;
@@ -626,6 +636,8 @@ static int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
 	loff_t				size;
 	loff_t				num_sectors;
 	loff_t				min_sectors;
+	unsigned int			blkbits;
+	unsigned int			blksize;
 
 	/* R/W if we can, R/O if we must */
 	ro = curlun->initially_ro;
@@ -670,17 +682,17 @@ static int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
 	}
 
 	if (curlun->cdrom) {
-		curlun->blksize = 2048;
-		curlun->blkbits = 11;
+		blksize = 2048;
+		blkbits = 11;
 	} else if (inode->i_bdev) {
-		curlun->blksize = bdev_logical_block_size(inode->i_bdev);
-		curlun->blkbits = blksize_bits(curlun->blksize);
+		blksize = bdev_logical_block_size(inode->i_bdev);
+		blkbits = blksize_bits(blksize);
 	} else {
-		curlun->blksize = 512;
-		curlun->blkbits = 9;
+		blksize = 512;
+		blkbits = 9;
 	}
 
-	num_sectors = size >> curlun->blkbits; /* File size in logic-block-size blocks */
+	num_sectors = size >> blkbits; /* File size in logic-block-size blocks */
 	min_sectors = 1;
 	if (curlun->cdrom) {
 		min_sectors = 300;	/* Smallest track is 300 frames */
@@ -697,7 +709,12 @@ static int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
 		goto out;
 	}
 
+	if (fsg_lun_is_open(curlun))
+		fsg_lun_close(curlun);
+
 	get_file(filp);
+	curlun->blksize = blksize;
+	curlun->blkbits = blkbits;
 	curlun->ro = ro;
 	curlun->filp = filp;
 	curlun->file_length = size;
@@ -711,16 +728,6 @@ out:
 }
 
 
-static void fsg_lun_close(struct fsg_lun *curlun)
-{
-	if (curlun->filp) {
-		LDBG(curlun, "close backing file\n");
-		fput(curlun->filp);
-		curlun->filp = NULL;
-	}
-}
-
-
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -871,19 +878,18 @@ static ssize_t fsg_store_file(struct device *dev, struct device_attribute *attr,
 	if (count > 0 && buf[count-1] == '\n')
 		((char *) buf)[count-1] = 0;		/* Ugh! */
 
-	/* Eject current medium */
-	down_write(filesem);
-	if (fsg_lun_is_open(curlun)) {
-		fsg_lun_close(curlun);
-		curlun->unit_attention_data = SS_MEDIUM_NOT_PRESENT;
-	}
 
 	/* Load new medium */
+	down_write(filesem);
 	if (count > 0 && buf[0]) {
+		/* fsg_lun_open() will close existing file if any. */
 		rc = fsg_lun_open(curlun, buf);
 		if (rc == 0)
 			curlun->unit_attention_data =
 					SS_NOT_READY_TO_READY_TRANSITION;
+	} else if (fsg_lun_is_open(curlun)) {
+		fsg_lun_close(curlun);
+		curlun->unit_attention_data = SS_MEDIUM_NOT_PRESENT;
 	}
 	up_write(filesem);
 	return (rc < 0 ? rc : count);
-- 
1.7.7.3

[PATCH 3/3] usb: gadget: mass_storage: require backing file for non-removable LUNs

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

The fsg_file_store() function does not check whether a LUN is removable or not
allowing one to specify an empty file name for a non-removable LUN.  This
commit adds explicit check of whether a file name is provided for
non-removable LUNs.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/storage_common.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index e576678..52334d7 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -878,6 +878,9 @@ static ssize_t fsg_store_file(struct device *dev, struct device_attribute *attr,
 	if (count > 0 && buf[count-1] == '\n')
 		((char *) buf)[count-1] = 0;		/* Ugh! */
 
+	/* Must specify a valid file if LUN is not removable. */
+	if (!curlun->removable && !*buf)
+		return -EINVAL;
 
 	/* Load new medium */
 	down_write(filesem);
-- 
1.7.7.3

Re: [PATCH 3/3] usb: gadget: mass_storage: require backing file for non-removable LUNs

[Alan Stern]

On Mon, 18 Jun 2012, Michal Nazarewicz wrote:

> From: Michal Nazarewicz <mina86@mina86.com>
> 
> The fsg_file_store() function does not check whether a LUN is removable or not
> allowing one to specify an empty file name for a non-removable LUN.  This
> commit adds explicit check of whether a file name is provided for
> non-removable LUNs.

Looks okay.  Note that in file_storage.c, the "file" attribute isn't 
writable if the LUN isn't removable, and the "ro" attribute isn't 
writable if the LUN is a cdrom.  Maybe you would prefer to do things 
that way instead.

Alan Stern

Re: [PATCH 2/3] usb: gadget: mass_storage: fail fsg_store_file() early if colud not open file

[Alan Stern]

On Mon, 18 Jun 2012, Michal Nazarewicz wrote:

> From: Michal Nazarewicz <mina86@mina86.com>
> 
> Currently, when a new value is stored to the “file” sysfs entry,
> fsg_store_file() will release existing backing file and only then attempt to
> open a new one.  If that fails, no new backing file is open.
> 
> This commit changes the fsg_lun_open() so that it closes existing backing file
> only after the new backing file has been successfully opened.  With that
> change, fsg_store_file() may use it to perform an atomic open operation with
> guarantee that logical unit will either point to the new backing file or still
> to the old one.

This is a good idea.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 3/3] usb: gadget: mass_storage: require backing file for non-removable LUNs

[Michal Nazarewicz]

On Mon, 18 Jun 2012 16:18:41 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Mon, 18 Jun 2012, Michal Nazarewicz wrote:
>
>> From: Michal Nazarewicz <mina86@mina86.com>
>>
>> The fsg_file_store() function does not check whether a LUN is removable or not
>> allowing one to specify an empty file name for a non-removable LUN.  This
>> commit adds explicit check of whether a file name is provided for
>> non-removable LUNs.
>
> Looks okay.  Note that in file_storage.c, the "file" attribute isn't
> writable if the LUN isn't removable, and the "ro" attribute isn't
> writable if the LUN is a cdrom.  Maybe you would prefer to do things
> that way instead.

Makes sense, I'll rewrite this patch to do that.

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

Re: [PATCH 3/3] usb: gadget: mass_storage: require backing file for non-removable LUNs

[Sergei Shtylyov]

Hello.

On 18-06-2012 16:37, Michal Nazarewicz wrote:

> From: Michal Nazarewicz<mina86@mina86.com>

> The fsg_file_store()

    Apparently, fsg_store_file(), judging by the patch itself.

> function does not check whether a LUN is removable or not
> allowing one to specify an empty file name for a non-removable LUN.  This
> commit adds explicit check of whether a file name is provided for
> non-removable LUNs.

> Signed-off-by: Michal Nazarewicz<mina86@mina86.com>
> ---
>   drivers/usb/gadget/storage_common.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)

> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
> index e576678..52334d7 100644
> --- a/drivers/usb/gadget/storage_common.c
> +++ b/drivers/usb/gadget/storage_common.c
> @@ -878,6 +878,9 @@ static ssize_t fsg_store_file(struct device *dev, struct device_attribute *attr,
>   	if (count > 0 && buf[count-1] == '\n')
>   		((char *) buf)[count-1] = 0;		/* Ugh! */
>
> +	/* Must specify a valid file if LUN is not removable. */
> +	if (!curlun->removable&&  !*buf)
> +		return -EINVAL;

WBR, Sergei