Re: [PATCH v3] firmware_loader: Abort new fw load request once firmware core knows about reboot

[Mukesh Ojha]

On 10/20/2023 12:36 AM, Luis Chamberlain wrote:
> On Thu, Oct 05, 2023 at 10:37:00AM +0530, Mukesh Ojha wrote:
> > There could be following scenario where there is a ongoing reboot
> > is going from processA which tries to call all the reboot notifier
> > callback and one of them is firmware reboot call which tries to
> > abort all the ongoing firmware userspace request under fw_lock but
> > there could be another processB which tries to do request firmware,
> > which came just after abort done from ProcessA and ask for userspace
> > to load the firmware and this can stop the ongoing reboot ProcessA
> > to stall for next 60s(default timeout) which may not be expected
> > behaviour everyone like to see, instead we should abort any firmware
> > load request which came once firmware knows about the reboot through
> > notification.
> >
> >        ProcessA                             ProcessB
> >
> > kernel_restart_prepare
> >    blocking_notifier_call_chain
> >     fw_shutdown_notify
> >       kill_pending_fw_fallback_reqs
> >        __fw_load_abort
> >         fw_state_aborted	               request_firmware
> >           __fw_state_set                 firmware_fallback_sysfs
> > ...                                       fw_load_from_user_helper
> > ..                                         ...
> > .                                          ..
> >                                              usermodehelper_read_trylock
> >                                               fw_load_sysfs_fallback
> >                                                fw_sysfs_wait_timeout
> > usermodehelper_disable
> >   __usermodehelper_disable
> >    down_write()
> >
> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > ---
> > Changes from v2: https://lore.kernel.org/lkml/1696431327-7369-1-git-send-email-quic_mojha@xxxxxxxxxxx/
> >   - Renamed the flag to fw_abort_load.
> >
> > Changes from v1: https://lore.kernel.org/lkml/1694773288-15755-1-git-send-email-quic_mojha@xxxxxxxxxxx/
> >   - Renamed the flag to fw_load_abort.
> >   - Kept the flag under fw_lock.
> >   - Repharsed commit text.
> >
> >   drivers/base/firmware_loader/fallback.c | 6 +++++-
> >   drivers/base/firmware_loader/firmware.h | 1 +
> >   drivers/base/firmware_loader/main.c     | 1 +
> >   3 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> > index bf68e3947814..a162020e98f2 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -57,6 +57,10 @@ void kill_pending_fw_fallback_reqs(bool only_kill_custom)
>
> This routine uses a bool for only_kill_custom for when the events we
> should kill ar ecustom or not. Piggy backing on it to assume that the
> negative of value represents a shutdown is abusing the semantics
> and muddies the waters. So to avoid that, just extend the arguments
> to kill_pending_fw_fallback_reqs() for a new bool shutdown, that allows
> the code to be clearer and the intent is kept clear.

Let me understand what does 'only_kill_custom' do,

1. It gets called from reboot notifier where it's default value is 
false. So, the intention here is to kill all the ongoing request along
with !buf->need_uevent ?

commit c4b768934be613fb882e4e4090946218d76c8e1b
Author: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
Date:   Tue May 2 01:31:03 2017 -0700

    firmware: share fw fallback killing on reboot/suspend

    We kill pending fallback requests on suspend and reboot,
    the only difference is that on suspend we only kill custom
    fallback requests. Provide a wrapper that lets us customize
    the request with a flag.

    This also lets us simplify the #ifdef'ery over the calls.

    Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
    Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

2. The second call is from fw_pm_notify() which is only
interested in aborting non-uevent calls.

And in this patch since, we are calling it from reboot which is the 
first case, so piggybacking on the 'only_kill_custom' would be fine for 
this patch. However, if you think we should rename this
'only_kill_custom' to something like its inverse 'kill_all' and reverse 
the below check to be more meaningful now ?

   		if (kill_all || !fw_priv->need_uevent)

> >   		if (!fw_priv->need_uevent || !only_kill_custom)
> >   			 __fw_load_abort(fw_priv);
> >   	}
> > +
> > +	if (!only_kill_custom)
> > +		fw_abort_load = true;
> > +
> >   	mutex_unlock(&fw_lock);
> >   }
> >   
> > @@ -86,7 +90,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
> >   	}
> >   
> >   	mutex_lock(&fw_lock);
> > -	if (fw_state_is_aborted(fw_priv)) {
> > +	if (fw_abort_load || fw_state_is_aborted(fw_priv)) {
>
> However, do we really need this ? Could we just use:
>
> if (system_state == SYSTEM_HALT ||
>      system_state == SYSTEM_POWER_OFF ||
>      system_state == SYSTEM_RESTART ||
>      fw_state_is_aborted(fw_priv))
>
> ?

There is slight window where system_state is not yet set and other
reboot notifiers after firmware one are running with ongoing
request_firmware().

-Mukesh

>    Luis