firmware: request_firmware() should propagate -ERESTARTSYS

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 <[email protected]>

---
 drivers/base/firmware_class.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Quoted 6 lines · Martin Fuzzey

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?

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Martin Fuzzey

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.

Quoted 10 lines · Martin Fuzzey

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Martin Fuzzey

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 [email protected] # 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 <[email protected]>

Cc [email protected] # 4.0

Acked-by Luis R. Rodriguez <[email protected]>

==========================================================================

Quoted 1 line

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

Quoted 3 lines

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.

From: Martin Fuzzey <[email protected]>

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.

Quoted 8 lines · Luis R. Rodriguez

From: Martin Fuzzey <[email protected]>

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?

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Luis R. Rodriguez

From: Martin Fuzzey <[email protected]>

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.

Quoted 6 lines

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().

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.

Quoted 3 lines

Quoted 3 lines

Can you give a simple example of what's going on and why it matters?

Quoted 5 lines

Quoted 4 lines

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?

Quoted 1 line

Quoted 1 line

Maybe SIGCHLD shouldn't interrupt firmware loading?

Quoted 3 lines · Andy Lutomirski

Quoted 3 lines

Quoted 3 lines

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)

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 3 lines · Andy Lutomirski

Quoted 3 lines

Quoted 3 lines

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)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 8 lines · Martin

Quoted 1 line

Quoted 1 line

Quoted 1 line

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.

Quoted 11 lines · Martin

Quoted 3 lines · Andy Lutomirski

Quoted 3 lines

Quoted 3 lines

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.

Quoted 6 lines

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:

No no, this is not how the fallback system works.

Quoted 1 line

5) A child dies and raises SIGCHLD

What child? The process doing the write() ?

Martin seems to be arguing -ERESTARTSYS should be sent back instead given it is what the wait returned after all.

If you're talking about a custom driver of sorts that triggered a request_firmware() call (note sync) then yes your earlier description is accurate and in this case as well the driver specific sysfs interface can end up returning the same error.

If this file is custom then its up to you to decide what you want for error on that file, but for the firmware_class.c I do agree on returning an agreed upon error so drivers can Do The Right Thing (TM).

Quoted 4 lines

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

OK so this seems to reveal an internal working of some android loader of some sort. Thanks.

Quoted 1 line

Note that, on the the userspace side write() is only called once

The write is to the custom driver trigger which calls request_firmware() ?

This could mean we get a loop on finit_module() if a signal is detected on firmware_request() on every call. Is that fine? Is it expected ?

So you seem to be suggesting the driver's should decide to mask or not -ERSTARTSYS.

Quoted 2 lines · Luis R. Rodriguez

No no, this is not how the fallback system works.

The sysfs file I was talking about is *not* the sysfs file involved in the firmware loading mechanism
but a *custom* sysfs file used by a driver to *trigger* the call to request_firmware() [synchronous] in the first place.

That is, my driver does not do request_firmware() in its probe() function like most but when requested by
userspace. That's a valid usage as far as I can tell. Nothing says request_firmware() is only allowed from probe.

Not sure it is relevant here but here's the reason for doing it this
way: (skip this bit if not interested)
I have a small microcontroller used to manage the power to the main CPU. It is connected to the CPU by a GPIO line.
On power up the microcrontroller powers up the main CPU and starts a timer. If the application doesn't start in time the microcontroller power cycles the CPU.

In addition to the the above the microcontroller is connected to the CPU by a I2C bus for various other functions
including firmware update (of the microcontroller software).

In order to keep the critical power up code on the microcontroller very simple the I2C connection is not available
until *after* the power up confirmation by the GPIO line.

From reading the below it seems this is right.

So, on the linux side, there is a custom driver which exports a sysfs entry that userspace writes to in order to confirm startup.

The driver, when that sysfs file is written to, first sets the GPIO line to signal the microcontroller that the application has started so it can stop its timer and activate the I2C interface.

Then it does a request_firmware() to obtain the firmware the microcontroler is supposed to have. It then verifies it using commands over I2C to compare checksums etc and updates it if needed.

Quoted 5 lines

Quoted 4 lines

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

Quoted 3 lines

OK so this seems to reveal an internal working of some android loader of some sort. Thanks.

Nothing Android specific here.

This is the standard *linux* behaviour.
The retry is done *in the kernel* not on the android userspace side

Quoted 2 lines

This could mean we get a loop on finit_module() if a signal is detected on firmware_request() on every call. Is that fine? Is it expected ?

Ok so here we are talking about the standard case

of request_firmware() being
called from probe() If the driver is a loadable module then yes the it will be retried. If it gets a signal on every try then something else is wrong I'd say

