Skip to contentSkip to conversation

A few mass_storage fixes.

PATCH In review Michal Nazarewicz 3 participants 8 messages Jun 18, 2012 – Jun 19, 2012 USB

lore
Share & tools
lore
#0 lore raw

cover A few mass_storage fixes.

From: Michal Nazarewicz <[email protected]>

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(-)
Signature · 1 line
1.7.7.3
#1 lore raw

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

From: Michal Nazarewicz <[email protected]>

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.

---
 drivers/usb/gadget/storage_common.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)
drivers/usb/gadget/storage_common.c +0 -10
index 8081ca3..8a8157f 100644
@@ -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;
 
Signature · 1 line
1.7.7.3
#2 lore raw

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

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

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.

---
 drivers/usb/gadget/storage_common.c |   52 +++++++++++++++++++---------------
 1 files changed, 29 insertions(+), 23 deletions(-)
drivers/usb/gadget/storage_common.c +29 -23
index 8a8157f..e576678 100644
@@ -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);
Signature · 1 line
1.7.7.3
Acked-by on v1 2/3
#3 lore raw

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

Quoted 11 lines · Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

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.

#4 lore raw

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

In reply to Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

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.

---
 drivers/usb/gadget/storage_common.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/usb/gadget/storage_common.c +3 -0
index e576678..52334d7 100644
@@ -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);
Signature · 1 line
1.7.7.3
#5 lore raw

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

Quoted 6 lines · Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

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

#6 lore raw

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

Quoted 9 lines · Alan Stern
Quoted 6 lines · Michal Nazarewicz

From: Michal Nazarewicz <[email protected]>

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.

Quoted 4 lines

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.

Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: [email protected]>--------------ooO--(_)--Ooo--
#7 lore raw

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

In reply to Michal Nazarewicz

Hello.

Quoted 3 lines · Michal Nazarewicz

From: Michal Nazarewicz<[email protected]>

The fsg_file_store()

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

Quoted 15 lines

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.

---
  drivers/usb/gadget/storage_common.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)
drivers/usb/gadget/storage_common.c +0 -0
index e576678..52334d7 100644

drivers/usb/gadget/storage_common.c static ssize_t fsg_store_file(struct device *dev, struct device_attribute *attr, · line 883

Quoted 6 lines
drivers/usb/gadget/storage_common.c +3 -0
@@ -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