lkml.org 
[lkml]   [2023]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v3] firmware_loader: Abort new fw load request once firmware core knows about reboot
From


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@quicinc.com>
>> ---
>> Changes from v2: https://lore.kernel.org/lkml/1696431327-7369-1-git-send-email-quic_mojha@quicinc.com/
>> - Renamed the flag to fw_abort_load.
>>
>> Changes from v1: https://lore.kernel.org/lkml/1694773288-15755-1-git-send-email-quic_mojha@quicinc.com/
>> - 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@kernel.org>
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@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

\
 
 \ /
  Last update: 2023-10-23 14:42    [W:0.059 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site