If the driver is compiled in then there is no retry since the call to probe doesn't go through the syscall machinary. But there shouldn't be a signal either in that
case since it's not being called from a userspace process in that case.

Quoted 3 lines

So you seem to be suggesting the driver's should decide to mask or not -ERSTARTSYS.

Only if the driver knows that it is not restartable *itself*. Shoudn't happen very often

Quoted 10 lines · Eric W. Biederman

Quoted 8 lines · Martin

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 2 lines

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.

Quoted 11 lines · Luis R. Rodriguez

Quoted 10 lines · Eric W. Biederman

Quoted 8 lines · Martin

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 23 lines · Dmitry Torokhov

Quoted 11 lines · Luis R. Rodriguez

Quoted 10 lines · Eric W. Biederman

Quoted 8 lines · Martin

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

Quoted 4 lines

Quoted 1 line

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 ?

Quoted 14 lines · Luis R. Rodriguez

Quoted 12 lines · Dmitry Torokhov

Quoted 11 lines · Luis R. Rodriguez

Quoted 10 lines · Eric W. Biederman

Quoted 8 lines · Martin

Quoted 1 line

Quoted 1 line

Quoted 1 line

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)

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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/

Quoted 6 lines

Quoted 6 lines

I think only interrupting firmware loading with fatal signals would make a lot of sense.

Quoted 2 lines

ret = swait_event_interruptible_timeout() is being used right now.

Quoted 4 lines

Quoted 2 lines

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.

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

drivers/base/firmware_class.c +1 -1

index b9f907eedbf7..70fc42e5e0da 100644
@@ -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)

include/linux/swait.h +6 -0

index c1f9c62a8a50..9c5ca2898b2f 100644
@@ -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;							\

include/linux/swait.h +9 -0

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

"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()

Quoted 6 lines · Luis R. Rodriguez

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.

Quoted 1 line

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.

Quoted 36 lines

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 ?

drivers/base/firmware_class.c +1 -1

index b9f907eedbf7..70fc42e5e0da 100644
@@ -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)

include/linux/swait.h +10 -0

index c1f9c62a8a50..9c5ca2898b2f 100644
@@ -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;								\
+})

include/linux/swait.h do { \ · line 187

Quoted 6 lines

include/linux/swait.h +6 -0

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

Quoted 11 lines

include/linux/swait.h +9 -0

+#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 */
 

Quoted 1 line

Luis

After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.

Quoted 5 lines

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

Quoted 7 lines

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.

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.

Quoted 14 lines · Martin Fuzzey

Quoted 6 lines · Luis R. Rodriguez

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:

Quoted 8 lines

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/

Quoted 4 lines

Quoted 1 line

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.

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 36 lines

Quoted 36 lines

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 ?

drivers/base/firmware_class.c +1 -1

index b9f907eedbf7..70fc42e5e0da 100644
@@ -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)

include/linux/swait.h +10 -0

index c1f9c62a8a50..9c5ca2898b2f 100644
@@ -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;								\
+})

Quoted 8 lines

