* [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback @ 2020-02-28 7:56 Junyong Sun 2020-02-28 13:07 ` Luis Chamberlain 0 siblings, 1 reply; 4+ messages in thread From: Junyong Sun @ 2020-02-28 7:56 UTC (permalink / raw) To: mcgrof, gregkh, rafael, sunjunyong; +Cc: linux-kernel 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. 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback 2020-02-28 7:56 [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback Junyong Sun @ 2020-02-28 13:07 ` Luis Chamberlain 2020-03-02 9:23 ` sunjunyong 0 siblings, 1 reply; 4+ messages in thread From: Luis Chamberlain @ 2020-02-28 13:07 UTC (permalink / raw) To: Junyong Sun; +Cc: gregkh, rafael, sunjunyong, linux-kernel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback 2020-02-28 13:07 ` Luis Chamberlain @ 2020-03-02 9:23 ` sunjunyong 2020-03-02 18:40 ` Luis Chamberlain 0 siblings, 1 reply; 4+ messages in thread From: sunjunyong @ 2020-03-02 9:23 UTC (permalink / raw) To: Luis Chamberlain; +Cc: gregkh, rafael, sunjunyong, linux-kernel 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@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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback 2020-03-02 9:23 ` sunjunyong @ 2020-03-02 18:40 ` Luis Chamberlain 0 siblings, 0 replies; 4+ messages in thread From: Luis Chamberlain @ 2020-03-02 18:40 UTC (permalink / raw) To: sunjunyong; +Cc: gregkh, rafael, sunjunyong, linux-kernel 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-02 18:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-28 7:56 [PATCH] firmware: fix a double abort case with fw_load_sysfs_fallback Junyong Sun 2020-02-28 13:07 ` Luis Chamberlain 2020-03-02 9:23 ` sunjunyong 2020-03-02 18:40 ` Luis Chamberlain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox