On Thu, Oct 03 2013, Andrzej Pietrasiewicz wrote:
> fsg_common_init is a lengthy function. Factor portions of it out.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> ---
> drivers/usb/gadget/f_mass_storage.c | 260 ++++++++++++++++++++++-------------
> drivers/usb/gadget/f_mass_storage.h | 6 +
> 2 files changed, 169 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index 61952b6..441bde5 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2829,18 +2829,172 @@ int fsg_common_set_cdev(struct fsg_common *common,
> return 0;
> }
>
> +static inline int fsg_common_add_sysfs(struct fsg_common *common,
> + struct fsg_lun *lun)
> +{
> + int rc;
> +
> + rc = device_register(&lun->dev);
> + if (rc) {
> + put_device(&lun->dev);
> + return rc;
> + }
> +
> + rc = device_create_file(&lun->dev,
> + lun->cdrom
> + ? &dev_attr_ro_cdrom
> + : &dev_attr_ro);
> + if (rc)
> + goto error_cdrom;
> + rc = device_create_file(&lun->dev,
> + lun->removable
> + ? &dev_attr_file
> + : &dev_attr_file_nonremovable);
> + if (rc)
> + goto error_removable;
> + rc = device_create_file(&lun->dev, &dev_attr_nofua);
> + if (rc)
> + goto error_nofua;
> +
> + return 0;
> +
> +error_nofua:
> + device_remove_file(&lun->dev,
> + lun->removable
> + ? &dev_attr_file
> + : &dev_attr_file_nonremovable);
> +error_removable:
> + device_remove_file(&lun->dev,
> + lun->cdrom
> + ? &dev_attr_ro_cdrom
> + : &dev_attr_ro);
Dose are no-ops if the file does not exist, right? So why not just
unconditionally remove both files and then you con use the other
function you've introduced earlier to remove them.
> +error_cdrom:
> + device_unregister(&lun->dev);
> + return rc;
> +}
> +
> #define MAX_LUN_NAME_LEN 80
>
> +int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg,
> + unsigned int id, const char *name,
> + const char **name_pfx)
> +{
> + struct fsg_lun *lun;
> + char *pathbuf;
> + int rc = -ENOMEM;
> + int name_len;
> +
> + if (!common->nluns || !common->luns)
> + return -ENODEV;
> +
> + if (common->luns[id])
> + return -EBUSY;
> +
> + name_len = strlen(name) + 1;
> + if (name_len > MAX_LUN_NAME_LEN)
> + return -ENAMETOOLONG;
> +
> + lun = kzalloc(sizeof(*lun), GFP_KERNEL);
> + if (!lun)
> + return -ENOMEM;
> +
> + lun->name = kstrndup(name, name_len, GFP_KERNEL);
> + if (!lun->name)
> + goto error_name;
Why do you care if the name is longer then MAX_LUN_NAME_LEN-1?
kstrdup() will allocate the space anyway, so why the limit at all?
And like discussed earlier, I believe that if common->sysfs, the copying
can be avoided anyway.
> + lun->name_pfx = name_pfx;
> +
> + lun->cdrom = !!cfg->cdrom;
> + lun->ro = cfg->cdrom || cfg->ro;
> + lun->initially_ro = lun->ro;
> + lun->removable = !!cfg->removable;
> +
> + common->luns[id] = lun;
> +
> + if (common->sysfs) {
> + lun->dev.release = fsg_lun_release;
> + lun->dev.parent = &common->gadget->dev;
> + dev_set_drvdata(&lun->dev, &common->filesem);
> + dev_set_name(&lun->dev, name);
> +
> + rc = fsg_common_add_sysfs(common, lun);
> + if (rc) {
> + pr_info("failed to register LUN%d: %d\n", id, rc);
> + goto error_sysfs;
> + }
> + }
> +
> + if (cfg->filename) {
> + rc = fsg_lun_open(lun, cfg->filename);
> + if (rc)
> + goto error_lun;
> + } else if (!lun->removable) {
> + pr_err("no file given for LUN%d\n", id);
> + rc = -EINVAL;
> + goto error_lun;
> + }
Actually, could !cfg->filename && !cfg->removable be checked somewhere
at the beginning of the function so that we don't waste our time
initialising stuff if we know we'll fail later on anyway?
> + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> + {
> + char *p = "(no medium)";
Just declare p alongside pathbuf at the beginning of the function and
drop the block. It just adds indentation level for no reason.
> + if (fsg_lun_is_open(lun)) {
> + p = "(error)";
> + if (pathbuf) {
> + p = d_path(&lun->filp->f_path,
> + pathbuf, PATH_MAX);
> + if (IS_ERR(p))
> + p = "(error)";
> + }
> + }
> + pr_info("LUN: %s%s%sfile: %s\n",
> + lun->removable ? "removable " : "",
> + lun->ro ? "read only " : "",
> + lun->cdrom ? "CD-ROM " : "",
> + p);
> + }
> + kfree(pathbuf);
> +
> + return 0;
> +
> +error_lun:
> + if (common->sysfs) {
> + fsg_common_remove_sysfs(lun);
> + device_unregister(&lun->dev);
> + }
> + fsg_lun_close(lun);
> +error_sysfs:
> + common->luns[id] = NULL;
> + kfree(lun->name);
> +error_name:
> + kfree(lun);
> + return rc;
> +}
> +
> +int fsg_common_create_luns(struct fsg_common *common, struct fsg_config *cfg)
> +{
> + char buf[40]; /* enough for 2^128 decimal */
Yeah, and that's why “char buf[8];” would be more then enough. ;)
> + int i, rc;
> +
> + for (i = 0; i < common->nluns; ++i) {
> + snprintf(buf, sizeof(buf), "lun%d", i);
> + rc = fsg_common_create_lun(common, &cfg->luns[i], i, buf, NULL);
> + if (rc)
> + goto fail;
> + }
> +
> + pr_info("Number of LUNs=%d\n", common->nluns);
> +
> + return 0;
> +
> +fail:
> + _fsg_common_remove_luns(common, i);
> + return rc;
> +}
> +
> struct fsg_common *fsg_common_init(struct fsg_common *common,
> struct usb_composite_dev *cdev,
> struct fsg_config *cfg)
> {
> - struct usb_gadget *gadget = cdev->gadget;
> - struct fsg_lun **curlun_it;
> - struct fsg_lun_config *lcfg;
> - int nluns, i, rc;
> - char *pathbuf;
> -
> + int i, rc;
>
> common = fsg_common_setup(common, !!common);
> if (IS_ERR(common))
> @@ -2942,7 +3034,6 @@ struct fsg_common *fsg_common_init(struct fsg_common *common,
> : "File-Stor Gadget"),
> i);
>
> -
O_o
> /* Tell the thread to start working */
> common->thread_task =
> kthread_create(fsg_main_thread, common, "file-storage");
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
