Re: [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback

[sunjunyong]

Hi Luis:

This issue is caused by concurrent situation like below:
when thread 1# wait firmware loading, thread 2# may write -1 to abort loading and wakeup thread 1# before it timeout.
so wait_for_completion_killable_timeout of thread 1# would return remaining time which is != 0 with fw_st->status FW_STATUS_ABORTED.
And the results would be converted into err -ENOENT in __fw_state_wait_common and transfered to fw_load_sysfs_fallback in thread 1#. 
The -ENOENT means firmware status is already at ABORTED, so fw_load_sysfs_fallback no need to get mutex to abort again.
BTW,the double abort issue would not cause kernel panic but slow down it sometimes.
-----------------------------
thread 1#,wait for loading
fw_load_sysfs_fallback
 ->fw_sysfs_wait_timeout
    ->__fw_state_wait_common
       ->wait_for_completion_killable_timeout

in __fw_state_wait_common,
...
93	ret = wait_for_completion_killable_timeout(&fw_st->completion, timeout);
94	if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
95		return -ENOENT;
96	if (!ret)
97		return -ETIMEDOUT;
98
99	return ret < 0 ? ret : 0;
-----------------------------
thread 2#, write -1 to abort loading
firmware_loading_store
 ->fw_load_abort
   ->__fw_load_abort
     ->fw_state_aborted
       ->__fw_state_set
         ->complete_all 

in __fw_state_set,
...
111         if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
112                 complete_all(&fw_st->completion);
...
-----------------------------
On Fri, Feb 28, 2020 at 01:07:35PM +0000, Luis Chamberlain wrote:
> On Fri, Feb 28, 2020 at 03:56:33PM +0800, Junyong Sun wrote:
> > fw_sysfs_wait_timeout may return err with -ENOENT
> > at fw_load_sysfs_fallback and firmware is already
> > in abort status, no need to abort again, so skip it.
> 
> What exactly is caused by this issue though? Are you seeing
> a kernel panic, some extra messages in the kernel log? This
> informationw ould be useful for the kernel commit log.
> 
> > Signed-off-by: Junyong Sun <sunjunyong@xxxxxxxxxx>
> > ---
> >  drivers/base/firmware_loader/fallback.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> > index 8704e1b..1e9c96e 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -525,7 +525,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
> >  	}
> >  
> >  	retval = fw_sysfs_wait_timeout(fw_priv, timeout);
> > -	if (retval < 0) {
> > +	if (retval < 0 && retval != -ENOENT) {
> >  		mutex_lock(&fw_lock);
> >  		fw_load_abort(fw_sysfs);
> >  		mutex_unlock(&fw_lock);
> > -- 
> > 2.7.4
> >