lkml.org 
[lkml]   [2023]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] firmware_loader: Add reboot_in_progress for user helper path
From


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@quicinc.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

\
 
 \ /
  Last update: 2023-09-19 10:57    [W:0.043 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site