Messages in this thread | | | Date | Mon, 2 Mar 2020 18:40:57 +0000 | From | Luis Chamberlain <> | Subject | Re: [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback |
| |
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@xiaomi.com> > > > --- > > > 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 > > >
| |