Message-ID: <20150529165706.GH2026@saruman.tx.rr.com>
Date:	Fri, 29 May 2015 11:57:06 -0500
From:	Felipe Balbi <balbi@...com>
To:	Pali Rohár <pali.rohar@...il.com>
CC:	<balbi@...com>, Krzysztof Opasiak <k.opasiak@...sung.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Pavel Machek <pavel@....cz>,
	Sebastian Reichel <sre@...nel.org>,
	Aaro Koskinen <aaro.koskinen@....fi>,
	Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
Subject: Re: [PATCH] usb: gadget: nokia: Add mass storage driver to g_nokia

On Thu, May 28, 2015 at 11:40:07PM +0200, Pali Rohár wrote:
> 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@...il.com>
> > > > >>>>---
> > > > >>>>
> > > > >>>>  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.

All right, I tried merging it and it added build breaks to the tree.
I'll wait for another version for v4.3 merge window.

cheers

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists