Re: [PATCH] usb: gadget: nokia: Add mass storage driver to g_nokia

From: Pali RohÃr
Date: Thu May 28 2015 - 17:40:23 EST

On Thursday 28 May 2015 18:34:31 Felipe Balbi wrote:
> On Thu, May 28, 2015 at 04:59:18PM +0200, Pali RohÃr wrote:
> > On Thursday 28 May 2015 16:51:07 Krzysztof Opasiak wrote:
> > > On 05/28/2015 04:31 PM, Pali RohÃr wrote:
> > > >On Thursday 28 May 2015 16:27:44 Krzysztof Opasiak wrote:
> > > >>On 05/28/2015 09:47 AM, Pali RohÃr wrote:
> > > >>>On Saturday 31 January 2015 10:53:30 Pali RohÃr wrote:
> > > >>>>This patch adds removable mass storage support to g_nokia
> > > >>>>gadget (for N900). It means that at runtime block device can
> > > >>>>be exported or unexported. So it does not export anything by
> > > >>>>default and thus allows to use MyDocs partition as before...
> > > >>>>
> > > >>>>Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
> > > >>>>---
> > > >>>>
> > > >>>> drivers/usb/gadget/legacy/Kconfig | 1 +
> > > >>>> drivers/usb/gadget/legacy/nokia.c | 102
> > > >>>> ++++++++++++++++++++++++++++++++++++- 2 files changed, 102
> > > >>>> insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>>diff --git a/drivers/usb/gadget/legacy/Kconfig
> > > >>>>b/drivers/usb/gadget/legacy/Kconfig index fd48ef3..36f6ba4
> > > >>>>100644
> > > >>>>--- a/drivers/usb/gadget/legacy/Kconfig
> > > >>>>+++ b/drivers/usb/gadget/legacy/Kconfig
> > > >>>>@@ -345,6 +345,7 @@ config USB_G_NOKIA
> > > >>>>
> > > >>>> select USB_F_OBEX
> > > >>>> select USB_F_PHONET
> > > >>>> select USB_F_ECM
> > > >>>>
> > > >>>>+ select USB_F_MASS_STORAGE
> > > >>>>
> > > >>>> help
> > > >>>>
> > > >>>> The Nokia composite gadget provides support for acm,
> > > >>>> obex and phonet in only one composite gadget driver.
> > > >>>>
> > > >>>>diff --git a/drivers/usb/gadget/legacy/nokia.c
> > > >>>>b/drivers/usb/gadget/legacy/nokia.c index 9b8fd70..a09bb50
> > > >>>>100644
> > > >>>>--- a/drivers/usb/gadget/legacy/nokia.c
> > > >>>>+++ b/drivers/usb/gadget/legacy/nokia.c
> > > >>>>@@ -24,6 +24,7 @@
> > > >>>>
> > > >>>> #include "u_phonet.h"
> > > >>>> #include "u_ecm.h"
> > > >>>> #include "gadget_chips.h"
> > > >>>>
> > > >>>>+#include "f_mass_storage.h"
> > > >>>>
> > > >>>> /* Defines */
> > > >>>>
> > > >>>>@@ -34,6 +35,29 @@ USB_GADGET_COMPOSITE_OPTIONS();
> > > >>>>
> > > >>>> USB_ETHERNET_MODULE_PARAMETERS();
> > > >>>>
> > > >>>>+static struct fsg_module_parameters fsg_mod_data = {
> > > >>>>+ .stall = 0,
> > > >>>>+ .luns = 2,
> > > >>>>+ .removable_count = 2,
> > > >>>>+ .removable = { 1, 1, },
> > > >>>>+};
> > > >>>>+
> > > >>>>+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
> > > >>>>+
> > > >>>>+#ifdef CONFIG_USB_GADGET_DEBUG_FILES
> > > >>>>+
> > > >>>>+static unsigned int fsg_num_buffers =
> > > >>>>CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS; +
> > > >>>>+#else
> > > >>>>+
> > > >>>>+/*
> > > >>>>+ * Number of buffers we will use.
> > > >>>>+ * 2 is usually enough for good buffering pipeline
> > > >>>>+ */
> > > >>>>+#define
> > > >>>>fsg_num_buffers CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS +
> > > >>>>+#endif /* CONFIG_USB_DEBUG */
> > > >>>>+
> > > >>>>
> > > >>>> #define NOKIA_VENDOR_ID 0x0421 /* Nokia */
> > > >>>> #define NOKIA_PRODUCT_ID 0x01c8 /* Nokia Gadget */
> > > >>>>
> > > >>>>@@ -94,6 +118,8 @@ static struct usb_function *f_obex1_cfg2;
> > > >>>>
> > > >>>> static struct usb_function *f_obex2_cfg2;
> > > >>>> static struct usb_function *f_phonet_cfg1;
> > > >>>> static struct usb_function *f_phonet_cfg2;
> > > >>>>
> > > >>>>+static struct usb_function *f_msg_cfg1;
> > > >>>>+static struct usb_function *f_msg_cfg2;
> > > >>>>
> > > >>>> static struct usb_configuration nokia_config_500ma_driver =
> > > >>>> {
> > > >>>>
> > > >>>>@@ -117,6 +143,7 @@ static struct usb_function_instance
> > > >>>>*fi_ecm;
> > > >>>>
> > > >>>> static struct usb_function_instance *fi_obex1;
> > > >>>> static struct usb_function_instance *fi_obex2;
> > > >>>> static struct usb_function_instance *fi_phonet;
> > > >>>>
> > > >>>>+static struct usb_function_instance *fi_msg;
> > > >>>>
> > > >>>> static int __init nokia_bind_config(struct
> > > >>>> usb_configuration *c) {
> > > >>>>
> > > >>>>@@ -125,6 +152,8 @@ static int __init
> > > >>>>nokia_bind_config(struct usb_configuration *c)
> > > >>>>
> > > >>>> struct usb_function *f_obex1 = NULL;
> > > >>>> struct usb_function *f_ecm;
> > > >>>> struct usb_function *f_obex2 = NULL;
> > > >>>>
> > > >>>>+ struct usb_function *f_msg;
> > > >>>>+ struct fsg_opts *fsg_opts;
> > > >>>>
> > > >>>> int status = 0;
> > > >>>> int obex1_stat = -1;
> > > >>>> int obex2_stat = -1;
> > > >>>>
> > > >>>>@@ -160,6 +189,12 @@ static int __init
> > > >>>>nokia_bind_config(struct usb_configuration *c)
> > > >>>>
> > > >>>> goto err_get_ecm;
> > > >>>>
> > > >>>> }
> > > >>>>
> > > >>>>+ f_msg = usb_get_function(fi_msg);
> > > >>>>+ if (IS_ERR(f_msg)) {
> > > >>>>+ status = PTR_ERR(f_msg);
> > > >>>>+ goto err_get_msg;
> > > >>>>+ }
> > > >>>>+
> > > >>>>
> > > >>>> if (!IS_ERR_OR_NULL(f_phonet)) {
> > > >>>>
> > > >>>> phonet_stat = usb_add_function(c, f_phonet);
> > > >>>> if (phonet_stat)
> > > >>>>
> > > >>>>@@ -187,21 +222,36 @@ static int __init
> > > >>>>nokia_bind_config(struct usb_configuration *c)
> > > >>>>
> > > >>>> pr_debug("could not bind ecm config %d\n", status);
> > > >>>> goto err_ecm;
> > > >>>>
> > > >>>> }
> > > >>>>
> > > >>>>+
> > > >>>>+ fsg_opts = fsg_opts_from_func_inst(fi_msg);
> > > >>>>+
> > > >>>>+ status = fsg_common_run_thread(fsg_opts->common);
> > > >>>>+ if (status)
> > > >>>>+ goto err_msg;
> > > >>>>+
> > > >>>>+ status = usb_add_function(c, f_msg);
> > > >>>>+ if (status)
> > > >>>>+ goto err_msg;
> > > >>>>+
> > > >>>>
> > > >>>> if (c == &nokia_config_500ma_driver) {
> > > >>>>
> > > >>>> f_acm_cfg1 = f_acm;
> > > >>>> f_ecm_cfg1 = f_ecm;
> > > >>>> f_phonet_cfg1 = f_phonet;
> > > >>>> f_obex1_cfg1 = f_obex1;
> > > >>>> f_obex2_cfg1 = f_obex2;
> > > >>>>
> > > >>>>+ f_msg_cfg1 = f_msg;
> > > >>>>
> > > >>>> } else {
> > > >>>>
> > > >>>> f_acm_cfg2 = f_acm;
> > > >>>> f_ecm_cfg2 = f_ecm;
> > > >>>> f_phonet_cfg2 = f_phonet;
> > > >>>> f_obex1_cfg2 = f_obex1;
> > > >>>> f_obex2_cfg2 = f_obex2;
> > > >>>>
> > > >>>>+ f_msg_cfg2 = f_msg;
> > > >>>>
> > > >>>> }
> > > >>>>
> > > >>>> return status;
> > > >>>>
> > > >>>>+err_msg:
> > > >>>>+ usb_remove_function(c, f_ecm);
> > > >>>>
> > > >>>> err_ecm:
> > > >>>> usb_remove_function(c, f_acm);
> > > >>>>
> > > >>>> err_conf:
> > > >>>>@@ -211,6 +261,8 @@ err_conf:
> > > >>>> usb_remove_function(c, f_obex1);
> > > >>>>
> > > >>>> if (!phonet_stat)
> > > >>>>
> > > >>>> usb_remove_function(c, f_phonet);
> > > >>>>
> > > >>>>+ usb_put_function(f_msg);
> > > >>>>
> > > >>>>+err_get_msg:
> > > >>>> usb_put_function(f_ecm);
> > > >>>>
> > > >>>> err_get_ecm:
> > > >>>> usb_put_function(f_acm);
> > > >>>>
> > > >>>>@@ -227,6 +279,8 @@ err_get_acm:
> > > >>>> static int __init nokia_bind(struct usb_composite_dev
> > > >>>> *cdev) {
> > > >>>>
> > > >>>> struct usb_gadget *gadget = cdev->gadget;
> > > >>>>
> > > >>>>+ struct fsg_opts *fsg_opts;
> > > >>>>+ struct fsg_config fsg_config;
> > > >>>>
> > > >>>> int status;
> > > >>>>
> > > >>>> status = usb_string_ids_tab(cdev, strings_dev);
> > > >>>>
> > > >>>>@@ -267,11 +321,46 @@ static int __init nokia_bind(struct
> > > >>>>usb_composite_dev *cdev)
> > > >>>>
> > > >>>> goto err_acm_inst;
> > > >>>>
> > > >>>> }
> > > >>>>
> > > >>>>+ fi_msg = usb_get_function_instance("mass_storage");
> > > >>>>+ if (IS_ERR(fi_msg)) {
> > > >>>>+ status = PTR_ERR(fi_msg);
> > > >>>>+ goto err_ecm_inst;
> > > >>>>+ }
> > > >>>>+
> > > >>>>+ /* set up mass storage function */
> > > >>>>+ fsg_config_from_params(&fsg_config, &fsg_mod_data,
> > > >>>>fsg_num_buffers); + fsg_config.vendor_name = "Nokia";
> > > >>>>+ fsg_config.product_name = "N900";
> > > >>>>+
> > > >>>>+ fsg_opts = fsg_opts_from_func_inst(fi_msg);
> > > >>>>+ fsg_opts->no_configfs = true;
> > > >>>>+
> > > >>>>+ status = fsg_common_set_num_buffers(fsg_opts->common,
> > > >>>>fsg_num_buffers); + if (status)
> > > >>>>+ goto err_msg_inst;
> > > >>>>+
> > > >>>>+ status = fsg_common_set_nluns(fsg_opts->common,
> > > >>>>fsg_config.nluns); + if (status)
> > > >>>>+ goto err_msg_buf;
> > > >>>>+
> > > >>>>+ status = fsg_common_set_cdev(fsg_opts->common, cdev,
> > > >>>>fsg_config.can_stall); + if (status)
> > > >>>>+ goto err_msg_set_nluns;
> > > >>>>+
> > > >>>>+ fsg_common_set_sysfs(fsg_opts->common, true);
> > > >>>>+
> > > >>>>+ status = fsg_common_create_luns(fsg_opts->common,
> > > >>>>&fsg_config); + if (status)
> > > >>>>+ goto err_msg_set_nluns;
> > > >>>>+
> > > >>>>+ fsg_common_set_inquiry_string(fsg_opts->common,
> > > >>>>fsg_config.vendor_name, +
fsg_config.product_name);
> > > >>>>+
> > > >>>>
> > > >>>> /* finally register the configuration */
> > > >>>> status = usb_add_config(cdev, &nokia_config_500ma_driver,
> > > >>>>
> > > >>>> nokia_bind_config);
> > > >>>>
> > > >>>> if (status < 0)
> > > >>>>
> > > >>>>- goto err_ecm_inst;
> > > >>>>+ goto err_msg_set_cdev;
> > > >>>>
> > > >>>> status = usb_add_config(cdev, &nokia_config_100ma_driver,
> > > >>>>
> > > >>>> nokia_bind_config);
> > > >>>>
> > > >>>>@@ -292,6 +381,14 @@ err_put_cfg1:
> > > >>>> if (!IS_ERR_OR_NULL(f_phonet_cfg1))
> > > >>>>
> > > >>>> usb_put_function(f_phonet_cfg1);
> > > >>>>
> > > >>>> usb_put_function(f_ecm_cfg1);
> > > >>>>
> > > >>>>+err_msg_set_cdev:
> > > >>>>+ fsg_common_remove_luns(fsg_opts->common);
> > > >>>>+err_msg_set_nluns:
> > > >>>>+ fsg_common_free_luns(fsg_opts->common);
> > > >>>>+err_msg_buf:
> > > >>>>+ fsg_common_free_buffers(fsg_opts->common);
> > > >>>>+err_msg_inst:
> > > >>>>+ usb_put_function_instance(fi_msg);
> > > >>>>
> > > >>>> err_ecm_inst:
> > > >>>> usb_put_function_instance(fi_ecm);
> > > >>>>
> > > >>>> err_acm_inst:
> > > >>>>@@ -325,7 +422,10 @@ static int __exit nokia_unbind(struct
> > > >>>>usb_composite_dev *cdev)
> > > >>>>
> > > >>>> usb_put_function(f_acm_cfg2);
> > > >>>> usb_put_function(f_ecm_cfg1);
> > > >>>> usb_put_function(f_ecm_cfg2);
> > > >>>>
> > > >>>>+ usb_put_function(f_msg_cfg1);
> > > >>>>+ usb_put_function(f_msg_cfg2);
> > > >>>>
> > > >>>>+ usb_put_function_instance(fi_msg);
> > > >>>>
> > > >>>> usb_put_function_instance(fi_ecm);
> > > >>>> if (!IS_ERR(fi_obex2))
> > > >>>>
> > > >>>> usb_put_function_instance(fi_obex2);
> > > >>>
> > > >>>Opening discussion about this patch again as this is still
> > > >>>only one solution how to use use mass storage without
> > > >>>breaking other stuff...
> > > >>>
> > > >>>Please understand finally that usb networking is very very
> > > >>>important for development on this device and usb mass storage
> > > >>>is needed for transferring bigger data.
> > > >>>
> > > >>>Also to clarify potential regressions: This patch adds
> > > >>>*optional* usb mass storage support and block device can be
> > > >>>attached/detached to driver at runtime. It does *not* enforce
> > > >>>to export some (mmc) device automatically. It is optional and
> > > >>>userspace can decide...
> > > >>>
> > > >>>So MyDocs N900 partition is not automatically enforced to be
> > > >>>exported via usb (as some people thought) and so it does not
> > > >>>break usb networking or mass storage mode or MyDocs parition
> > > >>>on N900 with Maemo system.
> > > >>
> > > >>Instead of adding mass storage to legacy gadget you may switch
> > > >>to configfs interface and compose equivalent of g_nokia + mass
> > > >>storage without any changes in kernel.
> > > >>
> > > >>Best regards,
> > > >
> > > >That requires userspace. And there are developers who use
> > > >nfsroot via usb networking and so userspace will be there after
> > > >properly initializing usb gadget... So it is not easy and
> > > >reason why I'm opening this discussion again.
> > >
> > > Couldn't you simply use initramfs to compose your gadget?
> >
> > This is another next complication... I could spend time on hacking
> > serious problems on Nokia N900 (camera not working, bluetooth not
> > working, improving cellular audio/voice calls) or start playing
> > with initramfs and other kernel infrastructure just because usb is
> > broken and small fix which enable all needed usb functionality
> > (usb networking, usb TTY, usb phonet, usb mass storage) cannot be
> > accepted and merged because it really helps people to develop...
> > :-( I see that it is under legacy drivers, but without it is hard
> > (and maybe not possible) to develop for Nokia N900 device. Nobody
> > until now proposed any other patch which fix our problems and
> > provide working usb functionality for mass storage, network, TTY
> > and phonet.
>
> Sure this causes no problems to original Maemo userland ? Also, IIRC,
> we were running out of both FIFO space and physical endpoints with
> g_nokia already. Do all functions still work if used all at once ?
>
> If you can confirm these two questions I'll take this patch. As long
> as there are no regressions with original userland from Nokia, it
> should be fine.
>
> cheers

In this my patch usb gadget support is removable (see fsg_mod_data). It
means that at runtime (when module is already loaded and initialized!)
userspace can decide to export some block device via usb. And unexport
it at every time. Also this patch does not export any device by default.
So it does not lock any block device (like MyDocs), so there is really
no regressions like you though about MyDocs. usb networking, tty and
also phone will still work via g_nokia. And if you do not export block
device MyDocs it will stay normally mounted in N900 VFS.

--
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.