From patchwork Mon Apr 4 12:57:25 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Nazarewicz?= X-Patchwork-Id: 8740921 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0293AC0553 for ; Mon, 4 Apr 2016 12:57:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B61D02022A for ; Mon, 4 Apr 2016 12:57:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D680C2027D for ; Mon, 4 Apr 2016 12:57:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753704AbcDDM5d (ORCPT ); Mon, 4 Apr 2016 08:57:33 -0400 Received: from mail-lf0-f54.google.com ([209.85.215.54]:34968 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752017AbcDDM5b convert rfc822-to-8bit (ORCPT ); Mon, 4 Apr 2016 08:57:31 -0400 Received: by mail-lf0-f54.google.com with SMTP id c126so50448134lfb.2 for ; Mon, 04 Apr 2016 05:57:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:from:to:cc:subject:in-reply-to:organization:references :user-agent:face:date:message-id:mime-version :content-transfer-encoding; bh=HNayrumauDgQX1M3YgbWtUeimQCEwA9U7pz6ERob4Go=; b=CZnLDCJ3M6d/cHw58dd3E4CAqB9bSUeVxKfa/b1Y5y/HSx2K/r0Zr4suBvR/ri6loL jg6VhmNrK55/OzKt/gkXr3gH9LJpsFCtZHr2BX18Fj3MF1G56QpuKdXE7Gjm9/qev+Hr U96I4C3gan8zbkqa9oq+4uK9/utBafOVT6wb1+2Ovo1KhlDhbFNTVaYzcC/b4r534vJY LMIQuylbUWLOxAFLHPdzl9HOrevFm/Aqs4fsVKT4aNEMXPApApYl6E51OFqQY10gIQkE Nb67PisRoPudgndjQJnagvWpysE0vyA+ywJLOy43YmTBWYwTXECr5xt+TjWhyt59cn+M X3JA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:in-reply-to :organization:references:user-agent:face:date:message-id :mime-version:content-transfer-encoding; bh=HNayrumauDgQX1M3YgbWtUeimQCEwA9U7pz6ERob4Go=; b=SqFsOUti/+I9ldwaz3C7YAOR8+wycNw1TSjPT9b2j9gfQaJ+kH0ECEF+Q0iTPI2/Vu +RWKZU05vZmUVWGhj8k0oHh2QODz9XSiBTYksi2lqZfSXonmjwCJA5zihhQmgk/5fQhf d7lMTiRv2AtxJt/A3mCUJS5KW7wirWErOh8kKDSb0mojxOhjklViRgEiKW3HCRmGAtzu 9fKZtgF20LwQaHr7K81yRj2vVXnyGy2Zqe4mv0hizzWg7Yx6wYOpAaz77ouMyCDeYseN gxH9LZN10JupwYI14S8bhFY0WVtLdYXmHnuOy/0iSiYg32DOkPol7TkZCdu9fiBXjDNL 3wEg== X-Gm-Message-State: AD7BkJICCOPi2m7cfOfHL73AY/6F0Wecv5/1QPu1DlMj550K/MqYwNOb7HkLnCsfOcli9jJH X-Received: by 10.194.216.99 with SMTP id op3mr18627673wjc.26.1459774649091; Mon, 04 Apr 2016 05:57:29 -0700 (PDT) Received: from mpn-glaptop ([2620:0:105f:310:b57e:eb12:1445:46d7]) by smtp.gmail.com with ESMTPSA id 198sm13881379wml.22.2016.04.04.05.57.26 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 04 Apr 2016 05:57:26 -0700 (PDT) From: Michal Nazarewicz To: Alan Stern Cc: Ivaylo Dimitrov , Felipe Balbi , Tony Lindgren , Bin Liu , pali =?utf-8?Q?Roh=C3=A1r?= , USB list , linux-omap@vger.kernel.org, Greg Kroah-Hartman , Robert Baldyga , Andrzej Pietrasiewicz Subject: Re: USB gadgets with configfs hang reboot In-Reply-To: Organization: http://mina86.com/ References: User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.1 (x86_64-unknown-linux-gnu) Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII= X-Face: -TR8(rDTHy/(xl?SfWd1|3:TTgDIatE^t'vop%*gVg[kn$t{EpK(P"VQ=~T2#ysNmJKN$"yTRLB4YQs$4{[.]Fc1)*O]3+XO^oXM>Q#b^ix, O)Zbn)q[y06$`e3?C)`CwR9y5riE=fv^X@x$y?D:XO6L&x4f-}}I4=VRNwiA^t1-ZrVK^07.Pi/57c_du'& X-PGP: 50751FF4 X-PGP-FP: AC1F 5F5C D418 88F8 CC84 5858 2060 4012 5075 1FF4 X-Hashcash: 1:20:160404:linux-usb@vger.kernel.org::L491vHp+saov0wAe:0000000000000000000000000000000000000Pvc X-Hashcash: 1:20:160404:felipe.balbi@linux.intel.com::0xETxoBS4UNXgu/5:0000000000000000000000000000000001FEl X-Hashcash: 1:20:160404:ivo.g.dimitrov.75@gmail.com::Pxjr1z9Z87+Dssw/:00000000000000000000000000000000002YTQ X-Hashcash: 1:20:160404:b-liu@ti.com::vjZC8Adclkdb7Vrs:000003Fiw X-Hashcash: 1:20:160404:andrzej.p@samsung.com::LismldlYYdYlLFbf:00000000000000000000000000000000000000002kSS X-Hashcash: 1:20:160404:stern@rowland.harvard.edu::F4nwF2xvvw4HQg3v:0000000000000000000000000000000000003gPS X-Hashcash: 1:20:160404:pali.rohar@gmail.com::6qqIt/c2OuBBvmqd:000000000000000000000000000000000000000003cwV X-Hashcash: 1:20:160404:gregkh@linuxfoundation.org::QyN4OSaD8HA3oIez:000000000000000000000000000000000005jWJ X-Hashcash: 1:20:160404:linux-omap@vger.kernel.org::aiqt0d25gzbNN31z:000000000000000000000000000000000004lpi X-Hashcash: 1:20:160404:r.baldyga@samsung.com::iCilrY1WHCBrvdsq:00000000000000000000000000000000000000007IZm X-Hashcash: 1:20:160404:tony@atomide.com::nAkQpIiP0RXC4g+N:0CGzL Date: Mon, 04 Apr 2016 14:57:25 +0200 Message-ID: MIME-Version: 1.0 Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Apr 02 2016, Alan Stern wrote: > On Sat, 2 Apr 2016, Michal Nazarewicz wrote: >> At the same time, mass storage should work fine if it’s bound to >> multiple configurations. Only one configuration can be active at any >> given time so interfaces on different configurations cannot interfere >> with each other. > Yes, it _should_. But it doesn't with the nokia legacy driver. > I don't know if this has any connection with configfs; it could be > a problem with the way f_mass_storage interacts with the composite > core. I believe the failure is related to the thread being started twice where it indeed shouldn’t. >> The problem we are having is that when mass storage is added to >> a configuration, fsg_bind is called and it starts the thread. > This is what I'm not sure about. Which callbacks does the composite > core invoke when a config is installed or uninstalled? usb_add_config is what is called to create a usb_configuration. It initialises the structure and passes it to a callback bind function (most code removed for brevity): int usb_add_config(struct usb_composite_dev *cdev, struct usb_configuration *config, int (*bind)(struct usb_configuration *)) { usb_add_config_only(cdev, config); bind(config); /* set_alt(), or next bind(), sets up ep->claimed as needed */ usb_ep_autoconfig_reset(cdev->gadget); return 0; } The bind callback calls usb_add_function to add usb_function to a newly created usb_configuration (again, most code removed for brevity): int usb_add_function(struct usb_configuration *config, struct usb_function *function) { function->config = config; value = function->bind(config, function); return 0; } For mass_storage, function->bind is fsg_bind (ditto): static struct usb_function *fsg_alloc(struct usb_function_instance *fi) { struct fsg_dev *fsg kzalloc(sizeof(*fsg), GFP_KERNEL); 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; fsg->common = common; return &fsg->function; } > Those callbacks should be where the thread is started and stopped. Starting thread in that callback is what is happening right now and because single usb_function_instance can have multiple usb_function structures each binding to separate usb_configuration, this causes problem where the thread is started multiple times. This is also why the thread is not stopped in fsg_unbind but only once fsg_common structure is released. Conceptually, the thread should be started when fsg_common structure is created (which is at the same time when usb_function_instance is created) and stopped when fsg_common is released. At this point, I’m not entirely sure if there is a reason why this isn’t the case. The only reason I can think of is that starting the thread right away may be considered wasteful since the thread won’t have anything to do until the function is bound to a configuration. In the current code, there may also be issues where perhaps the thread would not get stopped if fsg_bind has never been called. Because of all that, I think the best course of action is to just check whether the thread is running and conditionally start it in fsg_bind, i.e.: This should get rid of all the confusion and just do the right thing. --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2979,20 +2979,7 @@ EXPORT_SYMBOL_GPL(fsg_common_set_inquiry_string); int fsg_common_run_thread(struct fsg_common *common) { - common->state = FSG_STATE_IDLE; - /* Tell the thread to start working */ - common->thread_task = - kthread_create(fsg_main_thread, common, "file-storage"); - if (IS_ERR(common->thread_task)) { - common->state = FSG_STATE_TERMINATED; - return PTR_ERR(common->thread_task); - } - - DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task)); - - wake_up_process(common->thread_task); - - return 0; + /* kill this function and all call sites */ } EXPORT_SYMBOL_GPL(fsg_common_run_thread); @@ -3005,6 +2992,7 @@ static void fsg_common_release(struct kref *ref) if (common->state != FSG_STATE_TERMINATED) { raise_exception(common, FSG_STATE_EXIT); wait_for_completion(&common->thread_notifier); + common->thread_task = NULL; } for (i = 0; i < ARRAY_SIZE(common->luns); ++i) { @@ -3050,9 +3038,21 @@ static int fsg_bind(struct usb_configuration *c, struct usb_function *f) if (ret) return ret; fsg_common_set_inquiry_string(fsg->common, NULL, NULL); - ret = fsg_common_run_thread(fsg->common); - if (ret) + } + + if (!common->thread_task) { + common->state = FSG_STATE_IDLE; + common->thread_task = + kthread_create(fsg_main_thread, common, "file-storage"); + if (IS_ERR(common->thread_task)) { + int ret = PTR_ERR(common->thread_task); + common->thread_task = NULL; + common->state = FSG_STATE_TERMINATED; return ret; + } + DBG(common, "I/O thread pid: %d\n", + task_pid_nr(common->thread_task)); + wake_up_process(common->thread_task); } fsg->gadget = gadget;