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

[Luis Chamberlain]

On Mon, Mar 02, 2020 at 05:23:01PM +0800, sunjunyong wrote:
> 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.

OK so just clarify in your patch's commit log that without your change
you'd just abort twice, it would not create an issue, and the change is
just a minor optimization.

Can you re-submit with that change?

  Luis

> -----------------------------
> 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
> > >