From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753335Ab3KLG00 (ORCPT ); Tue, 12 Nov 2013 01:26:26 -0500 Received: from cantor2.suse.de ([195.135.220.15]:41185 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752610Ab3KLG0N (ORCPT ); Tue, 12 Nov 2013 01:26:13 -0500 Date: Tue, 12 Nov 2013 07:26:08 +0100 Message-ID: From: Takashi Iwai To: Ming Lei Cc: Linux Kernel Mailing List , Prarit Bhargava , Greg Kroah-Hartman , x86@kernel.org, amd64-microcode Subject: Re: [PATCH v2 3/3] firmware: Avoid bogus fallback warning In-Reply-To: References: <1384183278-19787-1-git-send-email-tiwai@suse.de> <1384183278-19787-4-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.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 Tue, 12 Nov 2013 09:40:24 +0800, Ming Lei wrote: At Tue, 12 Nov 2013 09:40:24 +0800, Ming Lei wrote: > > On Mon, Nov 11, 2013 at 11:21 PM, Takashi Iwai wrote: > > The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct > > firmware loading failure] introduced a new warning message about > > falling back to user helper, but this isn't true when > > CONFIG_FW_LOADER_USER_HELPER isn't set. > > > > For avoiding the confusion, add a proper ifdef. And now we can remove > > the dummy fw_load_from_user_helper(), too, since it's no longer called > > with CONFIG_FW_LOADER_USER_HELPER=n. > > > > Signed-off-by: Takashi Iwai > > --- > > drivers/base/firmware_class.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index 7f48a6ffb0df..bb03c71bd94d 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void) > > #endif > > > > #else /* CONFIG_FW_LOADER_USER_HELPER */ > > -static inline int > > -fw_load_from_user_helper(struct firmware *firmware, const char *name, > > - struct device *device, bool uevent, bool nowait, > > - long timeout) > > -{ > > - return -ENOENT; > > -} > > - > > /* No abort during direct loading */ > > #define is_fw_load_aborted(buf) false > > > > @@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name, > > if (ret) { > > dev_warn(device, "Direct firmware load failed with error %d\n", > > ret); > > +#ifdef CONFIG_FW_LOADER_USER_HELPER > > if (fallback) { > > dev_warn(device, "Falling back to user helper\n"); > > I think it is simpler to put above line at the entry of > fw_load_from_user_helper() > since we always do direct-loading first, and code should be cleaner. > > Thanks, > -- > Ming Lei > > > On Mon, Nov 11, 2013 at 11:21 PM, Takashi Iwai wrote: > > The commit [3e358ac2bb5b: firmware: Be a bit more verbose about direct > > firmware loading failure] introduced a new warning message about > > falling back to user helper, but this isn't true when > > CONFIG_FW_LOADER_USER_HELPER isn't set. > > > > For avoiding the confusion, add a proper ifdef. And now we can remove > > the dummy fw_load_from_user_helper(), too, since it's no longer called > > with CONFIG_FW_LOADER_USER_HELPER=n. > > > > Signed-off-by: Takashi Iwai > > --- > > drivers/base/firmware_class.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index 7f48a6ffb0df..bb03c71bd94d 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -940,14 +940,6 @@ static void kill_requests_without_uevent(void) > > #endif > > > > #else /* CONFIG_FW_LOADER_USER_HELPER */ > > -static inline int > > -fw_load_from_user_helper(struct firmware *firmware, const char *name, > > - struct device *device, bool uevent, bool nowait, > > - long timeout) > > -{ > > - return -ENOENT; > > -} > > - > > /* No abort during direct loading */ > > #define is_fw_load_aborted(buf) false > > > > @@ -1097,11 +1089,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name, > > if (ret) { > > dev_warn(device, "Direct firmware load failed with error %d\n", > > ret); > > +#ifdef CONFIG_FW_LOADER_USER_HELPER > > if (fallback) { > > dev_warn(device, "Falling back to user helper\n"); > > I think it is simpler to put above line at the entry of > fw_load_from_user_helper() > since we always do direct-loading first, and code should be cleaner. OK, that makes sense. While looking back at the code, I think the first warning ("Direct firmware load failed" should be suppressed, too, when no fallback is set. For example, non-existing firmware is no error at all for microcode driver, as a firmware is purely optional. Showing a warning at each failure would result in lots of bogus warnings and it'd confuse users as if something critical happened. So, the first dev_warn() is better in the "if (fallback)" block, IMO. I'm going to prepare the v3 patch series together with bit flags change. thanks, Takashi