Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753923AbaFDOUW (ORCPT ); Wed, 4 Jun 2014 10:20:22 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35101 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753479AbaFDOUU (ORCPT ); Wed, 4 Jun 2014 10:20:20 -0400 Date: Wed, 04 Jun 2014 16:20:16 +0200 Message-ID: From: Takashi Iwai To: Tom Gundersen Cc: linux-kernel@vger.kernel.org, Ming Lei , Greg Kroah-Hartman , Abhay Salunke , Stefan Roese , Arnd Bergmann , Kay Sievers Subject: Re: [PATCH] firmware loader: allow disabling of udev as firmware loader In-Reply-To: <1401733474-1767-1-git-send-email-teg@jklm.no> References: <1401733474-1767-1-git-send-email-teg@jklm.no> 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.3 (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 At Mon, 2 Jun 2014 20:24:34 +0200, Tom Gundersen wrote: > > Currently (at least) the dell-rbu driver selects FW_LOADER_USER_HELPER, > which means that distros can't really stop loading firmware through udev > without breaking other users (though some have). > > Ideally we would remove/disable the udev firmware helper in both the kernel > and in udev, but if we were to disable it in udev and not the kernel, the result > would be (seemingly) hung kernels as no one would be around to cancel firmware > requests. > > This patch allows udev firmware loading to be disabled while still allowing > non-udev firmware loading, as done by the dell-rbu driver, to continue > working. This is achieved by only using the fallback mechanism when the > uevent is suppressed. > > Tested with > FW_LOADER_USER_HELPER=n > LATTICE_ECP3_CONFIG=y > DELL_RBU=y > and udev without the firmware loading support, but I don't have the hardware > to test the lattice/dell drivers, so additional testing would be appreciated. The logic of this patch looks good to me, but the Kconfig items become confusing by this. Basically what we'd need is a Kconfig item deciding whether to build the user helper or not, in addition to a Kconfig item for deciding the fallback mode of request_firmware(). What about the patch like below instead? It's smaller and the meaning of Kconfig items are clearer. (In the final form, the help text change you added should be included there, too.) The only (and biggest) drawback is, however, that the user-selectable Kconfig would be actually renamed from CONFIG_FW_LOADER_USER_HELPER to CONFIG_FW_LOADER_USER_HELPER_FALLBACK. thanks, Takashi --- diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 8fa8deab6449..195b08f49209 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -144,8 +144,12 @@ config EXTRA_FIRMWARE_DIR some other directory containing the firmware files. config FW_LOADER_USER_HELPER + bool + +config FW_LOADER_USER_HELPER_FALLBACK bool "Fallback user-helper invocation for firmware loading" depends on FW_LOADER + select FW_LOADER_USER_HELPER default y help This option enables / disables the invocation of user-helper diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d276e33880be..e98fd78c5c40 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -100,9 +100,14 @@ static inline long firmware_loading_timeout(void) #define FW_OPT_UEVENT (1U << 0) #define FW_OPT_NOWAIT (1U << 1) #ifdef CONFIG_FW_LOADER_USER_HELPER -#define FW_OPT_FALLBACK (1U << 2) +#define FW_OPT_USERHELPER (1U << 2) #else -#define FW_OPT_FALLBACK 0 +#define FW_OPT_USERHELPER 0 +#endif +#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK +#define FW_OPT_FALLBACK FW_OPT_USERHELPER +#else +#define FW_OPT_FALLBACK 0 #endif struct firmware_cache { @@ -1111,7 +1116,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name, ret = fw_get_filesystem_firmware(device, fw->priv); if (ret) { - if (opt_flags & FW_OPT_FALLBACK) { + if (opt_flags & FW_OPT_USERHELPER) { dev_warn(device, "Direct firmware load failed with error %d\n", ret); @@ -1277,7 +1282,7 @@ request_firmware_nowait( fw_work->context = context; fw_work->cont = cont; fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK | - (uevent ? FW_OPT_UEVENT : 0); + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER); if (!try_module_get(module)) { kfree(fw_work); -- 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/