include/linux/swait.h do { \ · line 187

Quoted 6 lines

include/linux/swait.h +6 -0

+
+#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.

Quoted 11 lines

Quoted 11 lines

include/linux/swait.h +9 -0

+#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 */
 

Quoted 3 lines

Quoted 1 line

Luis

After replacing TASK_INTERRUPTIBLE with TASK_KILLABLE above it works for me.

Great, thanks for testing.

Luis

Adding fsdevel folks.

Quoted 11 lines · Alan Cox

Quoted 5 lines

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

Quoted 8 lines

Quoted 7 lines

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

Quoted 4 lines

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

Quoted 5 lines · Alan Cox

Yep everyone codes

	write(disk_file, "foo", 3);

not while(..) blah around it.

Quoted 6 lines · Luis R. Rodriguez

Quoted 5 lines · Alan Cox

Yep everyone codes

	write(disk_file, "foo", 3);

not while(..) blah around it.

In general I/O to tty devices and other character mode devices was where you definitely needed to check for EINTR/EAGAIN because that was the place where historically Unix systems would interrupt system calls --- e.g., a user typing control-Z, for example.

And in general writes to file systems and block devices in *general* were never interrupted by signals, although that was always a non-portable assumption.

So I've always subscribed to the "be liberal in what you accept, conservative in what you send" rule of thumb. Which is to say, any programs *I* write I'll in general always check for EINTR/EAGAIN and check for partial writes, but in general, as a kernel program I try to adhere to the long-standing Unix trandition for disk based files.

This does beg the question about whether firmware devices are more like tty devices or block devices or files, though.

If before signals
never caused them to return EINTR/EAGAIN, then it's probably best to not break backwards compatbility.

That being said, note that you also have the option of using -ERESTARTNOINTR (always restart the system call, regardless of how the sighandle flags were set), and -ERESTARTNOHAND (restart the system call always if there was no signal handler and the process was not killed), in addition to -ERESTARTSYS.

So that might be another option
that's fairly easy to implement or experiment with.

Quoted 2 lines · Theodore Ts'o

Quoted 1 line · Luis R. Rodriguez

On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:

We rely on swait, and swait right now only uses -ERESTARTSYS. Are you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR or -ERESTARTNOHAND if we see fit for some future functionality / need ?

Quoted 4 lines · Luis R. Rodriguez

Quoted 2 lines · Theodore Ts'o

Quoted 1 line · Luis R. Rodriguez

On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:

Quoted 3 lines

We rely on swait, and swait right now only uses -ERESTARTSYS. Are you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR or -ERESTARTNOHAND if we see fit for some future functionality / need ?

I think that has essentially nothing to do with swait. User code does some syscall. That syscall triggers a firmware load. The caller gets a signal. If you're going to let firmware load get interrupted, you need to consider what the syscall is.

What's wrong with saying that the only way to interrupt firmware loading is to kill the process? So ctrl-c will no longer interrupt it, but I do not think that ease of aborting firmware update is primary goal here. I consider simple is good here.

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

Quoted 4 lines

What's wrong with saying that the only way to interrupt firmware loading is to kill the process? So ctrl-c will no longer interrupt it, but I do not think that ease of aborting firmware update is primary goal here. I consider simple is good here.

Agreed 100%. The user process did not ask for firmware load, it asked for an I/O operation. Semantically it should appear as if someone else did the firmware load and it just had to wait for it to happen.

Quoted 5 lines · Martin

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

Martin, just for completeness on documenting on the commit log of the next swait proposed fix for this -- what signal did the process get from which you note the child dies below ? Exactly what in Android sent this signal ?

Quoted 9 lines · Luis R. Rodriguez

Quoted 6 lines · Martin

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

Quoted 4 lines

Martin, just for completeness on documenting on the commit log of the next swait proposed fix for this -- what signal did the process get from which you
note the child dies below ? Exactly what in Android sent this signal ?

Android didn't send the signal, the kernel did (SIGCHLD).

Like this:

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally unrelated to firmware loading]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason - in my case normal termination)

5) Kernel delivers SIGCHLD to pid=1 to tell it a child has died, which causes -ERESTARTSYS to be returned from wait_*

Quoted 9 lines · Andy Lutomirski

Quoted 7 lines · Luis R. Rodriguez

Quoted 2 lines · Theodore Ts'o

Quoted 1 line · Luis R. Rodriguez

On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:

We rely on swait, and swait right now only uses -ERESTARTSYS. Are you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR or -ERESTARTNOHAND if we see fit for some future functionality / need ?

Quoted 4 lines

I think that has essentially nothing to do with swait. User code does some syscall. That syscall triggers a firmware load. The caller gets a signal. If you're going to let firmware load get interrupted, you need to consider what the syscall is.

I think it is way too complicated and I do not think driver writers will stand a chance of implementing this correctly, given that often firmware load might be triggered indirectly and by multitude of syscalls.

Quoted 15 lines · Dmitry Torokhov

Quoted 13 lines · Andy Lutomirski

Quoted 7 lines · Luis R. Rodriguez

Quoted 2 lines · Theodore Ts'o

Quoted 1 line · Luis R. Rodriguez

On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:

We rely on swait, and swait right now only uses -ERESTARTSYS. Are you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR or -ERESTARTNOHAND if we see fit for some future functionality / need ?

I think that has essentially nothing to do with swait. User code does some syscall. That syscall triggers a firmware load. The caller gets a signal. If you're going to let firmware load get interrupted, you need to consider what the syscall is.

Quoted 4 lines

I think it is way too complicated and I do not think driver writers will stand a chance of implementing this correctly, given that often firmware load might be triggered indirectly and by multitude of syscalls.

That's what I meant, but I said it unclearly. I meant that, if we're going to start allowing interruption, we would need to audit all the callers. Ugh.

I suppose we could have request_firmware_interruptable(), but that seems like it's barely worth it.

Quoted 20 lines · Martin Fuzzey

Quoted 9 lines · Luis R. Rodriguez

Quoted 6 lines · Martin

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

Quoted 4 lines

Martin, just for completeness on documenting on the commit log of the next swait proposed fix for this -- what signal did the process get from which you
note the child dies below ? Exactly what in Android sent this signal ?

Android didn't send the signal, the kernel did (SIGCHLD).

Like this:

Quoted 8 lines

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally unrelated to firmware loading]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason - in my case normal termination)

Interesting, could one interpretation here be that the process successfully finishing + the signal being sent beats out the timing of the firmware_class syfs code detecting that the write completed ?

Quoted 2 lines

5) Kernel delivers SIGCHLD to pid=1 to tell it a child has died, which causes -ERESTARTSYS to be returned from wait_*

Luis

Quoted 4 lines · Luis R. Rodriguez

Quoted 4 lines

Android didn't send the signal, the kernel did (SIGCHLD).

Like this:

Quoted 8 lines

Quoted 8 lines

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally unrelated to firmware loading]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason - in my case normal termination)

Martin just to be clear, by "normal case termination" do you mean completing successfully ?? Ie the firmware actually did make it onto the device ?

Quoted 13 lines · Luis R. Rodriguez

Quoted 4 lines · Luis R. Rodriguez

Quoted 4 lines

Android didn't send the signal, the kernel did (SIGCHLD).

Like this:

Quoted 8 lines

Quoted 8 lines

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally unrelated to firmware loading]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason - in my case normal termination)

Quoted 3 lines

Martin just to be clear, by "normal case termination" do you mean completing successfully ?? Ie the firmware actually did make it onto the device ?

The firmware did *not* make it onto the device since the request_firmware() call returned an error
(the code that would have transfered it to the device is only executed following a successful request_firmware)

The process that terminates normally is unrelated to firmware loading as I said above.

The only things that matter are:
- It is a child process of the process that calls request_firmware()
- It terminates *while* the the wait_ is still in progress

Here is a way of reproducing the problem using the test_firmware module (which I only just saw) on normal linux with no Android or custom driver

#!/bin/sh
set -e

# Make sure the system firmware loader doesn't get in the way /etc/init.d/udev stop

modprobe test_firmware

DIR=/sys/devices/virtual/misc/test_firmware

echo 10 >/sys/class/firmware/timeout;
sleep 2 &
echo -n "/some/non/existing/file.bin" > "$DIR"/trigger_request;

If run with the "sleep 2 &" it terminates after 2 seconds
If the sleep is commented it runs for the expected 10 seconds (the firmware loading timeout)

Since the sleep process is a child of the script process requesting a firmware load its death causes a SIGCHLD causing request_firmware() to abort prematurely.

Quoted 3 lines · Andy Lutomirski

That's what I meant, but I said it unclearly. I meant that, if we're going to start allowing interruption, we would need to audit all the callers. Ugh.

There are actually two audits worth evaluating if what we've concluded is fair game:

a) firmware sync calls on interruptible paths
b) use of swait / old interruptible waits on sysfs paths

Quoted 13 lines · Luis R. Rodriguez

Quoted 4 lines · Luis R. Rodriguez

Quoted 4 lines

Android didn't send the signal, the kernel did (SIGCHLD).

Like this:

Quoted 8 lines

Quoted 8 lines

1) Android init (pid=1) fork()s (say pid=42) [this child process is totally unrelated to firmware loading]
2) Android init (pid=1) does a write() on a (driver custom) sysfs file which ends up calling request_firmware() kernel side
3) The firmware loading fallback mechanism is used, the request is sent to userspace and pid 1 waits in the kernel on wait_*
4) before firmware loading completes pid 42 dies (for any reason - in my case normal termination)

Quoted 3 lines

Martin just to be clear, by "normal case termination" do you mean completing successfully ?? Ie the firmware actually did make it onto the device ?

The firmware did *not* make it onto the device since the request_firmware() call returned an error
(the code that would have transfered it to the device is only executed following a successful request_firmware)

The process that terminates normally is unrelated to firmware loading as I said above.

The only things that matter are:
- It is a child process of the process that calls request_firmware()
- It terminates *while* the the wait_ is still in progress

Here is a way of reproducing the problem using the test_firmware module (which I only just saw) on normal linux with no Android or custom driver

#!/bin/sh
set -e

# Make sure the system firmware loader doesn't get in the way /etc/init.d/udev stop

modprobe test_firmware

DIR=/sys/devices/virtual/misc/test_firmware

echo 10 >/sys/class/firmware/timeout;
sleep 2 &
echo -n "/some/non/existing/file.bin" > "$DIR"/trigger_request;

If run with the "sleep 2 &" it terminates after 2 seconds
If the sleep is commented it runs for the expected 10 seconds (the firmware loading timeout)

Since the sleep process is a child of the script process requesting a firmware load its death causes a SIGCHLD causing request_firmware() to abort prematurely.