Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

["Fuzzey, Martin" <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>]

On 26 May 2017 at 21:40, Luis R. Rodriguez <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, May 25, 2017 at 10:28:38AM +0200, Fuzzey, Martin wrote:
>> On 25 May 2017 at 06:13, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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:
>>
>
> No no, this is not how the fallback system works.
>

I was (implicitly) describing the fallback mechanism.
Indeed as you say the kernel now first tries to load the firmware
itself without involving userspace.
I'm describing what happens *after* this fails and the kernel falls
back to userspace

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.

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.


>> 5) A child dies and raises SIGCHLD
>
> What child? The process doing the write() ?
>
A child of the process doing the write on the drivers custom sysfs
entry that triggered the request_firmware()

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

Yes


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

Exactly


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

Yes


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

Nothing Android specific here.

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


>> Note that, on the the userspace side  write() is only called once
>
> The write is to the custom driver trigger which calls request_firmware() ?
>

Yes



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

> 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


Martin