[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f4255d1-51e9-8888-c32d-723a6a7afb5d@quicinc.com>
Date: Tue, 19 Sep 2023 13:27:05 +0530
From: Mukesh Ojha <quic_mojha@...cinc.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <mcgrof@...nel.org>, <russell.h.weight@...el.com>,
<rafael@...nel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] firmware_loader: Add reboot_in_progress for user helper
path
On 9/17/2023 3:42 PM, Greg KH wrote:
> On Fri, Sep 15, 2023 at 03:51:28PM +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 be expected behaviour everyone like to see, instead
>> we should abort every request which came after once firmware
>> marks reboot 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@...cinc.com>
>> ---
>> drivers/base/firmware_loader/fallback.c | 2 +-
>> drivers/base/firmware_loader/firmware.h | 1 +
>> drivers/base/firmware_loader/main.c | 2 ++
>> 3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
>> index bf68e3947814..a5546aeea91f 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -86,7 +86,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 (reboot_in_progress || fw_state_is_aborted(fw_priv)) {
>> mutex_unlock(&fw_lock);
>> retval = -EINTR;
>> goto out;
>
> What prevents reboot_in_progress to change right after you check it
> here?
e.g, reboot_in_progress was false, it gets added to the pending list
under fw_lock
list_add(&fw_priv->pending_list, &pending_fw_head);
reboot_in_progress = true, when all the outstanding fw request
are aborted during from reboot thread from below path. However,
I realize my mistake, reboot_in_progress should be modified
under fw_lock, will fix in v2.
>> fw_shutdown_notify
>> kill_pending_fw_fallback_reqs
So, idea is to revoke any fw request once firmware knows about ongoing
reboot and not delay the reboot process further for next default
60s .
>
> And what kernel driver is trying to call the reboot notifier that gets
> mixed up in this? Why not fix that driver to not need the reboot
> notifier at all (hint, I really doubt it needs it...)
'drivers/base/firmware_loader/main.c' has reboot notifier which aborts
the ongoing firmware requests and but can race with other parallel
ongoing request which are not yet added to pending list.
fw_load_sysfs_fallback-> stuck waiting for fw_lock which was held by
kill_pending_fw_fallback_reqs=>
mutex_lock(&fw_lock);
__fw_load_abort
>
>> --- a/drivers/base/firmware_loader/main.c
>> +++ b/drivers/base/firmware_loader/main.c
>> @@ -93,6 +93,7 @@ static inline struct fw_priv *to_fw_priv(struct kref *ref)
>> DEFINE_MUTEX(fw_lock);
>>
>> struct firmware_cache fw_cache;
>> +bool reboot_in_progress;
>
> Bad global name for a variable in the firmware_loader core.
bool abort_fw_load_req ??
-Mukesh
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists