Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1648062 > unrolled thread
| Started by | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| First post | 2017-05-23 15:40 +0200 |
| Last post | 2017-05-27 03:50 +0200 |
| Articles | 20 on this page of 29 — 9 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] firmware: request_firmware() should propagate -ERESTARTSYS Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-23 15:40 +0200
Re: [PATCH] firmware: request_firmware() should propagate -ERESTARTSYS Martin Fuzzey <mfuzzey@parkeon.com> - 2017-05-23 16:40 +0200
Re: [PATCH] firmware: request_firmware() should propagate -ERESTARTSYS "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-23 22:00 +0200
Re: [PATCH] firmware: request_firmware() should propagate -ERESTARTSYS "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 23:00 +0200
[PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 23:50 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Andy Lutomirski <luto@kernel.org> - 2017-05-25 00:10 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-25 00:40 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Andy Lutomirski <luto@kernel.org> - 2017-05-25 06:20 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Fuzzey, Martin" <mfuzzey@parkeon.com> - 2017-05-25 10:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback ebiederm@xmission.com (Eric W. Biederman) - 2017-05-26 13:20 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 21:50 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-05-27 03:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-05-27 03:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-05 22:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Martin Fuzzey <mfuzzey@parkeon.com> - 2017-06-06 11:10 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-06 18:40 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-06 20:00 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Alan Cox <alan@linux.intel.com> - 2017-06-06 17:00 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-06 18:50 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-06 20:00 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Theodore Ts'o <tytso@mit.edu> - 2017-06-07 00:20 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-07 02:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Andy Lutomirski <luto@kernel.org> - 2017-06-07 07:00 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-07 08:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Alan Cox <alan@linux.intel.com> - 2017-06-07 14:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-26 21:50 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-27 03:30 +0200
Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Fuzzey, Martin" <mfuzzey@parkeon.com> - 2017-05-27 03:50 +0200
Page 1 of 2 [1] 2 Next page →
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-05-23 15:40 +0200 |
| Subject | Re: [PATCH] firmware: request_firmware() should propagate -ERESTARTSYS |
| Message-ID | <tKicz-3OV-43@gated-at.bofh.it> |
On Tue, May 23, 2017 at 03:16:07PM +0200, Martin Fuzzey wrote: > When -ERESTARTSYS is returned by wait_* due to a signal this should > be returned from request_firmware() so that the syscall may be > restarted if necessary. > > Without this it is not possible to distinguish the case of > request_firmware() failing due to a timeout with that due to a signal. > > In my particular case request_firmware() was being called from > a sysfs .store() callback and the writing process was the > Android init process which received a SIGCHLD causing > request_firmware() to fail. > > Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com> > --- > drivers/base/firmware_class.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Nice find, should this go to the stable kernels as well? thanks, greg k-h
[toc] | [next] | [standalone]
| From | Martin Fuzzey <mfuzzey@parkeon.com> |
|---|---|
| Date | 2017-05-23 16:40 +0200 |
| Message-ID | <tKj8C-4rO-11@gated-at.bofh.it> |
| In reply to | #1648062 |
On 23/05/17 15:31, Greg Kroah-Hartman wrote: > On Tue, May 23, 2017 at 03:16:07PM +0200, Martin Fuzzey wrote: >> When -ERESTARTSYS is returned by wait_* due to a signal this should >> be returned from request_firmware() so that the syscall may be >> restarted if necessary. >> >> >> Nice find, should this go to the stable kernels as well? Yes I think it should. I have already applied a similar patch to my 4.4 tree. The exact same patch won't apply since the code has changed a bit since. So I was planning on sending for -stable-4.4 once it's in mainline. Regards, Martin
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-23 22:00 +0200 |
| Message-ID | <tKo8i-7OL-11@gated-at.bofh.it> |
| In reply to | #1648142 |
On Tue, May 23, 2017 at 04:32:49PM +0200, Martin Fuzzey wrote:
> On 23/05/17 15:31, Greg Kroah-Hartman wrote:
> > On Tue, May 23, 2017 at 03:16:07PM +0200, Martin Fuzzey wrote:
> > > When -ERESTARTSYS is returned by wait_* due to a signal this should
> > > be returned from request_firmware() so that the syscall may be
> > > restarted if necessary.
> > >
> > >
> > > Nice find, should this go to the stable kernels as well?
>
> Yes I think it should.
Thanks for the patch !
Just a bit of more nose diving validating this.
We actually used to send -ENOMEM for a long time always, and now you are
special-casing to allow -ERESTARTSYS -- we we must ask ourselves -- why
not other errors ?
Let us consider what is upstream only please and focus on stable later.
We use:
static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
{
long ret;
ret = swait_event_interruptible_timeout(fw_st->wq,
__fw_state_is_done(READ_ONCE(fw_st->status)),
timeout);
if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
return -ENOENT;
if (!ret)
return -ETIMEDOUT;
return ret < 0 ? ret : 0;
}
What can swait_event_interruptible_timeout() return ? It can return the value
of a timeout or whatever __swait_event_interruptible_timeout() returns.
__swait_event_interruptible_timeout() in turn uses ___swait_event() as
follows:
#define __swait_event_interruptible_timeout(wq, condition, timeout) \
___swait_event(wq, ___wait_cond_timeout(condition), \
TASK_INTERRUPTIBLE, timeout, \
__ret = schedule_timeout(__ret))
So this ultimately use ___swait_event():
/* as per ___wait_event() but for swait, therefore "exclusive == 0" */
#define ___swait_event(wq, condition, state, ret, cmd) \
({ \
struct swait_queue __wait; \
long __ret = ret; \
\
INIT_LIST_HEAD(&__wait.task_list); \
for (;;) { \
long __int = prepare_to_swait_event(&wq, &__wait, state);\
\
if (condition) \
break; \
\
if (___wait_is_interruptible(state) && __int) { \
__ret = __int; \
break; \
} \
\
cmd; \
} \
finish_swait(&wq, &__wait); \
__ret; \
})
And prepare_to_swait_event() can return -ERESTARTSYS on signal_pending_state()
otherwise it returns 0 ! So indeed -ERESTARTSYS is possible.
But what about other errors ? Considering the above it would seem then we
actually can only get -ERESTARTSYS or whatever schedule_timeout() returns.
schedule_timeout() is documented indicating it returns 0 when the timer has
expired otherwise the remaining time in jiffies will be returned. It further
clarifies that the return value is guaranteed to be be non-negative. And
___wait_is_interruptible() is:
#define ___wait_is_interruptible(state) \
(!__builtin_constant_p(state) || \
state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
So this piece of code:
\
if (___wait_is_interruptible(state) && __int) { \
__ret = __int; \
break; \
} \
Since ___wait_is_interruptible() is always true for us since we use
__swait_event_interruptible_timeout() the -ERESTARTSYS will always be sent if a
signal was sent.
In all this light then the patch is correct for upstream however let's consider
stable now. At first glance this seems like a fix for an old patch,
respectively commit 0542ad88fbdd81bb ("firmware loader: Fix
_request_firmware_load() return val for fw load abort" by Shuah Khan which was
merged since v3.17, *but* back then just used wait_for_completion() and ignored
any signals here, they were just not part of our semantics. But its important
to note then we always returned -ENOMEM before that patch and then at least
returned -EAGAIN in other cases. The next thing to note is commit
5d47ec02c37ea632398c ("firmware: Correct handling of fw_state_wait() return
value") by Bjorn Andersson. This took place *after* the swait changes. Bjorn
fixed an issue but also forgot to address the special case of -ERESTARTSYS,
given right below fw_state_wait_timeout() on _request_firmware_load() the
return value would be lost. It would seem Bjorn assumed the return value would
be propagated but did not notice the error special casing below which would
loose it.
So before and after Bjorn's changes we were still *trying* to propagate the
-ERESTARTSYS error but it was still lost.
The -ERESTARTSYS from signals was still something we were capturing even prior
to the swait changes, see the kernel commit prior to the swait changes (0430cafcc4fb
"firmware: drop bit ops in favor of simple state machine"), if you git blame there,
will find a series of commits with the -ERESTARTSYS handled... I can trace back
to commit 68ff2a00dbf ("firmware_loader: handle timeout via
wait_for_completion_interruptible_timeout()") as having the -ERESTARTSYS check but
it had lost that error on _request_firmware_load() due to the :
if (retval == -ERESTARTSYS || !retval) {
mutex_lock(&fw_lock);
fw_load_abort(fw_priv);
mutex_unlock(&fw_lock);
}
if (is_fw_load_aborted(buf))
retval = -EAGAIN;
else if (!buf->data)
retval = -ENOMEM;
As noted earlier the above piece of code lost the error because of
0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val for
fw load abort" which was merged since v3.17, but back then it *did not*
propagate the error. So it would be incorrect to say that your patch fixes
commit 0542ad88fbdd81bb by Shuah Khan... We'd have to ask ourselves when
such an error actually became relevant.
It would seem the -ERESTARTSYS signal took effect first via commit 0cb64249ca500
("firmware_loader: abort request if wait_for_completion is interrupted") added
upstream via v4.0 and since it was introduced the error code was lost given the
-EAGAIN overwrite added *earlier* by Shuah Khan when such error codes were not
even relevant. So prior to v4.0 were we not even aborting due to signals.
As such this as far as I can tell this is a fix for a fix for this commit.
So we should use:
Fixes: 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion is interrupted")
Cc: stable@vger.kernel.org # 4.0
Also a more reflective subject and commit log would be appreciated, you can add
my Acked-by given I have also now tested it with all the test drivers and
test scripts:
==========================================================================
firmware: fix sending -ERESTARTSYS due to signal on fallback
Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
is interrupted") added via 4.0 added support to abort the fallback mechanism
when a signal was detected and wait_for_completion_interruptible() returned
-ERESTARTSYS. Although the abort was effective we were unfortunately never
really propagating this error though and as such userspace could not know
why the abort happened.
The error code was always being lost to an even older change, commit
0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val
for fw load abort") by Shuah Khan which was merged since v3.17. Back then
though we never were capturing these signals or bailing on a signal. After
this change though only only -EAGAIN was being relayed back to userspace
on non-memory errors including signals trying to interrupt our fallback
process.
It only makes sense to fix capturing -ERESTARTSYS since we were capturing
the error but when it was actually effective, since commit 0cb64249ca500
("firmware_loader: abort request if wait_for_completion is interrupted").
Only distributions relying on the fallback mechanism are impacted. An
example issue is on Android, when request_firmware() is called through
the firmware fallback mechanism -- syfs write call, sysfs .store(), and
Android sends a SIGCHLD to fail the write call -- in such cases the fact
that we failed due to a signal is lost.
Fix this and ensure we propagate -ERESTARTSYS so that handlers can whether
or not to restart write calls.
Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Cc: stable@vger.kernel.org # 4.0
Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>
==========================================================================
> I have already applied a similar patch to my 4.4 tree.
You might also be intersted in commit 2e700f8d85975 ("firmware: fix usermode
helper fallback loading".
> The exact same patch won't apply since the code has changed a bit since.
>
> So I was planning on sending for -stable-4.4 once it's in mainline.
Appreciated, after this is merged of course.
Luis
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-24 23:00 +0200 |
| Message-ID | <tKLxU-7aj-11@gated-at.bofh.it> |
| In reply to | #1648378 |
On Tue, May 23, 2017 at 09:55:33PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 23, 2017 at 04:32:49PM +0200, Martin Fuzzey wrote:
> > On 23/05/17 15:31, Greg Kroah-Hartman wrote:
> > > On Tue, May 23, 2017 at 03:16:07PM +0200, Martin Fuzzey wrote:
> > > > When -ERESTARTSYS is returned by wait_* due to a signal this should
> > > > be returned from request_firmware() so that the syscall may be
> > > > restarted if necessary.
> > > >
> > > >
> > > > Nice find, should this go to the stable kernels as well?
> >
> > Yes I think it should.
>
> Thanks for the patch !
>
> Just a bit of more nose diving validating this.
>
> We actually used to send -ENOMEM for a long time always, and now you are
> special-casing to allow -ERESTARTSYS -- we we must ask ourselves -- why
> not other errors ?
>
> Let us consider what is upstream only please and focus on stable later.
>
> We use:
>
> static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
> {
> long ret;
>
> ret = swait_event_interruptible_timeout(fw_st->wq,
> __fw_state_is_done(READ_ONCE(fw_st->status)),
> timeout);
> if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> return -ENOENT;
> if (!ret)
> return -ETIMEDOUT;
>
> return ret < 0 ? ret : 0;
> }
>
> What can swait_event_interruptible_timeout() return ? It can return the value
> of a timeout or whatever __swait_event_interruptible_timeout() returns.
> __swait_event_interruptible_timeout() in turn uses ___swait_event() as
> follows:
>
> #define __swait_event_interruptible_timeout(wq, condition, timeout) \
> ___swait_event(wq, ___wait_cond_timeout(condition), \
> TASK_INTERRUPTIBLE, timeout, \
> __ret = schedule_timeout(__ret))
>
> So this ultimately use ___swait_event():
>
> /* as per ___wait_event() but for swait, therefore "exclusive == 0" */
> #define ___swait_event(wq, condition, state, ret, cmd) \
> ({ \
> struct swait_queue __wait; \
> long __ret = ret; \
> \
> INIT_LIST_HEAD(&__wait.task_list); \
> for (;;) { \
> long __int = prepare_to_swait_event(&wq, &__wait, state);\
> \
> if (condition) \
> break; \
> \
> if (___wait_is_interruptible(state) && __int) { \
> __ret = __int; \
> break; \
> } \
> \
> cmd; \
> } \
> finish_swait(&wq, &__wait); \
> __ret; \
> })
>
> And prepare_to_swait_event() can return -ERESTARTSYS on signal_pending_state()
> otherwise it returns 0 ! So indeed -ERESTARTSYS is possible.
>
> But what about other errors ? Considering the above it would seem then we
> actually can only get -ERESTARTSYS or whatever schedule_timeout() returns.
> schedule_timeout() is documented indicating it returns 0 when the timer has
> expired otherwise the remaining time in jiffies will be returned. It further
> clarifies that the return value is guaranteed to be be non-negative. And
> ___wait_is_interruptible() is:
>
> #define ___wait_is_interruptible(state) \
> (!__builtin_constant_p(state) || \
> state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
>
> So this piece of code:
> \
> if (___wait_is_interruptible(state) && __int) { \
> __ret = __int; \
> break; \
> } \
>
> Since ___wait_is_interruptible() is always true for us since we use
> __swait_event_interruptible_timeout() the -ERESTARTSYS will always be sent if a
> signal was sent.
>
> In all this light then the patch is correct for upstream however let's consider
> stable now. At first glance this seems like a fix for an old patch,
> respectively commit 0542ad88fbdd81bb ("firmware loader: Fix
> _request_firmware_load() return val for fw load abort" by Shuah Khan which was
> merged since v3.17, *but* back then just used wait_for_completion() and ignored
> any signals here, they were just not part of our semantics. But its important
> to note then we always returned -ENOMEM before that patch and then at least
> returned -EAGAIN in other cases. The next thing to note is commit
> 5d47ec02c37ea632398c ("firmware: Correct handling of fw_state_wait() return
> value") by Bjorn Andersson. This took place *after* the swait changes. Bjorn
> fixed an issue but also forgot to address the special case of -ERESTARTSYS,
> given right below fw_state_wait_timeout() on _request_firmware_load() the
> return value would be lost. It would seem Bjorn assumed the return value would
> be propagated but did not notice the error special casing below which would
> loose it.
>
> So before and after Bjorn's changes we were still *trying* to propagate the
> -ERESTARTSYS error but it was still lost.
>
> The -ERESTARTSYS from signals was still something we were capturing even prior
> to the swait changes, see the kernel commit prior to the swait changes (0430cafcc4fb
> "firmware: drop bit ops in favor of simple state machine"), if you git blame there,
> will find a series of commits with the -ERESTARTSYS handled... I can trace back
> to commit 68ff2a00dbf ("firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()") as having the -ERESTARTSYS check but
> it had lost that error on _request_firmware_load() due to the :
>
> if (retval == -ERESTARTSYS || !retval) {
> mutex_lock(&fw_lock);
> fw_load_abort(fw_priv);
> mutex_unlock(&fw_lock);
> }
>
> if (is_fw_load_aborted(buf))
> retval = -EAGAIN;
> else if (!buf->data)
> retval = -ENOMEM;
>
> As noted earlier the above piece of code lost the error because of
> 0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val for
> fw load abort" which was merged since v3.17, but back then it *did not*
> propagate the error. So it would be incorrect to say that your patch fixes
> commit 0542ad88fbdd81bb by Shuah Khan... We'd have to ask ourselves when
> such an error actually became relevant.
>
> It would seem the -ERESTARTSYS signal took effect first via commit 0cb64249ca500
> ("firmware_loader: abort request if wait_for_completion is interrupted") added
> upstream via v4.0 and since it was introduced the error code was lost given the
> -EAGAIN overwrite added *earlier* by Shuah Khan when such error codes were not
> even relevant. So prior to v4.0 were we not even aborting due to signals.
>
> As such this as far as I can tell this is a fix for a fix for this commit.
>
> So we should use:
>
> Fixes: 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion is interrupted")
> Cc: stable@vger.kernel.org # 4.0
>
> Also a more reflective subject and commit log would be appreciated, you can add
> my Acked-by given I have also now tested it with all the test drivers and
> test scripts:
>
> ==========================================================================
> firmware: fix sending -ERESTARTSYS due to signal on fallback
>
> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
> is interrupted") added via 4.0 added support to abort the fallback mechanism
> when a signal was detected and wait_for_completion_interruptible() returned
> -ERESTARTSYS. Although the abort was effective we were unfortunately never
> really propagating this error though and as such userspace could not know
> why the abort happened.
>
> The error code was always being lost to an even older change, commit
> 0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val
> for fw load abort") by Shuah Khan which was merged since v3.17. Back then
> though we never were capturing these signals or bailing on a signal. After
> this change though only only -EAGAIN was being relayed back to userspace
> on non-memory errors including signals trying to interrupt our fallback
> process.
>
> It only makes sense to fix capturing -ERESTARTSYS since we were capturing
> the error but when it was actually effective, since commit 0cb64249ca500
> ("firmware_loader: abort request if wait_for_completion is interrupted").
>
> Only distributions relying on the fallback mechanism are impacted. An
> example issue is on Android, when request_firmware() is called through
> the firmware fallback mechanism -- syfs write call, sysfs .store(), and
> Android sends a SIGCHLD to fail the write call -- in such cases the fact
> that we failed due to a signal is lost.
>
> Fix this and ensure we propagate -ERESTARTSYS so that handlers can whether
> or not to restart write calls.
>
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> Cc: stable@vger.kernel.org # 4.0
> Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ==========================================================================
>
> > I have already applied a similar patch to my 4.4 tree.
>
> You might also be intersted in commit 2e700f8d85975 ("firmware: fix usermode
> helper fallback loading".
>
> > The exact same patch won't apply since the code has changed a bit since.
> >
> > So I was planning on sending for -stable-4.4 once it's in mainline.
>
> Appreciated, after this is merged of course.
I'll just send this with the commit log change myself.
Luis
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-24 23:50 +0200 |
| Subject | [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tKMki-7HS-25@gated-at.bofh.it> |
| In reply to | #1649949 |
From: Martin Fuzzey <mfuzzey@parkeon.com>
Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
is interrupted") added via 4.0 added support to abort the fallback mechanism
when a signal was detected and wait_for_completion_interruptible() returned
-ERESTARTSYS. Although the abort was effective we were unfortunately never
really propagating this error though and as such userspace could not know
why the abort happened.
The error code was always being lost to an even older change, commit
0542ad88fbdd81bb ("firmware loader: Fix _request_firmware_load() return val
for fw load abort") by Shuah Khan which was merged since v3.17. Back then
though we never were capturing these signals or bailing on a signal. After
this change though only only -EAGAIN was being relayed back to userspace
on non-memory errors including signals trying to interrupt our fallback
process.
It only makes sense to fix capturing -ERESTARTSYS since we were capturing
the error but when it was actually effective, since commit 0cb64249ca500
("firmware_loader: abort request if wait_for_completion is interrupted").
Only distributions relying on the fallback mechanism are impacted. An
example issue is on Android, when request_firmware() is called through
the firmware fallback mechanism -- syfs write call, sysfs .store(), and
Android sends a SIGCHLD to fail the write call -- in such cases the fact
that we failed due to a signal is lost.
Fix this and ensure we propagate -ERESTARTSYS so that handlers can
whether or not to restart write calls.
Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Cc: stable <stable@vger.kernel.org> # 4.0
Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>
[mcgrof: gave the commit log some serious love]
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
For those following drivers/base/firmware_class.c development -- I've pushed
out two new branches based on linux-next tag next-20170524, one is just the
driver-data API [0], and the other one just has this patch applied on top
of the driver-data API [1]. You'll want to use driver-data-stable if you are
working on the cache or the fallback mechanism since this fix does provide
a fix for the cache / fallback mechanism.
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170524-driver-data
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170524-driver-data-stable
Luis
drivers/base/firmware_class.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7af430a2d656..77c0e0792c30 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1309,9 +1309,10 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
mutex_unlock(&fw_lock);
}
- if (fw_state_is_aborted(&buf->fw_st))
- retval = -EAGAIN;
- else if (buf->is_paged_buf && !buf->data)
+ if (fw_state_is_aborted(&buf->fw_st)) {
+ if (retval != -ERESTARTSYS)
+ retval = -EAGAIN;
+ } else if (buf->is_paged_buf && !buf->data)
retval = -ENOMEM;
device_del(f_dev);
--
2.10.2
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-05-25 00:10 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tKMDD-84u-1@gated-at.bofh.it> |
| In reply to | #1649968 |
On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> From: Martin Fuzzey <mfuzzey@parkeon.com>
>
> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
> is interrupted") added via 4.0 added support to abort the fallback mechanism
> when a signal was detected and wait_for_completion_interruptible() returned
> -ERESTARTSYS. Although the abort was effective we were unfortunately never
> really propagating this error though and as such userspace could not know
> why the abort happened.
Can you give a simple example of what's going on and why it matters?
ERESTARTSYS and friends are highly magical, and I'm not convinced that
allowing _request_firmware_load to return -ERESTARTSYS is actually a
good idea. What if there are system calls that can't handle this
style of restart that start being restarted as a result?
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-25 00:40 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tKN6F-8dQ-1@gated-at.bofh.it> |
| In reply to | #1649987 |
On Wed, May 24, 2017 at 3:00 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> From: Martin Fuzzey <mfuzzey@parkeon.com>
>>
>> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
>> is interrupted") added via 4.0 added support to abort the fallback mechanism
>> when a signal was detected and wait_for_completion_interruptible() returned
>> -ERESTARTSYS. Although the abort was effective we were unfortunately never
>> really propagating this error though and as such userspace could not know
>> why the abort happened.
>
> Can you give a simple example of what's going on and why it matters?
>
> ERESTARTSYS and friends are highly magical, and I'm not convinced that
> allowing _request_firmware_load to return -ERESTARTSYS is actually a
> good idea. What if there are system calls that can't handle this
> style of restart that start being restarted as a result?
This seems to be a linux-api question, so Cc'ing them and Michael.
For those not familiar it is worth explaining first the user interface.
This describes the fallback mechanism of the Linux firmware API if
direct filesystem lookup fails. Although most distros disable this
stuff today some distros (Android) seem to be always relying on it
today. Other than Android, since most distros have the forced fallback
mechanism off but *enable* requests to require it, only 2 upstream
drivers do this, so on other distros we only have 2 drivers which
typically require the the fallback mechanism.
The exposed user interface here is we enable a knob through sysfs to
enable userspace to write a file as a fallback mechanism then, and we
expect to get woken up today when userspace finds the needed file and
then writes the file to the sysfs knob. We wait for userspace using
swait_event_interruptible_timeout(). While we wait we can get a
-ERESTARTSYS since swait_event_interruptible_timeout() uses
__swait_event_interruptible_timeout() under the hood and this in turn
___swait_event() which can prepare_to_swait_event() which can return
-ERESTARTSYS on signal_pending_state().
The issue discovered was that Android could issue SIGCHLD and the
waiter gets a signal but the reason for the exact reason for the
failure is not propagated. The proposed patch propagates -ERESTARTSYS
when that is returned on signal_pending_state() as we wait.
So linux-api folks please speak up.
Luis
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-05-25 06:20 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tKSpH-3gE-1@gated-at.bofh.it> |
| In reply to | #1650006 |
On Wed, May 24, 2017 at 3:38 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, May 24, 2017 at 3:00 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> From: Martin Fuzzey <mfuzzey@parkeon.com>
>>>
>>> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
>>> is interrupted") added via 4.0 added support to abort the fallback mechanism
>>> when a signal was detected and wait_for_completion_interruptible() returned
>>> -ERESTARTSYS. Although the abort was effective we were unfortunately never
>>> really propagating this error though and as such userspace could not know
>>> why the abort happened.
>>
>> Can you give a simple example of what's going on and why it matters?
>>
>> ERESTARTSYS and friends are highly magical, and I'm not convinced that
>> allowing _request_firmware_load to return -ERESTARTSYS is actually a
>> good idea. What if there are system calls that can't handle this
>> style of restart that start being restarted as a result?
>
> This seems to be a linux-api question, so Cc'ing them and Michael.
>
> For those not familiar it is worth explaining first the user interface.
>
> This describes the fallback mechanism of the Linux firmware API if
> direct filesystem lookup fails.
...
> While we wait we can get a
> -ERESTARTSYS since swait_event_interruptible_timeout() uses
> __swait_event_interruptible_timeout() under the hood and this in turn
> ___swait_event() which can prepare_to_swait_event() which can return
> -ERESTARTSYS on signal_pending_state().
This is too much kernel detail and too little ABI detail.
User code does some syscall. Kernel requests firmware and that
request gets interrupted. What syscall is this? read(2)? open(2)?
Something else?
mutex_lock_interruptible() returns -EINTR if interrupted. It seems
odd to be that requesting firmware would be different.
>
> The issue discovered was that Android could issue SIGCHLD and the
> waiter gets a signal but the reason for the exact reason for the
> failure is not propagated. The proposed patch propagates -ERESTARTSYS
> when that is returned on signal_pending_state() as we wait.
Maybe SIGCHLD shouldn't interrupt firmware loading?
[toc] | [prev] | [next] | [standalone]
| From | "Fuzzey, Martin" <mfuzzey@parkeon.com> |
|---|---|
| Date | 2017-05-25 10:30 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tKWjF-5Mt-27@gated-at.bofh.it> |
| In reply to | #1650163 |
On 25 May 2017 at 06:13, Andy Lutomirski <luto@kernel.org> wrote: >>> >>> Can you give a simple example of what's going on and why it matters? >>> Here is the use case in which I ran into this problem. I have a driver which does request_firmware() when a write() is done to a sysfs file. The write() was being done by an android init script (with the init interpreter "write" command). init, of course, forks lots of processes and some of the children die. So the scenario was the following: 1) Android init calls write() on the sysfs file 2) The sysfs .store() callback registered by a driver is called 3) The driver calls request_firmware() 4) request_firmware() sends the firmware load request to userspace and calls wait_for_completion_interruptible() 5) A child dies and raises SIGCHLD 6) wait_for_completion_interruptible() returns -ERESTARTSYS due to the signal 7) request_firmware() [before this patch] translated that to -EAGAIN 8) The driver (in my case) ignored this [because the firmware was not critical - it was for checking if a microcontroler was up to date] (but it could have returned it to userspace, same problem) The point being that, due to a signal (SIGCHLD) which has nothing to do with the firmware loading process, the firmware load was not done. Also EAGAIN is the same error used if the load request times out so it was impossible to distinguish the two cases. ERESTARTSYS is an internal error and is not returned to userspace. Instead it is handled by the linux syscall machinery which, after processing the signal either restarts (transpently to userspace) the syscall or returns EINTR to userspace (depending if the signal handler users SA_RESTART - see man 7 signal) With this patch here is what happens: 1) Android init calls write() on the sysfs file 2) The sysfs .store() callback registered by a driver is called 3) The driver calls request_firmware() 4) request_firmware() sends the firmware load request to userspace and calls wait_for_completion_interruptible() 5) A child dies and raises SIGCHLD 6) wait_for_completion_interruptible() returns -ERESTARTSYS due to the signal 7) request_firmware() [with this patch] returns -ERESTARTSYS 8) The driver returns -ERSTARTSYS from its sysfs .store method 9) The system call machinery invokes the signal handler 10) The signal handler does its stuff 11) Because SA_RESTART was set the system call is restarted (calling the sysfs .store) and we try it all again from step 2 Note that, on the the userspace side write() is only called once (the restart is transparent to userspace which is oblivious to all this) The kernel side write() (which calls .store() is called multiple times (so that code does need to know about this) >>> ERESTARTSYS and friends are highly magical, and I'm not convinced that >>> allowing _request_firmware_load to return -ERESTARTSYS is actually a >>> good idea. What if there are system calls that can't handle this >>> style of restart that start being restarted as a result? >> If the caller is unable to restart (for example if the driver's .store() callback had already done lots of stuff that couldn't be undone) it is free to translate -ERSTARTSYS to -EINTR before returning. But request_frimware() can't know about that. >>> Maybe SIGCHLD shouldn't interrupt firmware loading? I don't think there's a way of doing that without disabling all signals (ie using the non interruptible wait variants). It used to be that way (which is why I only ran into this after updating from an ancient 3.16 kernel to a slightly less ancient 4.4) But there are valid reasons for wanting to be able to interrupt firmware loading (like being able to kill the userspace helper) Hope this explains it better, Regads, Martin
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-05-26 13:20 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tLlrH-5bT-9@gated-at.bofh.it> |
| In reply to | #1650299 |
"Fuzzey, Martin" <mfuzzey@parkeon.com> writes: > On 25 May 2017 at 06:13, Andy Lutomirski <luto@kernel.org> wrote: >>>> >>>> Can you give a simple example of what's going on and why it matters? >>>> > > > Here is the use case in which I ran into this problem. > > I have a driver which does request_firmware() when a write() is done > to a sysfs file. > > The write() was being done by an android init script (with the init > interpreter "write" command). > init, of course, forks lots of processes and some of the children die. > > So the scenario was the following: > > 1) Android init calls write() on the sysfs file > 2) The sysfs .store() callback registered by a driver is called > 3) The driver calls request_firmware() > 4) request_firmware() sends the firmware load request to userspace and > calls wait_for_completion_interruptible() > 5) A child dies and raises SIGCHLD > 6) wait_for_completion_interruptible() returns -ERESTARTSYS due to the signal > 7) request_firmware() [before this patch] translated that to -EAGAIN > 8) The driver (in my case) ignored this [because the firmware was not > critical - it was for checking if a microcontroler was up to date] > (but it could have returned it to userspace, same problem) > > The point being that, due to a signal (SIGCHLD) which has nothing to > do with the firmware loading process, the firmware load was not done. > Also EAGAIN is the same error used if the load request times out so it > was impossible to distinguish the two cases. > > ERESTARTSYS is an internal error and is not returned to userspace. > Instead it is handled by the linux syscall machinery which, after > processing the signal either restarts (transpently to userspace) the > syscall or returns EINTR to userspace (depending if the signal handler > users SA_RESTART - see man 7 signal) > > > With this patch here is what happens: > > 1) Android init calls write() on the sysfs file > 2) The sysfs .store() callback registered by a driver is called > 3) The driver calls request_firmware() > 4) request_firmware() sends the firmware load request to userspace and > calls wait_for_completion_interruptible() > 5) A child dies and raises SIGCHLD > 6) wait_for_completion_interruptible() returns -ERESTARTSYS due to the signal > 7) request_firmware() [with this patch] returns -ERESTARTSYS > 8) The driver returns -ERSTARTSYS from its sysfs .store method > 9) The system call machinery invokes the signal handler > 10) The signal handler does its stuff > 11) Because SA_RESTART was set the system call is restarted (calling > the sysfs .store) and we try it all again from step 2 > > Note that, on the the userspace side write() is only called once (the > restart is transparent to userspace which is oblivious to all this) > The kernel side write() (which calls .store() is called multiple times > (so that code does need to know about this) > > >>>> ERESTARTSYS and friends are highly magical, and I'm not convinced that >>>> allowing _request_firmware_load to return -ERESTARTSYS is actually a >>>> good idea. What if there are system calls that can't handle this >>>> style of restart that start being restarted as a result? >>> > > If the caller is unable to restart (for example if the driver's > .store() callback had already done lots of stuff that couldn't be > undone) it is free to translate -ERSTARTSYS to -EINTR before > returning. > But request_frimware() can't know about that. > > >>>> Maybe SIGCHLD shouldn't interrupt firmware loading? > > I don't think there's a way of doing that without disabling all > signals (ie using the non interruptible wait variants). > It used to be that way (which is why I only ran into this after > updating from an ancient 3.16 kernel to a slightly less ancient 4.4) > But there are valid reasons for wanting to be able to interrupt > firmware loading (like being able to kill the userspace helper) Perhaps simply using a killable wait and not a fully interruptible wait would be better? It sounds like the code really is not prepared for an truly interruptible wait here. Eric
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-26 21:50 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tLtpf-1zn-3@gated-at.bofh.it> |
| In reply to | #1651313 |
On Fri, May 26, 2017 at 06:09:29AM -0500, Eric W. Biederman wrote:
> "Fuzzey, Martin" <mfuzzey@parkeon.com> writes:
> >>>> Maybe SIGCHLD shouldn't interrupt firmware loading?
> >
> > I don't think there's a way of doing that without disabling all
> > signals (ie using the non interruptible wait variants).
> > It used to be that way (which is why I only ran into this after
> > updating from an ancient 3.16 kernel to a slightly less ancient 4.4)
> > But there are valid reasons for wanting to be able to interrupt
> > firmware loading (like being able to kill the userspace helper)
>
> Perhaps simply using a killable wait and not a fully interruptible
> wait would be better?
What do you mean by a killable wait BTW?
ret = swait_event_interruptible_timeout() is being used right now.
The problem is we have:
if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
return -ENOENT;
if (!ret)
return -ETIMEDOUT;
return ret < 0 ? ret : 0;
The (!ret) return -ETIMEDOUT ensures that if there was no time left
then we know we ran out of time.
The ret < 0 ? ret makes sure we send any errors
swait_event_interruptible_timeout() sent.
But the caller of this code has:
if (fw_state_is_aborted(&buf->fw_st))
retval = -EAGAIN;
else if (buf->is_paged_buf && !buf->data)
retval = -ENOMEM;
And this retval is used. so we mask all errors with -EAGAIN.
So Martin is asking us to let us send -ERESTARTSYS back down to drivers.
These potentially could send back down to probe, and so finit_module()
could get this.
Another use case is a custom syfs knob which triggers a request_firmware(),
in such case this is a simple write(), but Anroid is configured to retry
if -ERESTARTSYS so I gather it will *retry* writing again to this file
if -ERESTARTSYS was sent and therefore triggering another firmware request.
> It sounds like the code really is not prepared for an truly
> interruptible wait here.
Can you clarify what you mean?
Luis
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-05-27 03:30 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tLyIi-4Vw-33@gated-at.bofh.it> |
| In reply to | #1651591 |
On Fri, May 26, 2017 at 12:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Fri, May 26, 2017 at 06:09:29AM -0500, Eric W. Biederman wrote: >> "Fuzzey, Martin" <mfuzzey@parkeon.com> writes: >> >>>> Maybe SIGCHLD shouldn't interrupt firmware loading? >> > >> > I don't think there's a way of doing that without disabling all >> > signals (ie using the non interruptible wait variants). >> > It used to be that way (which is why I only ran into this after >> > updating from an ancient 3.16 kernel to a slightly less ancient 4.4) >> > But there are valid reasons for wanting to be able to interrupt >> > firmware loading (like being able to kill the userspace helper) >> >> Perhaps simply using a killable wait and not a fully interruptible >> wait would be better? > > What do you mean by a killable wait BTW? https://lwn.net/Articles/288056/ I think only interrupting firmware loading with fatal signals would make a lot of sense. > > ret = swait_event_interruptible_timeout() is being used right now. It looks like we are missing swait_event_killable*(), but I do not think it would be hard to add. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-05-27 03:30 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tLyIj-4Vw-59@gated-at.bofh.it> |
| In reply to | #1651623 |
On Fri, May 26, 2017 at 02:32:31PM -0700, Luis R. Rodriguez wrote: > On Fri, May 26, 2017 at 2:26 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Fri, May 26, 2017 at 12:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > >> On Fri, May 26, 2017 at 06:09:29AM -0500, Eric W. Biederman wrote: > >>> "Fuzzey, Martin" <mfuzzey@parkeon.com> writes: > >>> >>>> Maybe SIGCHLD shouldn't interrupt firmware loading? > >>> > > >>> > I don't think there's a way of doing that without disabling all > >>> > signals (ie using the non interruptible wait variants). > >>> > It used to be that way (which is why I only ran into this after > >>> > updating from an ancient 3.16 kernel to a slightly less ancient 4.4) > >>> > But there are valid reasons for wanting to be able to interrupt > >>> > firmware loading (like being able to kill the userspace helper) > >>> > >>> Perhaps simply using a killable wait and not a fully interruptible > >>> wait would be better? > >> > >> What do you mean by a killable wait BTW? > > > > https://lwn.net/Articles/288056/ > > > > I think only interrupting firmware loading with fatal signals would > > make a lot of sense. > > > >> > >> ret = swait_event_interruptible_timeout() is being used right now. > > > > It looks like we are missing swait_event_killable*(), but I do not > > think it would be hard to add. > > What should we do for stable ? Is this a *stable* issue ? I think it is, as you have users complaining about behavior. I do not think we need to make their lives harder than needed by requiring handling signals. I do not see why we could not introduce wait_event_killable_timeout() and swait_event_killable_timeout() into -stables. Thanks. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-06-05 22:30 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tP6Nr-7zW-1@gated-at.bofh.it> |
| In reply to | #1651634 |
On Fri, May 26, 2017 at 02:55:18PM -0700, Dmitry Torokhov wrote:
> On Fri, May 26, 2017 at 02:32:31PM -0700, Luis R. Rodriguez wrote:
> > On Fri, May 26, 2017 at 2:26 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, May 26, 2017 at 12:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > >> On Fri, May 26, 2017 at 06:09:29AM -0500, Eric W. Biederman wrote:
> > >>> "Fuzzey, Martin" <mfuzzey@parkeon.com> writes:
> > >>> >>>> Maybe SIGCHLD shouldn't interrupt firmware loading?
> > >>> >
> > >>> > I don't think there's a way of doing that without disabling all
> > >>> > signals (ie using the non interruptible wait variants).
> > >>> > It used to be that way (which is why I only ran into this after
> > >>> > updating from an ancient 3.16 kernel to a slightly less ancient 4.4)
> > >>> > But there are valid reasons for wanting to be able to interrupt
> > >>> > firmware loading (like being able to kill the userspace helper)
> > >>>
> > >>> Perhaps simply using a killable wait and not a fully interruptible
> > >>> wait would be better?
> > >>
> > >> What do you mean by a killable wait BTW?
> > >
> > > https://lwn.net/Articles/288056/
Read it thanks ! As per this it states, "Kernel code which uses interruptible
sleeps must always check to see whether it woke up as a result of a signal,
and, if so, clean up whatever it was doing and return -EINTR back to user
space." -- but also on the same article it quotes Alan Cox as having noted
"Unix tradition (and thus almost all applications) believe file store writes to
be non signal interruptible. It would not be safe or practical to change that
guarantee."
For these two reasons then it would seem best we do two things actually:
1) return -EINTR instead of -EAGAIN when we detect swait_event_interruptible_timeout()
got interrupted by a signal (it returns -ERESTARTSYS)
2) Do as you note below and add wait_event_killable_timeout()
> > > I think only interrupting firmware loading with fatal signals would
> > > make a lot of sense.
> > >
> > >>
> > >> ret = swait_event_interruptible_timeout() is being used right now.
> > >
> > > It looks like we are missing swait_event_killable*(), but I do not
> > > think it would be hard to add.
> >
> > What should we do for stable ? Is this a *stable* issue ?
>
> I think it is, as you have users complaining about behavior. I do not
> think we need to make their lives harder than needed by requiring
> handling signals.
Makes sense, specially given the long tradition, it would breaking a long
tradition. Even though in this case we are dealing with sysfs files it
should be no different.
> I do not see why we could not introduce wait_event_killable_timeout()
> and swait_event_killable_timeout() into -stables.
After seeing how simple it is to do so I tend to agree. Greg, Peter,
what are your thoughts ?
Martin Fuzzey can you test this patch as an alternative to your issue ?
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b9f907eedbf7..70fc42e5e0da 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
{
long ret;
- ret = swait_event_interruptible_timeout(fw_st->wq,
+ ret = swait_event_killable_timeout(fw_st->wq,
__fw_state_is_done(READ_ONCE(fw_st->status)),
timeout);
if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
diff --git a/include/linux/swait.h b/include/linux/swait.h
index c1f9c62a8a50..9c5ca2898b2f 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -169,4 +169,29 @@ do { \
__ret; \
})
+#define __swait_event_killable(wq, condition) \
+ (void)___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
+
+#define swait_event_killable(wq, condition) \
+({ \
+ int __ret = 0; \
+ if (!(condition)) \
+ __ret = __swait_event_killable(wq, condition); \
+ __ret; \
+})
+
+#define __swait_event_killable_timeout(wq, condition, timeout) \
+ ___swait_event(wq, ___wait_cond_timeout(condition), \
+ TASK_INTERRUPTIBLE, timeout, \
+ __ret = schedule_timeout(__ret))
+
+#define swait_event_killable_timeout(wq, condition, timeout) \
+({ \
+ long __ret = timeout; \
+ if (!___wait_cond_timeout(condition)) \
+ __ret = __swait_event_killable_timeout(wq, \
+ condition, timeout); \
+ __ret; \
+})
+
#endif /* _LINUX_SWAIT_H */
Luis
[toc] | [prev] | [next] | [standalone]
| From | Martin Fuzzey <mfuzzey@parkeon.com> |
|---|---|
| Date | 2017-06-06 11:10 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPiEW-6IR-19@gated-at.bofh.it> |
| In reply to | #1658130 |
On 05/06/17 22:24, Luis R. Rodriguez wrote:
>
>
> For these two reasons then it would seem best we do two things actually:
>
> 1) return -EINTR instead of -EAGAIN when we detect swait_event_interruptible_timeout()
> got interrupted by a signal (it returns -ERESTARTSYS)
I disagree. That would force userspace to handle the signal rather than
having the kernel retry.
From Documentation/DocBook/kernel-hacking.tmpl:
After you slept you should check if a signal occurred: the
Unix/Linux way of handling signals is to temporarily exit the
system call with the <constant>-ERESTARTSYS</constant> error. The
system call entry code will switch back to user context, process
the signal handler and then your system call will be restarted
(unless the user disabled that). So you should be prepared to
process the restart, e.g. if you're in the middle of manipulating
some data structure.
> 2) Do as you note below and add wait_event_killable_timeout()
Hum,
I do think that would be better but, (please correct me if I'm wrong)
the _killable_ variants only allow
SIGKILL (and not SIGINT).
0cb64249ca "firmware_loader: abort request if wait_for_completion is
interrupted"
specifically mentrions ctrl-c (SIGINT) in the commit message so that
would no longer work.
Myself I think having to use kill -9 to interrupt firmware loading by a
usespace helper is OK but others may disagree.
> I do not see why we could not introduce wait_event_killable_timeout()
> and swait_event_killable_timeout() into -stables.
> After seeing how simple it is to do so I tend to agree. Greg, Peter,
> what are your thoughts ?
>
> Martin Fuzzey can you test this patch as an alternative to your issue ?
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b9f907eedbf7..70fc42e5e0da 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
> {
> long ret;
>
> - ret = swait_event_interruptible_timeout(fw_st->wq,
> + ret = swait_event_killable_timeout(fw_st->wq,
> __fw_state_is_done(READ_ONCE(fw_st->status)),
> timeout);
> if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> diff --git a/include/linux/swait.h b/include/linux/swait.h
> index c1f9c62a8a50..9c5ca2898b2f 100644
> --- a/include/linux/swait.h
> +++ b/include/linux/swait.h
> @@ -169,4 +169,29 @@ do { \
> __ret; \
> })
>
> +#define __swait_event_killable(wq, condition) \
> + (void)___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
> +
> +#define swait_event_killable(wq, condition) \
> +({ \
> + int __ret = 0; \
> + if (!(condition)) \
> + __ret = __swait_event_killable(wq, condition); \
> + __ret; \
> +})
> +
> +#define __swait_event_killable_timeout(wq, condition, timeout) \
> + ___swait_event(wq, ___wait_cond_timeout(condition), \
> + TASK_INTERRUPTIBLE, timeout, \
> + __ret = schedule_timeout(__ret))
> +
Should be TASK_KILLABLE above
> +#define swait_event_killable_timeout(wq, condition, timeout) \
> +({ \
> + long __ret = timeout; \
> + if (!___wait_cond_timeout(condition)) \
> + __ret = __swait_event_killable_timeout(wq, \
> + condition, timeout); \
> + __ret; \
> +})
> +
> #endif /* _LINUX_SWAIT_H */
>
> Luis
After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.
Regards,
Martin
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-06-06 18:40 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPpGq-2N6-27@gated-at.bofh.it> |
| In reply to | #1658504 |
Adding fsdevel for review on the correct semantics of handling signals on
write(), in this case a sysfs write which triggered a sync request firmware
call and what the firmware API should return in such case of a signal (I gather
this should be -EINTR and not -ERESTARTSYS). Also whether or not SIGINT should
be followed or if only allowing SIGKILL is fine (fine by me, but it would
change old behaviour).
Hoping between fsdevel and linux-api folks we can hash this out.
On Tue, Jun 06, 2017 at 11:04:37AM +0200, Martin Fuzzey wrote:
> On 05/06/17 22:24, Luis R. Rodriguez wrote:
> >
> >
> > For these two reasons then it would seem best we do two things actually:
> >
> > 1) return -EINTR instead of -EAGAIN when we detect swait_event_interruptible_timeout()
> > got interrupted by a signal (it returns -ERESTARTSYS)
>
>
> I disagree. That would force userspace to handle the signal rather than
> having the kernel retry.
>
> From Documentation/DocBook/kernel-hacking.tmpl:
>
> After you slept you should check if a signal occurred: the
> Unix/Linux way of handling signals is to temporarily exit the
> system call with the <constant>-ERESTARTSYS</constant> error. The
> system call entry code will switch back to user context, process
> the signal handler and then your system call will be restarted
> (unless the user disabled that). So you should be prepared to
> process the restart, e.g. if you're in the middle of manipulating
> some data structure.
This applies but you are missing my point that the LWN article [0] I referred
to also stated "Kernel code which uses interruptible sleeps must always check
to see whether it woke up as a result of a signal, and, if so, clean up
whatever it was doing and return -EINTR back to user space." -- I realize there
may be contradiction with above documentation -- this perhaps can be clarified
with fsdevel folks *but* regardless of that the same article notes Alan Cox
explains that "Unix tradition (and thus almost all applications) believe file
store writes to be non signal interruptible. It would not be safe or practical
to change that guarantee." So for this reason alone there does seem to be an
exemption to the above documentation worth noting for file store writes, and
the patch which you tested below *moves* the sysfs write op for firmware in
that direction by adding a new killable swait.
[0] https://lwn.net/Articles/288056/
> > 2) Do as you note below and add wait_event_killable_timeout()
>
> Hum, I do think that would be better but, (please correct me if I'm wrong)
> the _killable_ variants only allow SIGKILL (and not SIGINT).
That seems correct given a TASK_KILLABLE is also TASK_UNINTERRUPTIBLE.
> 0cb64249ca "firmware_loader: abort request if wait_for_completion is
> interrupted"
>
> specifically mentrions ctrl-c (SIGINT) in the commit message so that would
> no longer work.
Great point, but it *also* allowed SIGKILL, so I do feel the goal was also to
allow it to be killable. I'm afraid that patch probably did not get proper
review from sufficient folks and its worth now asking ourselves what we'd like
to do. I'm fine with letting go of SIGINT for firmware sysfs calls for the
sake of keeping with the long standing unix tradition on write, given we *still
have SIGKILL*.
> Myself I think having to use kill -9 to interrupt firmware loading by a
> usespace helper is OK but others may disagree.
Its why I added fsdevel as well. This is really a semantics and uapi question.
Between fsdevel and linux-api folks I would hope we can come to a sensible
resolution.
> > I do not see why we could not introduce wait_event_killable_timeout()
> > and swait_event_killable_timeout() into -stables.
> > After seeing how simple it is to do so I tend to agree. Greg, Peter,
> > what are your thoughts ?
> >
> > Martin Fuzzey can you test this patch as an alternative to your issue ?
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index b9f907eedbf7..70fc42e5e0da 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
> > {
> > long ret;
> > - ret = swait_event_interruptible_timeout(fw_st->wq,
> > + ret = swait_event_killable_timeout(fw_st->wq,
> > __fw_state_is_done(READ_ONCE(fw_st->status)),
> > timeout);
> > if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> > diff --git a/include/linux/swait.h b/include/linux/swait.h
> > index c1f9c62a8a50..9c5ca2898b2f 100644
> > --- a/include/linux/swait.h
> > +++ b/include/linux/swait.h
> > @@ -169,4 +169,29 @@ do { \
> > __ret; \
> > })
> > +#define __swait_event_killable(wq, condition) \
> > + (void)___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
> > +
> > +#define swait_event_killable(wq, condition) \
> > +({ \
> > + int __ret = 0; \
> > + if (!(condition)) \
> > + __ret = __swait_event_killable(wq, condition); \
> > + __ret; \
> > +})
> > +
> > +#define __swait_event_killable_timeout(wq, condition, timeout) \
> > + ___swait_event(wq, ___wait_cond_timeout(condition), \
> > + TASK_INTERRUPTIBLE, timeout, \
> > + __ret = schedule_timeout(__ret))
> > +
>
> Should be TASK_KILLABLE above
Oops yes sorry.
> > +#define swait_event_killable_timeout(wq, condition, timeout) \
> > +({ \
> > + long __ret = timeout; \
> > + if (!___wait_cond_timeout(condition)) \
> > + __ret = __swait_event_killable_timeout(wq, \
> > + condition, timeout); \
> > + __ret; \
> > +})
> > +
> > #endif /* _LINUX_SWAIT_H */
> >
> > Luis
>
> After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.
Great, thanks for testing.
Luis
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-06-06 20:00 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPqVQ-3xj-19@gated-at.bofh.it> |
| In reply to | #1658934 |
Used wrong alias for fsdevel now, its linux-fsdevel ...
Luis
On Tue, Jun 06, 2017 at 06:34:01PM +0200, Luis R. Rodriguez wrote:
> Adding fsdevel for review on the correct semantics of handling signals on
> write(), in this case a sysfs write which triggered a sync request firmware
> call and what the firmware API should return in such case of a signal (I gather
> this should be -EINTR and not -ERESTARTSYS). Also whether or not SIGINT should
> be followed or if only allowing SIGKILL is fine (fine by me, but it would
> change old behaviour).
>
> Hoping between fsdevel and linux-api folks we can hash this out.
>
> On Tue, Jun 06, 2017 at 11:04:37AM +0200, Martin Fuzzey wrote:
> > On 05/06/17 22:24, Luis R. Rodriguez wrote:
> > >
> > >
> > > For these two reasons then it would seem best we do two things actually:
> > >
> > > 1) return -EINTR instead of -EAGAIN when we detect swait_event_interruptible_timeout()
> > > got interrupted by a signal (it returns -ERESTARTSYS)
> >
> >
> > I disagree. That would force userspace to handle the signal rather than
> > having the kernel retry.
> >
> > From Documentation/DocBook/kernel-hacking.tmpl:
> >
> > After you slept you should check if a signal occurred: the
> > Unix/Linux way of handling signals is to temporarily exit the
> > system call with the <constant>-ERESTARTSYS</constant> error. The
> > system call entry code will switch back to user context, process
> > the signal handler and then your system call will be restarted
> > (unless the user disabled that). So you should be prepared to
> > process the restart, e.g. if you're in the middle of manipulating
> > some data structure.
>
> This applies but you are missing my point that the LWN article [0] I referred
> to also stated "Kernel code which uses interruptible sleeps must always check
> to see whether it woke up as a result of a signal, and, if so, clean up
> whatever it was doing and return -EINTR back to user space." -- I realize there
> may be contradiction with above documentation -- this perhaps can be clarified
> with fsdevel folks *but* regardless of that the same article notes Alan Cox
> explains that "Unix tradition (and thus almost all applications) believe file
> store writes to be non signal interruptible. It would not be safe or practical
> to change that guarantee." So for this reason alone there does seem to be an
> exemption to the above documentation worth noting for file store writes, and
> the patch which you tested below *moves* the sysfs write op for firmware in
> that direction by adding a new killable swait.
>
> [0] https://lwn.net/Articles/288056/
>
> > > 2) Do as you note below and add wait_event_killable_timeout()
> >
> > Hum, I do think that would be better but, (please correct me if I'm wrong)
> > the _killable_ variants only allow SIGKILL (and not SIGINT).
>
> That seems correct given a TASK_KILLABLE is also TASK_UNINTERRUPTIBLE.
>
> > 0cb64249ca "firmware_loader: abort request if wait_for_completion is
> > interrupted"
> >
> > specifically mentrions ctrl-c (SIGINT) in the commit message so that would
> > no longer work.
>
> Great point, but it *also* allowed SIGKILL, so I do feel the goal was also to
> allow it to be killable. I'm afraid that patch probably did not get proper
> review from sufficient folks and its worth now asking ourselves what we'd like
> to do. I'm fine with letting go of SIGINT for firmware sysfs calls for the
> sake of keeping with the long standing unix tradition on write, given we *still
> have SIGKILL*.
>
> > Myself I think having to use kill -9 to interrupt firmware loading by a
> > usespace helper is OK but others may disagree.
>
> Its why I added fsdevel as well. This is really a semantics and uapi question.
> Between fsdevel and linux-api folks I would hope we can come to a sensible
> resolution.
>
> > > I do not see why we could not introduce wait_event_killable_timeout()
> > > and swait_event_killable_timeout() into -stables.
> > > After seeing how simple it is to do so I tend to agree. Greg, Peter,
> > > what are your thoughts ?
> > >
> > > Martin Fuzzey can you test this patch as an alternative to your issue ?
> > >
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index b9f907eedbf7..70fc42e5e0da 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -131,7 +131,7 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
> > > {
> > > long ret;
> > > - ret = swait_event_interruptible_timeout(fw_st->wq,
> > > + ret = swait_event_killable_timeout(fw_st->wq,
> > > __fw_state_is_done(READ_ONCE(fw_st->status)),
> > > timeout);
> > > if (ret != 0 && fw_st->status == FW_STATUS_ABORTED)
> > > diff --git a/include/linux/swait.h b/include/linux/swait.h
> > > index c1f9c62a8a50..9c5ca2898b2f 100644
> > > --- a/include/linux/swait.h
> > > +++ b/include/linux/swait.h
> > > @@ -169,4 +169,29 @@ do { \
> > > __ret; \
> > > })
> > > +#define __swait_event_killable(wq, condition) \
> > > + (void)___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
> > > +
> > > +#define swait_event_killable(wq, condition) \
> > > +({ \
> > > + int __ret = 0; \
> > > + if (!(condition)) \
> > > + __ret = __swait_event_killable(wq, condition); \
> > > + __ret; \
> > > +})
> > > +
> > > +#define __swait_event_killable_timeout(wq, condition, timeout) \
> > > + ___swait_event(wq, ___wait_cond_timeout(condition), \
> > > + TASK_INTERRUPTIBLE, timeout, \
> > > + __ret = schedule_timeout(__ret))
> > > +
> >
> > Should be TASK_KILLABLE above
>
> Oops yes sorry.
>
> > > +#define swait_event_killable_timeout(wq, condition, timeout) \
> > > +({ \
> > > + long __ret = timeout; \
> > > + if (!___wait_cond_timeout(condition)) \
> > > + __ret = __swait_event_killable_timeout(wq, \
> > > + condition, timeout); \
> > > + __ret; \
> > > +})
> > > +
> > > #endif /* _LINUX_SWAIT_H */
> > >
> > > Luis
> >
> > After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.
>
> Great, thanks for testing.
>
> Luis
>
--
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg
[toc] | [prev] | [next] | [standalone]
| From | Alan Cox <alan@linux.intel.com> |
|---|---|
| Date | 2017-06-06 17:00 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPo7D-1Hh-1@gated-at.bofh.it> |
| In reply to | #1658130 |
> "Unix tradition (and thus almost all applications) believe file store > writes to > be non signal interruptible. It would not be safe or practical to > change that > guarantee." Yep everyone codes write(disk_file, "foo", 3); not while(..) blah around it. > For these two reasons then it would seem best we do two things > actually: > > 1) return -EINTR instead of -EAGAIN when we detect > swait_event_interruptible_timeout() > got interrupted by a signal (it returns -ERESTARTSYS) > 2) Do as you note below and add wait_event_killable_timeout() Pedantic detail that I don't think affects you If you have completed a part of the I/O then you should return the byte processed count not EINTR, but -1,EINTR if no progress was made. Alan
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-06-06 18:50 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPpQ5-2QG-11@gated-at.bofh.it> |
| In reply to | #1658842 |
Adding fsdevel folks. On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote: > > "Unix tradition (and thus almost all applications) believe file store > > writes to > > be non signal interruptible. It would not be safe or practical to > > change that > > guarantee." > > Yep everyone codes > > write(disk_file, "foo", 3); > > not while(..) blah around it. Thanks for the confirmation! That's a simple enough explanation. > > For these two reasons then it would seem best we do two things > > actually: > > > > 1) return -EINTR instead of -EAGAIN when we detect > > swait_event_interruptible_timeout() > > got interrupted by a signal (it returns -ERESTARTSYS) > > 2) Do as you note below and add wait_event_killable_timeout() > > Pedantic detail that I don't think affects you > > If you have completed a part of the I/O then you should return the byte > processed count not EINTR, but -1,EINTR if no progress was made. You are right with some new exceptions and with regards to the future: The syfs loading interface for firmware currently goes through the data file exposed on syfs, the respective write op firmware_data_write() only checks for signals at the beginning. After that its a full one swoop try to write if you are following the old tradition and are using a buffer allocated by the firmware API. If you are using the relatively new request_firmware_into_buf() added by Stephen Boyd which lets the driver provide the allocated buffer then we have a loop in firmware_rw() which should be fixed to: 1) Check for signals 2) Do what you noted above. Furthermore Yi Li over at Intel is adding some new API calls which would re-use some of this for FPGA firmwares which are also very large, that work should consider the above and fix appropriately as well. Luis
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-06-06 20:00 +0200 |
| Subject | Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback |
| Message-ID | <tPqVQ-3xj-5@gated-at.bofh.it> |
| In reply to | #1658940 |
Using the right linux-fsdevel this time also, this was the second reply. Luis On Tue, Jun 06, 2017 at 06:47:34PM +0200, Luis R. Rodriguez wrote: > Adding fsdevel folks. > > On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote: > > > "Unix tradition (and thus almost all applications) believe file store > > > writes to > > > be non signal interruptible. It would not be safe or practical to > > > change that > > > guarantee." > > > > Yep everyone codes > > > > write(disk_file, "foo", 3); > > > > not while(..) blah around it. > > Thanks for the confirmation! That's a simple enough explanation. > > > > For these two reasons then it would seem best we do two things > > > actually: > > > > > > 1) return -EINTR instead of -EAGAIN when we detect > > > swait_event_interruptible_timeout() > > > got interrupted by a signal (it returns -ERESTARTSYS) > > > 2) Do as you note below and add wait_event_killable_timeout() > > > > Pedantic detail that I don't think affects you > > > > If you have completed a part of the I/O then you should return the byte > > processed count not EINTR, but -1,EINTR if no progress was made. > > You are right with some new exceptions and with regards to the future: > > The syfs loading interface for firmware currently goes through the > data file exposed on syfs, the respective write op firmware_data_write() > only checks for signals at the beginning. After that its a full one > swoop try to write if you are following the old tradition and are using > a buffer allocated by the firmware API. > > If you are using the relatively new request_firmware_into_buf() added > by Stephen Boyd which lets the driver provide the allocated buffer then > we have a loop in firmware_rw() which should be fixed to: > > 1) Check for signals > 2) Do what you noted above. > > Furthermore Yi Li over at Intel is adding some new API calls which would > re-use some of this for FPGA firmwares which are also very large, that > work should consider the above and fix appropriately as well. > > Luis > -- Luis Rodriguez, SUSE LINUX GmbH Maxfeldstrasse 5; D-90409 Nuernberg
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web