Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759753AbcDESfW (ORCPT ); Tue, 5 Apr 2016 14:35:22 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:43792 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751780AbcDESfU (ORCPT ); Tue, 5 Apr 2016 14:35:20 -0400 Date: Tue, 5 Apr 2016 14:35:19 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Michal Nazarewicz cc: Ivaylo Dimitrov , Tony Lindgren , , , Felipe Balbi , Bin Liu , Subject: Re: [PATCH] usb: f_mass_storage: test whether thread is running before starting another In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2825 Lines: 59 On Tue, 5 Apr 2016, Michal Nazarewicz wrote: > > On Tue, 5 Apr 2016, Michal Nazarewicz wrote: > >> When binding the function to usb_configuration, check whether the thread > >> is running before starting another one. Without that, when function > >> instance is added to multiple configurations, fsg_bing starts multiple > >> threads with all but the latest one being forgotten by the driver. This > >> leads to obvious thread leaks, possible lockups when trying to halt the > >> machine and possible more issues. > >> > >> This fixes issues with legacy/multi¹ gadget as well as configfs gadgets > >> when mass_storage function is added to multiple configurations. > >> > >> This change also simplifies API since the legacy gadgets no longer need > >> to worry about starting the thread by themselves (which was where bug > >> in legacy/multi was in the first place). > >> > >> ¹ I have no example failure though. Conclusion that legacy/multi has > >> a bug is based purely on me reading the code. > >> > >> Signed-off-by: Michal Nazarewicz > > On Tue, Apr 05 2016, Alan Stern wrote: > > This doesn't address the problem I raised in a previous email. > > Sharing one thread among several function instances in the same config > > will not work if one of them encounters an error. > > Each usb_function_instance has its own fsg_common and its own thread. > This was true in the past and is true with this patch as well. We're getting confused by the stupid terminology again. Yes, each usb_function_instance has its own fsg_common and its own thread. But multiple usb_function structures (each one being a separate function instance) can belong to the same usb_function_instance and they will all share the same fsg_common. That's what happened with the nokia driver. It creates one usb_function_instance with two usb_function structures. In this case they are in different configs, so sharing a thread doesn't matter. But it would matter if they were in the same config. > And unless I’m missing something, sharing a thread among multiple > usb_function’s does not prevent the driver from working correctly. Suppose one usb_function is carrying out an I/O operation while another one in the same config gets a Set-Interface request from the host. The request causes the driver to raise an FSG_STATE_CONFIG_CHANGE exception. When the thread sees this exception, it will abort the I/O that it is carrying out for the first usb_function. In other words, exceptions raised by one instance will affect the shared thread even when it's doing work for a different instance. > Having the thread run even when it’s not used may be considered wasteful > but that’s an orthogonal issue to the configfs failure. Yes. Having extra unused threads isn't terrible. Alan Stern