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

["Luis R. Rodriguez" <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Jun 6, 2017 at 11:25 PM, Dmitry Torokhov
> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Jun 06, 2017 at 09:56:47PM -0700, Andy Lutomirski wrote:
>>> On Tue, Jun 6, 2017 at 5:22 PM, Luis R. Rodriguez <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> > On Tue, Jun 06, 2017 at 06:11:51PM -0400, Theodore Ts'o wrote:
>>> >> On Tue, Jun 06, 2017 at 06:47:34PM +0200, Luis R. Rodriguez wrote:
>>> >> > 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.
>>
>> 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.

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

As for a) we drove tons of code away from using sync, request
firmware, and on async firmware the return value is lost, we only can
currently know if a failure of some sort occurred. If that push to
async failed we also scared away tons of drivers to use
request_firmware() calls on init and later incorrectly on probe due to
serialized init + probe delay on boot. From what I recall my last
Coccinelle evil-monster-hunt, I did indeed find quite a bit of drivers
still still relying on sync firmware which ultimately revealed use on
a probe path. The signal however was only effective as of commit
0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
is interrupted") merged as of v4.0. Creating awareness of the issue
seems fair, but I don't think its worth a huge fly swatter.

I have no information to contribute for b) other than I was reluctant
to even consider it.

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

I don't think that's worth it, given the signal was effective only as
of v4.0, we already had a big push away from sync requests, and also
had the "don't use request firmware" on init scare which also
propagated to probe later.

  Luis