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(-)
--
1.7.7.3
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.
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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
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.
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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
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.
Signed-off-by: Michal Nazarewicz <[email protected]>
---
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
On Mon, 18 Jun 2012, Michal Nazarewicz wrote:
> 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
On Mon, 18 Jun 2012, Michal Nazarewicz wrote:
> 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.
Acked-by: Alan Stern <[email protected]>
On Mon, 18 Jun 2012 16:18:41 +0200, Alan Stern <[email protected]> wrote:
> On Mon, 18 Jun 2012, Michal Nazarewicz wrote:
>
>> 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.
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: [email protected]>--------------ooO--(_)--Ooo--
Hello.
On 18-06-2012 16:37, Michal Nazarewicz wrote:
> From: Michal Nazarewicz<[email protected]>
> 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<[email protected]>
> ---
> 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