Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938080AbdD0B4p (ORCPT ); Wed, 26 Apr 2017 21:56:45 -0400 Received: from mx2.suse.de ([195.135.220.15]:38369 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S936458AbdD0B4g (ORCPT ); Wed, 26 Apr 2017 21:56:36 -0400 Date: Thu, 27 Apr 2017 03:56:32 +0200 From: "Luis R. Rodriguez" To: "Coelho, Luciano" Cc: "gregkh@linuxfoundation.org" , "mcgrof@kernel.org" , "linux-kernel@vger.kernel.org" , "pjones@redhat.com" , "moritz.fischer@ettus.com" , "takahiro.akashi@linaro.org" , "dhowells@redhat.com" , "pmladek@suse.com" , "Berg, Johannes" , "rjw@rjwysocki.net" , "yi1.li@linux.intel.com" , "kvalo@codeaurora.org" , "luto@kernel.org" , "arend.vanspriel@broadcom.com" , "rafal@milecki.pl" , "dwmw2@infradead.org" , "wagi@monom.org" , "atull@opensource.altera.com" , "Grumbach, Emmanuel" Subject: Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend Message-ID: <20170427015632.GG28800@wotan.suse.de> References: <20170330032450.17121-1-mcgrof@kernel.org> <20170330032450.17121-2-mcgrof@kernel.org> <1491460726.28535.42.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1491460726.28535.42.camel@intel.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2367 Lines: 63 On Thu, Apr 06, 2017 at 06:38:47AM +0000, Coelho, Luciano wrote: > Hi Luis, > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index ac350c518e0c..d2e2d83aaf26 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv) > > > > static LIST_HEAD(pending_fw_head); > > > > -/* reboot notifier for avoid deadlock with usermode_lock */ > > static int fw_shutdown_notify(struct notifier_block *unused1, > > unsigned long unused2, void *unused3) > > { > > - mutex_lock(&fw_lock); > > - while (!list_empty(&pending_fw_head)) > > - __fw_load_abort(list_first_entry(&pending_fw_head, > > - struct firmware_buf, > > - pending_list)); > > - mutex_unlock(&fw_lock); > > + /* > > + * Kill all pending fallback requests to avoid both stalling shutdown, > > + * and avoid a deadlock with the usermode_lock. > > + */ > > + kill_pending_fw_fallback_reqs(false); > > You are calling this function before you declare it, this won't compile > when CONFIG_FW_LOADER_USER_HELPER is not set. Actually allnoconfig compiles fine given the code in question is needed only when CONFIG_FW_LOADER_USER_HELPER is loaded, however it does fail to compile when CONFIG_FW_LOADER_USER_HELPER is set, and its odd that 0-day did not pick that up. I've fixed this by moving kill_pending_fw_fallback_reqs() up above in a separate patch first. > > @@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware, > > return _request_firmware_load(fw_priv, opt_flags, timeout); > > } > > > > -#ifdef CONFIG_PM_SLEEP > > -/* kill pending requests without uevent to avoid blocking suspend */ > > -static void kill_requests_without_uevent(void) > > +static void kill_pending_fw_fallback_reqs(bool only_kill_custom) > > { > > struct firmware_buf *buf; > > struct firmware_buf *next; > > > > mutex_lock(&fw_lock); > > list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) { > > - if (!buf->need_uevent) > > + if (!only_kill_custom) > > + __fw_load_abort(buf); > > + else if (!buf->need_uevent) > > __fw_load_abort(buf); > Why not use this? > > if (!only_kill_custom || !buf->need_uevent) > __fw_load_abort(buf); Sure thing! Thanks for the review ! Luis