* [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;
as well as URLs for NNTP newsgroup(s).