On Wed, Oct 09 2013, Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx> wrote:
> From this commit on f_mass_storage is available through configfs.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
Just tiny nit-picking to follow:
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index d80be5f..00d3687 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -3295,6 +3296,342 @@ static int fsg_bind_config(struct usb_composite_dev *cdev,
>
> #else
>
> +static inline struct fsg_lun_opts *to_fsg_lun_opts(struct config_item *item)
> +{
> + return container_of(to_config_group(item), struct fsg_lun_opts, group);
> +}
> +
> +static inline struct fsg_opts *to_fsg_opts(struct config_item *item)
> +{
> + return container_of(to_config_group(item), struct fsg_opts,
> + func_inst.group);
> +}
Personally I'd prefer “fsg_lun_opts_from_config_item” and
“fsg_opts_from_config_item”, but whatever.
> +
> +CONFIGFS_ATTR_STRUCT(fsg_lun_opts);
> +CONFIGFS_ATTR_OPS(fsg_lun_opts);
> +
> +static void fsg_lun_attr_release(struct config_item *item)
> +{
> + struct fsg_lun_opts *lun_opts;
> +
> + lun_opts = to_fsg_lun_opts(item);
> + kfree(lun_opts);
> +}
> +
> +static struct configfs_item_operations fsg_lun_item_ops = {
> + .release = fsg_lun_attr_release,
> + .show_attribute = fsg_lun_opts_attr_show,
> + .store_attribute = fsg_lun_opts_attr_store,
Inconsistent way of lining up the equal sign. The first entry has
a tab, the other two have spaces, plus the last one is not aligned at
all.
> +#define MAX_NAME_LEN 40
This is never used.
> +
> +static struct config_group *fsg_lun_make(struct config_group *group,
> + const char *name)
> +{
> + struct fsg_lun_opts *opts;
> + struct fsg_opts *fsg_opts;
> + struct fsg_lun_config config;
> + char *num_str;
> + u8 num;
> + int ret;
> +
> + num_str = strchr(name, '.');
> + if (!num_str) {
> + pr_err("Unable to locate . in LUN.NUMBER\n");
> + return ERR_PTR(-EINVAL);
> + }
> + num_str++;
> +
> + ret = kstrtou8(num_str, 0, &num);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + fsg_opts = to_fsg_opts(&group->cg_item);
> + if (num >= FSG_MAX_LUNS)
> + return ERR_PTR(-ENODEV);
return ERR_PTR(-ERANGE);
to be consistant with what kstrotou8 returns if a big number is given.
Alternatively, make it -ENODEV when kstrotou8 return -ERANGE.
> + mutex_lock(&fsg_opts->lock);
> + if (fsg_opts->refcnt || fsg_opts->common->luns[num]) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memset(&config, 0, sizeof(config));
> + config.removable = true;
> +
> +
One empty line too many.
> + ret = fsg_common_create_lun(fsg_opts->common, &config, num, name,
> + (const char **)&group->cg_item.ci_name);
> + if (ret) {
> + kfree(opts);
> + goto out;
> + }
> + opts->lun = fsg_opts->common->luns[num];
> + opts->lun_id = num;
> + mutex_unlock(&fsg_opts->lock);
> +
> + config_group_init_type_name(&opts->group, name, &fsg_lun_type);
> +
> + return &opts->group;
> +out:
> + mutex_unlock(&fsg_opts->lock);
> + return ERR_PTR(ret);
> +}
> +static struct configfs_item_operations fsg_item_ops = {
> + .release = fsg_attr_release,
> + .show_attribute = fsg_opts_attr_show,
> + .store_attribute = fsg_opts_attr_store,
Wrong alignment as well.
> +};
> +static ssize_t fsg_opts_stall_store(struct fsg_opts *opts, const char *page,
> + size_t len)
> +{
> + int ret;
> + u8 num;
> +
> + mutex_lock(&opts->lock);
> + if (opts->refcnt) {
> + ret = -EBUSY;
> + goto end;
> + }
> + ret = kstrtou8(page, 0, &num);
Here, like in other places, you might consider using strtobool. In
particular, I see no reason why use kstrotou8 to be honest. If “2” is
supposed to be a valid “true” value, why not allow for “256” as well?
This goes for other places as well.
> + if (ret)
> + goto end;
> +
> + opts->common->can_stall = num != 0;
> + ret = len;
> +
> +end:
> + mutex_unlock(&opts->lock);
> + return ret;
> +}
--
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
