On Thu, Oct 03 2013, Andrzej Pietrasiewicz wrote:
> Converting mass storage to the new function interface requires converting
> the USB mass storage's function code and its users.
> This patch converts the f_mass_storage.c to the new function interface.
> The file is now compiled into a separate usb_f_mass_storage.ko module.
> The old function interface is provided by means of a preprocessor conditional
> directives. After all users are converted, the old interface can be removed.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index d90a0b0..4a86b0c 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -2627,11 +2628,17 @@ void fsg_common_get(struct fsg_common *common)
> {
> kref_get(&common->ref);
> }
> +#ifndef USB_FMS_INCLUDED
> +EXPORT_SYMBOL(fsg_common_get);
> +#endif
Perhaps instead of checking it each time, define
a EXPORT_SYMBOL_GPL_IF_MODULE macro and when the macro is defined
provide a comment explaining why it is here?
>
> void fsg_common_put(struct fsg_common *common)
> {
> kref_put(&common->ref, fsg_common_release);
> }
> +#ifndef USB_FMS_INCLUDED
> +EXPORT_SYMBOL(fsg_common_put);
> +#endif
>
> /* check if fsg_num_buffers is within a valid range */
> static inline int fsg_num_buffers_validate(unsigned int fsg_num_buffers)
> int fsg_common_set_nluns(struct fsg_common *common, int nluns)
> {
> @@ -3146,6 +3186,21 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
> unsigned max_burst;
> int ret;
>
> +#ifndef USB_FMS_INCLUDED
> + struct fsg_opts *opts;
> + opts = container_of(f->fi, struct fsg_opts, func_inst);
> + if (!opts->no_configfs) {
> + ret = fsg_common_set_cdev(fsg->common, c->cdev,
> + fsg->common->can_stall);
> + if (ret)
> + return ret;
> + fsg_common_set_inquiry_string(fsg->common, 0, 0);
> + ret = fsg_common_run_thread(fsg->common);
> + if (ret)
> + return ret;
> + }
> +#endif
> +
> fsg->gadget = gadget;
>
> /* New interface */
> @@ -3197,7 +3252,27 @@ autoconf_fail:
> return -ENOTSUPP;
> }
>
> -/****************************** ADD FUNCTION ******************************/
> +/****************************** ALLOCATE FUNCTION *************************/
> +
> +#ifdef USB_FMS_INCLUDED
> +
> +static void old_fsg_unbind(struct usb_configuration *c, struct usb_function *f)
Perhaps __deprecated? Also, why name change?
> +{
> + struct fsg_dev *fsg = fsg_from_func(f);
> + struct fsg_common *common = fsg->common;
> +
> + DBG(fsg, "unbind\n");
> + if (fsg->common->fsg == fsg) {
> + fsg->common->new_fsg = NULL;
> + raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> + /* FIXME: make interruptible or killable somehow? */
> + wait_event(common->fsg_wait, common->fsg != fsg);
> + }
> +
> + fsg_common_put(common);
> + usb_free_all_descriptors(&fsg->function);
> + kfree(fsg);
> +}
>
> static int fsg_bind_config(struct usb_composite_dev *cdev,
> struct usb_configuration *c,
> @@ -3234,6 +3309,111 @@ static int fsg_bind_config(struct usb_composite_dev *cdev,
> return rc;
> }
>
> +#else
> +
> +static void fsg_free_inst(struct usb_function_instance *fi)
> +{
> + struct fsg_opts *opts;
> +
> + opts = container_of(fi, struct fsg_opts, func_inst);
> + fsg_common_put(opts->common);
> + kfree(opts);
> +}
> +
> +static struct usb_function_instance *fsg_alloc_inst(void)
> +{
> + struct fsg_opts *opts;
> + int ret;
Perhaps call it “rc”, since technically it's not return value from the
function.
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts)
> + return ERR_PTR(-ENOMEM);
> + opts->func_inst.free_func_inst = fsg_free_inst;
> + opts->common = fsg_common_setup(opts->common, false);
> + if (IS_ERR(opts->common)) {
> + ret = PTR_ERR(opts->common);
> + goto release_opts;
> + }
> + ret = fsg_common_set_nluns(opts->common, FSG_MAX_LUNS);
> + if (ret)
> + goto release_opts;
> +
> + ret = fsg_common_set_num_buffers(opts->common,
> + CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS);
> + if (ret)
> + goto release_luns;
> +
> + pr_info(FSG_DRIVER_DESC ", version: " FSG_DRIVER_VERSION "\n");
> +
> + return &opts->func_inst;
> +
> +release_luns:
> + kfree(opts->common->luns);
> +release_opts:
> + kfree(opts);
> + return ERR_PTR(ret);
> +}
> +static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
> +{
> + struct fsg_dev *fsg = fsg_from_func(f);
> + struct fsg_common *common = fsg->common;
> +
> + DBG(fsg, "unbind\n");
> + if (fsg->common->fsg == fsg) {
> + fsg->common->new_fsg = NULL;
> + raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> + /* FIXME: make interruptible or killable somehow? */
> + wait_event(common->fsg_wait, common->fsg != fsg);
> + }
> +
> + usb_free_all_descriptors(&fsg->function);
> +}
This looks almost identical to old_fsg_unbind(). Why not #ifdef just
the last part of the function?
> +
> +static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
> +{
> + struct fsg_opts *opts = container_of(fi, struct fsg_opts, func_inst);
> + struct fsg_common *common = opts->common;
> + struct fsg_dev *fsg;
> +
> + fsg = kzalloc(sizeof(*fsg), GFP_KERNEL);
> + if (unlikely(!fsg))
> + return ERR_PTR(-ENOMEM);
> +
> + fsg->function.name = FSG_DRIVER_DESC;
> + fsg->function.bind = fsg_bind;
> + fsg->function.unbind = fsg_unbind;
> + fsg->function.setup = fsg_setup;
> + fsg->function.set_alt = fsg_set_alt;
> + fsg->function.disable = fsg_disable;
> + fsg->function.free_func = fsg_free;
Tab character after “free_func”. Use either spaces or tabs
consistently.
> +
> + fsg->common = common;
> + /*
> + * Our caller holds a reference to common structure so we
> + * don't have to be worry about it being freed until we return
> + * from this function. So instead of incrementing counter now
> + * and decrement in error recovery we increment it only when
> + * call to usb_add_function() was successful.
> + */
> +
> + return &fsg->function;
> +}
> +
> +DECLARE_USB_FUNCTION_INIT(mass_storage, fsg_alloc_inst, fsg_alloc);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Michal Nazarewicz");
> +
> +#endif
>
> /************************* Module parameters *************************/
>
--
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
