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
