A few mass_storage fixes.
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
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
@@ -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
PATCH 2/3 usb: gadget: mass_storage: fail fsg_store_file() early if colud not open file
In reply to Michal NazarewiczFrom: 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
@@ -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
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.
PATCH 3/3 usb: gadget: mass_storage: require backing file for non-removable LUNs
In reply to Michal NazarewiczFrom: 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
@@ -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
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
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--
Re: [PATCH 3/3] usb: gadget: mass_storage: require backing file for non-removable LUNs
In reply to Michal NazarewiczHello.
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.
Signed-off-by Michal Nazarewicz<[email protected]>--- drivers/usb/gadget/storage_common.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/usb/gadget/storage_common.c +0
-0
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