From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161120AbbE2Q7m (ORCPT ); Fri, 29 May 2015 12:59:42 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:44841 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752628AbbE2Q7e (ORCPT ); Fri, 29 May 2015 12:59:34 -0400 Date: Fri, 29 May 2015 11:57:06 -0500 From: Felipe Balbi To: Pali =?iso-8859-1?Q?Roh=E1r?= CC: , Krzysztof Opasiak , Greg Kroah-Hartman , , , Pavel Machek , Sebastian Reichel , Aaro Koskinen , Ivaylo Dimitrov Subject: Re: [PATCH] usb: gadget: nokia: Add mass storage driver to g_nokia Message-ID: <20150529165706.GH2026@saruman.tx.rr.com> Reply-To: References: <1422698010-2907-1-git-send-email-pali.rohar@gmail.com> <20150528145918.GR16509@pali> <20150528163431.GA25787@saruman.tx.rr.com> <201505282340.07878@pali> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7IgncvKP0CVPV/ZZ" Content-Disposition: inline In-Reply-To: <201505282340.07878@pali> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --7IgncvKP0CVPV/ZZ Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, May 28, 2015 at 11:40:07PM +0200, Pali Roh=E1r wrote: > On Thursday 28 May 2015 18:34:31 Felipe Balbi wrote: > > On Thu, May 28, 2015 at 04:59:18PM +0200, Pali Roh=E1r wrote: > > > On Thursday 28 May 2015 16:51:07 Krzysztof Opasiak wrote: > > > > On 05/28/2015 04:31 PM, Pali Roh=E1r wrote: > > > > >On Thursday 28 May 2015 16:27:44 Krzysztof Opasiak wrote: > > > > >>On 05/28/2015 09:47 AM, Pali Roh=E1r wrote: > > > > >>>On Saturday 31 January 2015 10:53:30 Pali Roh=E1r 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=E1r > > > > >>>>--- > > > > >>>> > > > > >>>> 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 > > > > >>>> =09 > > > > >>>> 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 =3D { > > > > >>>>+ .stall =3D 0, > > > > >>>>+ .luns =3D 2, > > > > >>>>+ .removable_count =3D 2, > > > > >>>>+ .removable =3D { 1, 1, }, > > > > >>>>+}; > > > > >>>>+ > > > > >>>>+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data); > > > > >>>>+ > > > > >>>>+#ifdef CONFIG_USB_GADGET_DEBUG_FILES > > > > >>>>+ > > > > >>>>+static unsigned int fsg_num_buffers =3D > > > > >>>>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 =3D > > > > >>>> { > > > > >>>> > > > > >>>>@@ -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 =3D NULL; > > > > >>>> struct usb_function *f_ecm; > > > > >>>> struct usb_function *f_obex2 =3D NULL; > > > > >>>> > > > > >>>>+ struct usb_function *f_msg; > > > > >>>>+ struct fsg_opts *fsg_opts; > > > > >>>> > > > > >>>> int status =3D 0; > > > > >>>> int obex1_stat =3D -1; > > > > >>>> int obex2_stat =3D -1; > > > > >>>> > > > > >>>>@@ -160,6 +189,12 @@ static int __init > > > > >>>>nokia_bind_config(struct usb_configuration *c) > > > > >>>> > > > > >>>> goto err_get_ecm; > > > > >>>> =09 > > > > >>>> } > > > > >>>> > > > > >>>>+ f_msg =3D usb_get_function(fi_msg); > > > > >>>>+ if (IS_ERR(f_msg)) { > > > > >>>>+ status =3D PTR_ERR(f_msg); > > > > >>>>+ goto err_get_msg; > > > > >>>>+ } > > > > >>>>+ > > > > >>>> > > > > >>>> if (!IS_ERR_OR_NULL(f_phonet)) { > > > > >>>> =09 > > > > >>>> phonet_stat =3D 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; > > > > >>>> =09 > > > > >>>> } > > > > >>>> > > > > >>>>+ > > > > >>>>+ fsg_opts =3D fsg_opts_from_func_inst(fi_msg); > > > > >>>>+ > > > > >>>>+ status =3D fsg_common_run_thread(fsg_opts->common); > > > > >>>>+ if (status) > > > > >>>>+ goto err_msg; > > > > >>>>+ > > > > >>>>+ status =3D usb_add_function(c, f_msg); > > > > >>>>+ if (status) > > > > >>>>+ goto err_msg; > > > > >>>>+ > > > > >>>> > > > > >>>> if (c =3D=3D &nokia_config_500ma_driver) { > > > > >>>> =09 > > > > >>>> f_acm_cfg1 =3D f_acm; > > > > >>>> f_ecm_cfg1 =3D f_ecm; > > > > >>>> f_phonet_cfg1 =3D f_phonet; > > > > >>>> f_obex1_cfg1 =3D f_obex1; > > > > >>>> f_obex2_cfg1 =3D f_obex2; > > > > >>>> > > > > >>>>+ f_msg_cfg1 =3D f_msg; > > > > >>>> > > > > >>>> } else { > > > > >>>> =09 > > > > >>>> f_acm_cfg2 =3D f_acm; > > > > >>>> f_ecm_cfg2 =3D f_ecm; > > > > >>>> f_phonet_cfg2 =3D f_phonet; > > > > >>>> f_obex1_cfg2 =3D f_obex1; > > > > >>>> f_obex2_cfg2 =3D f_obex2; > > > > >>>> > > > > >>>>+ f_msg_cfg2 =3D f_msg; > > > > >>>> > > > > >>>> } > > > > >>>> =09 > > > > >>>> return status; > > > > >>>> > > > > >>>>+err_msg: > > > > >>>>+ usb_remove_function(c, f_ecm); > > > > >>>> > > > > >>>> err_ecm: > > > > >>>> usb_remove_function(c, f_acm); > > > > >>>> =20 > > > > >>>> err_conf: > > > > >>>>@@ -211,6 +261,8 @@ err_conf: > > > > >>>> usb_remove_function(c, f_obex1); > > > > >>>> =09 > > > > >>>> if (!phonet_stat) > > > > >>>> =09 > > > > >>>> usb_remove_function(c, f_phonet); > > > > >>>> > > > > >>>>+ usb_put_function(f_msg); > > > > >>>> > > > > >>>>+err_get_msg: > > > > >>>> usb_put_function(f_ecm); > > > > >>>> =20 > > > > >>>> 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) { > > > > >>>> =20 > > > > >>>> struct usb_gadget *gadget =3D cdev->gadget; > > > > >>>> > > > > >>>>+ struct fsg_opts *fsg_opts; > > > > >>>>+ struct fsg_config fsg_config; > > > > >>>> > > > > >>>> int status; > > > > >>>> =09 > > > > >>>> status =3D 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; > > > > >>>> =09 > > > > >>>> } > > > > >>>> > > > > >>>>+ fi_msg =3D usb_get_function_instance("mass_storage"); > > > > >>>>+ if (IS_ERR(fi_msg)) { > > > > >>>>+ status =3D 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 =3D "Nokia"; > > > > >>>>+ fsg_config.product_name =3D "N900"; > > > > >>>>+ > > > > >>>>+ fsg_opts =3D fsg_opts_from_func_inst(fi_msg); > > > > >>>>+ fsg_opts->no_configfs =3D true; > > > > >>>>+ > > > > >>>>+ status =3D fsg_common_set_num_buffers(fsg_opts->common, > > > > >>>>fsg_num_buffers); + if (status) > > > > >>>>+ goto err_msg_inst; > > > > >>>>+ > > > > >>>>+ status =3D fsg_common_set_nluns(fsg_opts->common, > > > > >>>>fsg_config.nluns); + if (status) > > > > >>>>+ goto err_msg_buf; > > > > >>>>+ > > > > >>>>+ status =3D 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 =3D 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, + =20 > fsg_config.product_name); > > > > >>>>+ > > > > >>>> > > > > >>>> /* finally register the configuration */ > > > > >>>> status =3D usb_add_config(cdev, &nokia_config_500ma_driver, > > > > >>>> =09 > > > > >>>> nokia_bind_config); > > > > >>>> =09 > > > > >>>> if (status < 0) > > > > >>>> > > > > >>>>- goto err_ecm_inst; > > > > >>>>+ goto err_msg_set_cdev; > > > > >>>> > > > > >>>> status =3D usb_add_config(cdev, &nokia_config_100ma_driver, > > > > >>>> =09 > > > > >>>> nokia_bind_config); > > > > >>>> > > > > >>>>@@ -292,6 +381,14 @@ err_put_cfg1: > > > > >>>> if (!IS_ERR_OR_NULL(f_phonet_cfg1)) > > > > >>>> =09 > > > > >>>> usb_put_function(f_phonet_cfg1); > > > > >>>> =09 > > > > >>>> 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); > > > > >>>> =20 > > > > >>>> 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)) > > > > >>>> =09 > > > > >>>> 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. > > > >=20 > > > > Couldn't you simply use initramfs to compose your gadget? > > >=20 > > > 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. > >=20 > > 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 ? > >=20 > > 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. > >=20 > > cheers >=20 > In this my patch usb gadget support is removable (see fsg_mod_data). It= =20 > means that at runtime (when module is already loaded and initialized!)=20 > userspace can decide to export some block device via usb. And unexport=20 > it at every time. Also this patch does not export any device by default.= =20 > So it does not lock any block device (like MyDocs), so there is really=20 > no regressions like you though about MyDocs. usb networking, tty and=20 > also phone will still work via g_nokia. And if you do not export block=20 > 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 --=20 balbi --7IgncvKP0CVPV/ZZ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVaJpiAAoJEIaOsuA1yqRET2oP/0UXK3z7oQvQR+rcjXWQuhrP GmG36iSj9sjQrVvvF5UuZFbNu0pVVgwCj1GnWzDpsURCRzKtf6PQVQiHnljrRmv0 biib8vkRt3l/OmjKJBELnrS7fL4oVAvIDwfU5W+vtW+drdKeESrEydG1CWCWkM6o gZdHqqn91CnLY8sCMdM9GF8ABPM7UVRNIUrwiAWZ740pJc7i1c/fOieHFRWidOBA od5k2XUeU+ZpT87HQUaBnNWTT3WqF8wJf91vD21J84d/RnbWW8dKVOMKCol7qpI2 TkeYnqkOqgrnJzOn2n1hCs7eds5d5OvIR7u15lshULagySSbtAUQOoxDirGUi77M C+j2zo9WdiQapWAuKyw6kZkBxvWaNpjsRI/zblEN+dDwZWccW9AH7Z2PLB6OU5Ec ZRtzZxRzDKWiMHQ9CcGWx6G9bxcNmyUgejwgL9Ylv5UKGoFXI7uXW8cmoLQTIS28 89nkWVw4+8XLsMP/S4OZ6fNx4N5GaBjhNcmXoAs3k7hwe/3IBK3mwg4lz0gWFBdT 32MY9gltgTFbUl3OVYQnNgFvAXdXOdCSADktdQ3df0OIUrfATBmbx6lbKEGD0JRo DJ5X0wBuLV9Ed8egCM8f5Rp1a31+hgoG4HvvPv4s34msI9GPsG/nfoGQcrS/V3AZ b9klPFrMCwaWsYBngWmq =Fv5T -----END PGP SIGNATURE----- --7IgncvKP0CVPV/ZZ--