Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752010Ab3ELHUv (ORCPT ); Sun, 12 May 2013 03:20:51 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50316 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751405Ab3ELHUu (ORCPT ); Sun, 12 May 2013 03:20:50 -0400 Date: Sun, 12 May 2013 09:20:47 +0200 Message-ID: From: Takashi Iwai To: Ming Lei Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman Subject: Re: [PATCH 1/3] firmware: Avoid superfluous usermodehelper lock In-Reply-To: References: <1367996197-32748-1-git-send-email-tiwai@suse.de> <1367996197-32748-2-git-send-email-tiwai@suse.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3314 Lines: 74 At Sat, 11 May 2013 21:01:27 +0800, Ming Lei wrote: > > On Fri, May 10, 2013 at 5:32 PM, Takashi Iwai wrote: > > At Fri, 10 May 2013 09:25:51 +0800, > >> > >> Anyway, if you want to force killing loader, please only kill these > >> FW_ACTION_NOHOTPLUG before suspend and reboot, and do > >> not touch FW_ACTION_HOTPLUG. Is it OK for you? > > > > Note that, as with my patch, only the shutdown case is handled. Let's > > not mixing up suspend and shutdown behavior for now. > > > > I see no reason why we need to wait at shutdown even for > > FW_ACTION_HOTPLUG. At that point, there should be no longer > > user-space action. It means that the driver shouldn't get any more > > data to finish the f/w loading upon that point. Thus the only > > For example, when one ethernet driver is requesting its firmware, > the loader is forced to be killed before shutdown, so the driver can't set the > WoL any more in its shutdown(), then users start to complain it is a > regression. First off, this can't happen because, as mentioned earlier, the point we're discussing is already the moment after user-space is supposed to have been finished by init, i.e. there is already no udev. Thus the pending f/w request via usermode helper can never finish. So, it makes no sense to wait for any user-space action at that point. It just locks up. Secondly, if this would ever work, it's still just a luck. The protection is only about the usermode helper lock in the f/w loading code. This doesn't mean that the whole pending driver work would be finished. In other words, such a driver design is just broken. > That is why I suggest you to only kill requests of FW_ACTION_NOHOTPLUG. > > Also it isn't good to affect all drivers just for fixing two insane drivers. > > > possible consequence is the timeout, which is equivalent with the > > immediate abort of the operation. > > > > As mentioned earlier, the suspend behavior may be different. We want > > to retry the f/w load. Ideally, the f/w loader should abort and > > automatically retry after resume. In this case, also there is no big > > I don't think it is good idea since suspend() may need firmware to > change the device's power state. Considered that only two insane > drivers use FW_ACTION_NOHOTPLUG, we can kill the two before > suspend just like the case of shutdown. The same argument can be applied. The protection in f/w loading doesn't guarantee that the pending driver action will be finished before suspend. It protects only the udev reaction. But, it doesn't protect the action after it. > > reason to distinguish FW_ACTION_* types. Even for udev case, the > > action can be easily retried. > > > > Or, a cleaner solution would be to get rid of FW_ACTION_NOHOTPLUG > > completely. As Kay mentioned, this was a big mistake from the very > > It is still not a good idea, hackers may need FW_ACTION_NOHOTPLUG > to debug its driver with private firmwares, or examples like dell's BIOS update. Oh no, it's a badly designed interface. It should be never used. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/