Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470064 > unrolled thread

Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe

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.

Contents

  Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe Daniel Vetter <daniel.vetter@ffwll.ch> - 2016-08-25 13:10 +0200
    Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-25 21:50 +0200
      Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe Daniel Vetter <daniel.vetter@ffwll.ch> - 2016-08-25 22:20 +0200
        Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-25 22:30 +0200
      Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-08-25 22:40 +0200
      Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on  init or probe "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-03 02:00 +0200
        [RFC] fs: add userspace critical mounts event support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-03 02:30 +0200
          Re: [RFC] fs: add userspace critical mounts event support Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-03 06:20 +0200
            Re: [RFC] fs: add userspace critical mounts event support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-09-03 06:30 +0200
              Re: [RFC] fs: add userspace critical mounts event support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-09-03 20:00 +0200
                Re: [RFC] fs: add userspace critical mounts event support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-09-03 20:20 +0200
                  Re: [RFC] fs: add userspace critical mounts event support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-07 00:00 +0200
                    Re: [RFC] fs: add userspace critical mounts event support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-09-07 00:30 +0200
                      Re: [RFC] fs: add userspace critical mounts event support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-07 01:20 +0200
                Re: [RFC] fs: add userspace critical mounts event support Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-03 20:30 +0200
            Re: [RFC] fs: add userspace critical mounts event support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-09-06 19:50 +0200
              Re: [RFC] fs: add userspace critical mounts event support Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-06 20:40 +0200
                Re: [RFC] fs: add userspace critical mounts event support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-09-06 23:20 +0200
                  Re: [RFC] fs: add userspace critical mounts event support Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-07 00:00 +0200
                    Re: [RFC] fs: add userspace critical mounts event support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-07 01:10 +0200
                Re: [RFC] fs: add userspace critical mounts event support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-07 00:40 +0200

Page 1 of 2  [1] 2  Next page →

#1470064 — Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe

On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, Aug 24, 2016 at 08:55:55AM +0200, Daniel Vetter wrote:
>> On Fri, Jun 17, 2016 at 12:54 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > Thou shalt not make firmware calls early on init or probe.
>
> <-- snip -->
>
>> > There are 4 offenders at this time:
>> >
>> > mcgrof@ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci
>> > mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report
>> >
>> > drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321.
>> > drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136.
>> > drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796.
>> > drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.
>>
>> Plus all gpu drivers which need firmware. And yes we must load them at
>> probe
>
> Do you have an upstream driver in mind that does this ? Is it on device
> drier module probe or a DRM subsystem specific probe call of some sort ?

i915 is the one I care about for obvious reasons ;-) It's all from the
pci device probe function, but nested really deeply.

>> because people are generally pissed when they boot their machine
>> and the screen goes black. On top of that a lot of people want their
>> gpu drivers to be built-in, but can't ship the firmware blobs in the
>> kernel image because gpl. Yep, there's a bit a contradiction there ...
>
> Can they use initramfs for this ?

Apparently that's also uncool with the embedded folks. Tbh I don't
know exactly why. Also I thought initramfs is available only after
built-in drivers have finished loading, but maybe that changed.

> Also just curious -- as with other subsystems, is it possible to load
> a generic driver first, say vesa, and then a more enhanced one later ?
> I am not saying this is ideal or am I suggesting this, I'd just like
> to know the feasibility of this.

Some users want a fully running gfx stack 2s after power-on. There's
not even enough time to load an uefi or vga driver first. i915
directly initializes the gpu from power-on state on those.

>> I think what would work is loading the different subsystems of the
>> driver in parallel (we already do that largely)
>
> Init level stuff is actually pretty synchronous, and in fact both
> init and probe are called serially. There are a few subsystems which
> have been doing things a bit differently, but these are exceptions.
>
> When you say we already do this largely, can you describe a bit more
> precisely what you mean ?

Oh, this isn't subsystems as in linux device/driver model, but
different parts within the driver. We fire up a bunch of struct work
to get various bits done asynchronously.

>>, and then if one
>> firmware blob isn't there yet simply stall that async worker until it
>> shows up.
>
> Is this an existing framework or do you mean if we add something
> generic to do this async loading of subsystems ?

normal workers, and flush_work and friends to sync up. We have some
really fancy ideas for essentially async init tasks that can declare
their depencies systemd-unit file style, but that's only in the
prototype stage. We need this (eventually) since handling the ordering
correctly is getting unwieldy. But again just struct work launched
from the main pci probe function.

>> But the answers I've gotten thus far from request_firmware()
>> folks (well at least Greg) is don't do that ...
>
> Well in this patch set I'm adding myself as a MAINTAINER and I've
> been extending the firmware API recently to help with a few new
> features I want, I've been wanting to hear more feedback from
> other's needs and I had actually not gotten much -- except
> only recently with the usermode helper and reasons why some
> folks thought they could not use direct firmware loading from
> the fs. I'm keen to hear or more use cases and needs specially if
> they have to do with improving boot time and asynchronous boot.
>
>> Is that problem still somewhere on the radar?
>
> Not mine.
>
>> Atm there's various
>> wait_for_rootfs hacks out there floating in vendor/product trees.
>
> This one I've heard about recently, and I suggested two possible
> solutions, with a preference to a simply notification of when
> rootfs is available from userspace.
>
>> "Avoid at all costs" sounds like upstream prefers to not care about
>> android/cros in those case (yes I know most arm socs don't need
>> firmware, which would make it a problem fo just be a subset of all
>> devices).
>
> In my days of dealing with Android I learned most folks did not frankly
> care too much about upstream-first model. That means things were reactive.
> That's a business mind set and that's fine. However for upstream we want
> what is best and to discuss. I haven't seen proposals so, so long as
> we just hear of "hacks" that some folks do in vendor/product trees,
> what can we do ?

One of the proposals which would have worked for us died a while back:

https://lkml.org/lkml/2014/6/15/47

That's essentially what I'd like to have.

[cut discussion about async probe]

> So .. I agree, let's avoid the hacks. Patches welcomed.

Hm, this is a definite change of tack - back when I discussed this
with Greg about 2 ks ago it sounded like "don't do this". The only
thing we need is some way to wait for rootfs before we do the
request_firmware. Everything else we handle already in the kernel.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [next] | [standalone]

#1470357

Summoning Felix for the embedded aspect on initramfs below.
Jörg might be interested in the async facilities you speak of as well.

On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
> On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Wed, Aug 24, 2016 at 08:55:55AM +0200, Daniel Vetter wrote:
> >> On Fri, Jun 17, 2016 at 12:54 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> > Thou shalt not make firmware calls early on init or probe.
> >
> > <-- snip -->
> >
> >> > There are 4 offenders at this time:
> >> >
> >> > mcgrof@ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci
> >> > mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report
> >> >
> >> > drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321.
> >> > drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136.
> >> > drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796.
> >> > drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.
> >>
> >> Plus all gpu drivers which need firmware. And yes we must load them at
> >> probe
> >
> > Do you have an upstream driver in mind that does this ? Is it on device
> > drier module probe or a DRM subsystem specific probe call of some sort ?
> 
> i915 is the one I care about for obvious reasons ;-) It's all from the
> pci device probe function, but nested really deeply.

The above SmPL grammar should capture well nested calls of calls within probe,
so curious why it didn't pick up i915. Let's see.

i915_pci_probe() --> i915_driver_load() -->
	i915_load_modeset_init() --> (drivers/gpu/drm/i915/i915_drv.c)
		a) intel_csr_ucode_init() (drivers/gpu/drm/i915/intel_csr.c)
		...
		b) intel_guc_init() (drivers/gpu/drm/i915/intel_guc_loader.c)

The two firmwares come from:

a) intel_csr_ucode_init() --> schedule_work(&dev_priv->csr.work);
	csr_load_work_fn() --> request_firmware()

b) intel_guc_init() --> guc_fw_fetch() request_firmware()

---

a) is not dealt with given the grammar does not include scheduled work,
however using work to offload loading firmware seems reasonable here.

b) This should have been picked up, but its not. Upon closer inspection
   the grammar currently expects the module init and probe to be on the
   same file. Loosening this:

diff --git a/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci b/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
index cf180c59e042..e19e6d3dfc0f 100644
--- a/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+++ b/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
@@ -49,7 +49,7 @@ identifier init;
 
 module_init(init);
 
-@ has_probe depends on defines_module_init@
+@ has_probe @
 identifier drv_calls, drv_probe;
 type bus_driver;
 identifier probe_op =~ "(probe)";
@@ -59,7 +59,7 @@ bus_driver drv_calls = {
 	.probe_op = drv_probe,
 };
 
-@hascall depends on !after_start && defines_module_init@
+@hascall depends on !after_start @
 position p;
 @@
 

I get a lot more complaints but still -- i915 b) case is not yet covered:

./drivers/bluetooth/ath3k.c: ERROR: driver call request firmware call on its probe routine on line 546.
./drivers/bluetooth/bcm203x.c: ERROR: driver call request firmware call on its probe routine on line 193.
./drivers/bluetooth/bcm203x.c: ERROR: driver call request firmware call on its probe routine on line 218.
./drivers/bluetooth/bfusb.c: ERROR: driver call request firmware call on its probe routine on line 655.
./drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321.
./drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136.
./drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.
./sound/soc/codecs/wm2000.c: ERROR: driver call request firmware call on its probe routine on line 893.
./sound/soc/sh/siu_dai.c: ERROR: driver call request firmware call on its probe routine on line 747.
./drivers/net/wireless/intersil/orinoco/orinoco_usb.c: ERROR: driver call request firmware call on its probe routine on line 1661.
./sound/soc/intel/common/sst-acpi.c: ERROR: driver call request firmware call on its probe routine on line 161.
./drivers/input/touchscreen/goodix.c: ERROR: driver call request firmware call on its probe routine on line 744.
./drivers/media/usb/go7007/go7007-loader.c: ERROR: driver call request firmware call on its probe routine on line 78.
./drivers/media/usb/go7007/go7007-loader.c: ERROR: driver call request firmware call on its probe routine on line 93.
./drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796.
./drivers/usb/misc/isight_firmware.c: ERROR: driver call request firmware call on its probe routine on line 50.
./drivers/usb/serial/mxuport.c: ERROR: driver call request firmware call on its probe routine on line 1067.
./sound/pci/hda/hda_intel.c: ERROR: driver call request firmware call on its probe routine on line 2015.
./drivers/media/usb/s2255/s2255drv.c: ERROR: driver call request firmware call on its probe routine on line 2304.
./drivers/misc/lattice-ecp3-config.c: ERROR: driver call request firmware call on its probe routine on line 206.
./drivers/net/wireless/ath/carl9170/usb.c: ERROR: driver call request firmware call on its probe routine on line 1103.
./drivers/video/fbdev/metronomefb.c: ERROR: driver call request firmware call on its probe routine on line 681.

At a quick glance these check out though, and it was because I loosened
the requirement to not necessarily have module_init -- usb for instance has
module_usb_driver()...

I believe i915 is not picked up given the request for firmware comes in
on separate files than both the module_init *and* probe calls, that is,
its implemented in drivers/gpu/drm/i915/intel_csr.c. Tuning this a bit more
can take a bit of time, but the intent stands -- as such I think the
current grammar is justified and we should evolve it to catch more and
as it does we touch base again to see what the implications are -- clearly
this means way many more drivers are requesting firmware on probe than
expected.

> >> because people are generally pissed when they boot their machine
> >> and the screen goes black. On top of that a lot of people want their
> >> gpu drivers to be built-in, but can't ship the firmware blobs in the
> >> kernel image because gpl. Yep, there's a bit a contradiction there ...
> >
> > Can they use initramfs for this ?
> 
> Apparently that's also uncool with the embedded folks.

What's uncool with embedded folks? To use initramfs for firmware ?
If so can you explain why ?

> Tbh I don't
> know exactly why. Also I thought initramfs is available only after
> built-in drivers have finished loading, but maybe that changed.

Its still the same, we run through all init calls first (do_initcalls()
and finally the initramfs is processed via prepare_namespace()

do_basic_setup() {
	...
	driver_init();
	...
	do_initcalls();
	...
}

kernel_init_freeable() {
	...
	do_basic_setup();
	...
	if (sys_access((const char __user *) ramdisk_execute_command, 0) != 0) {
		ramdisk_execute_command = NULL;
		prepare_namespace();
        }
}

Felix, are initramfs not kosher for embedded ? If so why not ? The question
here is where to stuff firmware if its needed early in boot and since
graphics folks want video turned on ASAP they often load firmware on probe
and due to licensing may not be able to use the built-in firmware facility.
Using initrafms is what would be expected for these cases then, if that's
not kosher what do you recommend ? Granted video may not be a concern for
embedded but since we're on topic figured we should square this away.

> > Also just curious -- as with other subsystems, is it possible to load
> > a generic driver first, say vesa, and then a more enhanced one later ?
> > I am not saying this is ideal or am I suggesting this, I'd just like
> > to know the feasibility of this.
> 
> Some users want a fully running gfx stack 2s after power-on. There's
> not even enough time to load an uefi or vga driver first. i915
> directly initializes the gpu from power-on state on those.

I see.. thanks.

> >> I think what would work is loading the different subsystems of the
> >> driver in parallel (we already do that largely)
> >
> > Init level stuff is actually pretty synchronous, and in fact both
> > init and probe are called serially. There are a few subsystems which
> > have been doing things a bit differently, but these are exceptions.
> >
> > When you say we already do this largely, can you describe a bit more
> > precisely what you mean ?
> 
> Oh, this isn't subsystems as in linux device/driver model, but
> different parts within the driver. We fire up a bunch of struct work
> to get various bits done asynchronously.

Thanks for the clarification.

> >>, and then if one
> >> firmware blob isn't there yet simply stall that async worker until it
> >> shows up.
> >
> > Is this an existing framework or do you mean if we add something
> > generic to do this async loading of subsystems ?
> 
> normal workers, and flush_work and friends to sync up. We have some
> really fancy ideas for essentially async init tasks that can declare
> their depencies systemd-unit file style, but that's only in the
> prototype stage.

Great, another solution. More reason to talk at KS/Plumbers to see
exactly what each different pipe dream is coming up with.

> We need this (eventually) since handling the ordering
> correctly is getting unwieldy. But again just struct work launched
> from the main pci probe function.

You mean its a struct work based solution ?

> >> But the answers I've gotten thus far from request_firmware()
> >> folks (well at least Greg) is don't do that ...
> >
> > Well in this patch set I'm adding myself as a MAINTAINER and I've
> > been extending the firmware API recently to help with a few new
> > features I want, I've been wanting to hear more feedback from
> > other's needs and I had actually not gotten much -- except
> > only recently with the usermode helper and reasons why some
> > folks thought they could not use direct firmware loading from
> > the fs. I'm keen to hear or more use cases and needs specially if
> > they have to do with improving boot time and asynchronous boot.
> >
> >> Is that problem still somewhere on the radar?
> >
> > Not mine.
> >
> >> Atm there's various
> >> wait_for_rootfs hacks out there floating in vendor/product trees.
> >
> > This one I've heard about recently, and I suggested two possible
> > solutions, with a preference to a simply notification of when
> > rootfs is available from userspace.
> >
> >> "Avoid at all costs" sounds like upstream prefers to not care about
> >> android/cros in those case (yes I know most arm socs don't need
> >> firmware, which would make it a problem fo just be a subset of all
> >> devices).
> >
> > In my days of dealing with Android I learned most folks did not frankly
> > care too much about upstream-first model. That means things were reactive.
> > That's a business mind set and that's fine. However for upstream we want
> > what is best and to discuss. I haven't seen proposals so, so long as
> > we just hear of "hacks" that some folks do in vendor/product trees,
> > what can we do ?
> 
> One of the proposals which would have worked for us died a while back:
> 
> https://lkml.org/lkml/2014/6/15/47

Interesting, the problem stated here, which comes from the fact (as clarified
above) the rootfs comes up only *after* we run do_initcalls() as such there are
possible theoretical races even with request_firmware_nowait() -- as such that
justifies even more the SmPL grammar rule to include even request_firmware_nowait()
on probe / init as this patch has.

Anyway yeah that patch has good intentions but its wrong for a slew of reasons.
The problem is the same as discussed with Bjorn recently. The solution
discussed is that since only userspace knows when the *real* rootfs is ready
(because of slew of reasons) the best we can do is have an event issued by
userspace to inform us of that.

> That's essentially what I'd like to have.

You're not alone.

> [cut discussion about async probe]
> 
> > So .. I agree, let's avoid the hacks. Patches welcomed.
> 
> Hm, this is a definite change of tack - back when I discussed this
> with Greg about 2 ks ago it sounded like "don't do this". The only
> thing we need is some way to wait for rootfs before we do the
> request_firmware. Everything else we handle already in the kernel.

OK so lets just get this userspace event done, and we're set.

  Luis

[toc] | [prev] | [next] | [standalone]

#1470381

Cutting down since a lot of this is probably better discussed at
ks/lpc. Aside, if you want to check out Chris Wilson's work on our new
depency handling, it's called kfence.

https://lkml.org/lkml/2016/7/17/37

On Thu, Aug 25, 2016 at 9:41 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > So .. I agree, let's avoid the hacks. Patches welcomed.
>>
>> Hm, this is a definite change of tack - back when I discussed this
>> with Greg about 2 ks ago it sounded like "don't do this". The only
>> thing we need is some way to wait for rootfs before we do the
>> request_firmware. Everything else we handle already in the kernel.
>
> OK so lets just get this userspace event done, and we're set.

Ah great. As long as that special wait-for-rootfs-pls firmware request
is still allowed, i915&gfx folks will be happy. We will call it from
probe though ;-) but all from our own workers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1470393

On Thu, Aug 25, 2016 at 10:10:52PM +0200, Daniel Vetter wrote:
> Cutting down since a lot of this is probably better discussed at
> ks/lpc. Aside, if you want to check out Chris Wilson's work on our new
> depency handling, it's called kfence.
> 
> https://lkml.org/lkml/2016/7/17/37

Thanks more reading.. :)

> On Thu, Aug 25, 2016 at 9:41 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> > So .. I agree, let's avoid the hacks. Patches welcomed.
> >>
> >> Hm, this is a definite change of tack - back when I discussed this
> >> with Greg about 2 ks ago it sounded like "don't do this". The only
> >> thing we need is some way to wait for rootfs before we do the
> >> request_firmware. Everything else we handle already in the kernel.
> >
> > OK so lets just get this userspace event done, and we're set.
> 
> Ah great. As long as that special wait-for-rootfs-pls firmware request
> is still allowed, i915&gfx folks will be happy. We will call it from
> probe though ;-) but all from our own workers.

We should strive for this to be transparent to drivers, ie, this safeguard of
looking for firmware should be something handled by the kernel as otherwise
we're just forcing a race condition given the review in the last thread.

  Luis

[toc] | [prev] | [next] | [standalone]

#1470405

On Thu, Aug 25, 2016 at 12:41 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:

>> > Can they use initramfs for this ?
>>
>> Apparently that's also uncool with the embedded folks.
>
> What's uncool with embedded folks? To use initramfs for firmware ?
> If so can you explain why ?

Because it is not needed? If you are embedded you have an option of
only compiling drivers and services that you need directly into the
kernel, then initramfs is just an extra piece that complicates your
boot process.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1475503

On Thu, Aug 25, 2016 at 09:41:33PM +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > Some users want a fully running gfx stack 2s after power-on. There's
> > not even enough time to load an uefi or vga driver first. i915
> > directly initializes the gpu from power-on state on those.
> 
> I see.. thanks.
> 
> > >> I think what would work is loading the different subsystems of the
> > >> driver in parallel (we already do that largely)
> > >
> > > Init level stuff is actually pretty synchronous, and in fact both
> > > init and probe are called serially. There are a few subsystems which
> > > have been doing things a bit differently, but these are exceptions.
> > >
> > > When you say we already do this largely, can you describe a bit more
> > > precisely what you mean ?
> > 
> > Oh, this isn't subsystems as in linux device/driver model, but
> > different parts within the driver. We fire up a bunch of struct work
> > to get various bits done asynchronously.
> 
> Thanks for the clarification.

<-- snip -->

> > One of the proposals which would have worked for us died a while back:
> > 
> > https://lkml.org/lkml/2014/6/15/47
> 
> Interesting, the problem stated here, which comes from the fact (as clarified
> above) the rootfs comes up only *after* we run do_initcalls() as such there are
> possible theoretical races even with request_firmware_nowait() -- as such that
> justifies even more the SmPL grammar rule to include even request_firmware_nowait()
> on probe / init as this patch has.
> 
> Anyway yeah that patch has good intentions but its wrong for a slew of reasons.
> The problem is the same as discussed with Bjorn recently. The solution
> discussed is that since only userspace knows when the *real* rootfs is ready
> (because of slew of reasons) the best we can do is have an event issued by
> userspace to inform us of that.

OK I have something I think we can build upon for this. Will send RFC.

  Luis

[toc] | [prev] | [next] | [standalone]

#1475511 — [RFC] fs: add userspace critical mounts event support

kernel_read_file_from_path() can try to read a file from
the system's filesystem. This is typically done for firmware
for instance, which lives in /lib/firmware. One issue with
this is that the kernel cannot know for sure when the real
final /lib/firmare/ is ready, and even if you use initramfs
drivers are currently initialized *first* prior to the initramfs
kicking off. During init we run through all init calls first
(do_initcalls()) and finally the initramfs is processed via
prepare_namespace():

do_basic_setup() {
   ...
   driver_init();
   ...
   do_initcalls();
   ...
}

kernel_init_freeable() {
   ...
   do_basic_setup();
   ...
   if (sys_access((const char __user *) ramdisk_execute_command, 0) != 0) {
      ramdisk_execute_command = NULL;
      prepare_namespace();
   }
}

This leaves a possible race between loading drivers and any uses
of kernel_read_file_from_path(). Because pivot_root() can be used,
this allows userspace further possibilities in terms of defining
when a kernel critical filesystem should be ready by.

We define kernel critical filesystems as filesystems which the
kernel needs for kernel_read_file_from_path(). Since only userspace
can know when kernel critical filesystems are mounted and ready,
let userspace notify the kernel of this, and enable a new kernel
configuration which lets the kernel wait for this event before
enabling reads from kernel_read_file_from_path().

A default timeout of 10s is used for now. You can override this
through the kernel-parameters using critical_mounts_timeout_ms=T
where T is in ms. cat /sys/kernel/critical_mounts_timeout_ms the
current system value.

When userspace is ready it can simply:

  echo 1 > /sys/kernel/critical_mounts_ready

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---

Note, this still leaves the puzzle of the fact that initramfs may carry
some firmware, and some drivers may be OK in using firmware from there,
the wait stuff would just get in the way. To address this I think can
perhaps instead check *one* for the file, and if its present immediately
give it back, we'd only resort to the wait in cases of failure.

Another thing -- other than firmware we have:

security/integrity/ima/ima_fs.c:        rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY);
sound/oss/sound_firmware.h:     err = kernel_read_file_from_path((char *)fn, (void **)fp, &size,

What paths are these? So we can document the current uses in the Kconfig
at least.

Thoughts ?

 Documentation/kernel-parameters.txt |  6 +++
 drivers/base/Kconfig                | 48 +++++++++++++++++++++++
 fs/exec.c                           |  3 ++
 include/linux/fs.h                  |  8 ++++
 kernel/ksysfs.c                     | 77 +++++++++++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 8ccacc44622a..1af89faa9fc9 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -849,6 +849,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			It will be ignored when crashkernel=X,high is not used
 			or memory reserved is below 4G.
 
+	critical_mounts_timeout_ms=T	[KNL] timeout in ms
+			Format: <integer>
+			Use this to override the kernel's default timeout for
+			waiting for critical system mount points to become
+			available.
+
 	cryptomgr.notests
                         [KNL] Disable crypto self-tests
 
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 12b4f5551501..21576c0a4898 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -25,6 +25,54 @@ config UEVENT_HELPER_PATH
 	  via /proc/sys/kernel/hotplug or via /sys/kernel/uevent_helper
 	  later at runtime.
 
+config CRITICAL_MOUNTS_WAIT
+	bool "Enable waiting for critical-filesystems-ready notification"
+	default n
+	help
+	  Kernel subsystems and device drivers often need to read files
+	  from the filesystem, however in doing this races are possible at
+	  bootup -- the subsystem requesting the file might look for it in /
+	  early in boot, but if we haven't yet mounted the real root
+	  filesystem we'll just tell the subsystem the file is not present and
+	  it will fail. Furthermore what path to the filesystem is used varies
+	  depending on the subsystem. To help the kernel we provide the option
+	  to let the kernel wait for all critical filesystems to mounted and
+	  ready before letting the kernel start trying to read files from the
+	  systems' filesystem. Since pivot_root() can be used and therefore a
+	  system might be configured to change its / filesystem at bootup as
+	  many times as it wishes, only userspace can realy know exactly when
+	  all critical filesystems are ready. Enabling this lets userspace
+	  communicate to the kernel when all critical filesystems are ready.
+
+	  What are the critical filesystems are obviously system specific, but
+	  examples of some are:
+
+	    o /lib/firmware/
+	    o /etc/XXX/
+
+	  If you enable this you must have a userspace init script or tool
+	  which will vet to ensure all critical filesystems are ready, once
+	  they are all ready it will inform the kenrel by setting the file
+	  /sys/kernel/critical_mounts_ready to 1.
+
+	  The kernel will wait by default 10 seconds for the event, if the
+	  the timeout is reached, it will proceed to just try to enable
+	  reading of the files from the kernel but warn.
+
+	  If not sure say "no" for now. You need proper userpace implementation
+	  for this.
+
+config CRITICAL_MOUNTS_WAIT_TIMEOUT
+	int "Timeout for critical-fs-reayd notification in miliseconds"
+	depends on CRITICAL_MOUNTS_WAIT
+	default 10000
+	help
+	  Defines the timeout for the kernel to wait for critical filesystems
+	  to be loaded. This if system specific as only the system will know
+	  exaclty when how long this typically takes. By default this is
+	  10 seconds. You can override at boot time by using the kernel
+	  parameter critical_mounts_timeout_ms.
+
 config DEVTMPFS
 	bool "Maintain a devtmpfs filesystem to mount at /dev"
 	help
diff --git a/fs/exec.c b/fs/exec.c
index 6fcfb3f7b137..0d46ad4aad11 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -57,6 +57,7 @@
 #include <linux/oom.h>
 #include <linux/compat.h>
 #include <linux/vmalloc.h>
+#include <linux/swait.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -949,6 +950,8 @@ int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
 	struct file *file;
 	int ret;
 
+	wait_for_critical_mounts(id);
+
 	if (!path || !*path)
 		return -EINVAL;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bd57feb7cf37..f59213ac8a8b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3202,4 +3202,12 @@ static inline bool dir_relax_shared(struct inode *inode)
 extern bool path_noexec(const struct path *path);
 extern void inode_nohighmem(struct inode *inode);
 
+#ifdef CONFIG_CRITICAL_MOUNTS_WAIT
+void wait_for_critical_mounts(enum kernel_read_file_id id);
+#else
+static inline void wait_for_critical_mounts(enum kernel_read_file_id id)
+{
+}
+#endif /* CONFIG_CRITICAL_MOUNTS_WAIT */
+
 #endif /* _LINUX_FS_H */
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index ee1bc1bb8feb..232af58d8760 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -21,6 +21,7 @@
 #include <linux/compiler.h>
 
 #include <linux/rcupdate.h>	/* rcu_expedited and rcu_normal */
+#include <linux/swait.h>
 
 #define KERNEL_ATTR_RO(_name) \
 static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
@@ -180,6 +181,78 @@ static ssize_t rcu_normal_store(struct kobject *kobj,
 KERNEL_ATTR_RW(rcu_normal);
 #endif /* #ifndef CONFIG_TINY_RCU */
 
+#ifdef CONFIG_CRITICAL_MOUNTS_WAIT
+static int are_critical_mounts_ready;
+
+static DECLARE_SWAIT_QUEUE_HEAD(critical_wq);
+static int critical_mounts_timeout_ms = CONFIG_CRITICAL_MOUNTS_WAIT_TIMEOUT;
+
+core_param(critical_mounts_timeout_ms, critical_mounts_timeout_ms, int, 0644);
+
+static bool critical_mounts_ready(void)
+{
+	return !!are_critical_mounts_ready;
+}
+
+
+static void __wait_for_critical_mounts(void)
+{
+	int ret;
+	struct swait_queue_head *wq = &critical_wq;
+
+	pr_debug("Waiting for critical filesystems...\n");
+	ret = swait_event_interruptible_timeout(*wq, critical_mounts_ready(),
+						msecs_to_jiffies(critical_mounts_timeout_ms));
+	if (ret > 0)
+		return;
+
+	WARN_ON(ret < 0);
+}
+static ssize_t critical_mounts_ready_show(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  char *buf)
+{
+	return sprintf(buf, "%d\n", critical_mounts_ready());
+}
+static ssize_t critical_mounts_ready_store(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   const char *buf, size_t count)
+{
+	if (kstrtoint(buf, 0, &are_critical_mounts_ready))
+		return -EINVAL;
+
+	return count;
+}
+KERNEL_ATTR_RW(critical_mounts_ready);
+
+static ssize_t critical_mounts_timeout_ms_show(struct kobject *kobj,
+					       struct kobj_attribute *attr,
+					       char *buf)
+{
+	return sprintf(buf, "%d\n", critical_mounts_timeout_ms);
+}
+KERNEL_ATTR_RO(critical_mounts_timeout_ms);
+
+void wait_for_critical_mounts(enum kernel_read_file_id id)
+{
+	switch (id) {
+	case READING_FIRMWARE:
+	case READING_FIRMWARE_PREALLOC_BUFFER:
+	case READING_POLICY:
+		if (!critical_mounts_ready()) {
+			pr_info("Waiting for critical filesystems...\n");
+			__wait_for_critical_mounts();
+		}
+		else
+			pr_info("All critical filesystems are ready!\n");
+		break;
+	default:
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(wait_for_critical_mounts);
+#endif /* CONFIG_CRITICAL_MOUNTS_WAIT */
+
 /*
  * Make /sys/kernel/notes give the raw contents of our kernel .notes section.
  */
@@ -225,6 +298,10 @@ static struct attribute * kernel_attrs[] = {
 	&rcu_expedited_attr.attr,
 	&rcu_normal_attr.attr,
 #endif
+#ifdef CONFIG_CRITICAL_MOUNTS_WAIT
+	&critical_mounts_ready_attr.attr,
+	&critical_mounts_timeout_ms_attr.attr,
+#endif
 	NULL
 };
 
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1475536 — Re: [RFC] fs: add userspace critical mounts event support

On Fri, Sep 2, 2016 at 5:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>
> Thoughts ?

I really think this is a horrible hack.

It's basically the kernel giving up, and relying on user space to give
a single flag, and it's broken nasty crap.  Worse, it's broken nasty
crap with a user interface, so we'll be stuck with it forever. Please
no.

What are the drivers that need this, and why can't those drivers just
be fixed to not do braindead things?

                    Linus

[toc] | [prev] | [next] | [standalone]

#1475538 — Re: [RFC] fs: add userspace critical mounts event support

On Fri, Sep 2, 2016 at 9:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Sep 2, 2016 at 5:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>
>> Thoughts ?
>
> I really think this is a horrible hack.
>
> It's basically the kernel giving up, and relying on user space to give
> a single flag, and it's broken nasty crap.  Worse, it's broken nasty
> crap with a user interface, so we'll be stuck with it forever. Please
> no.

I agree that interface is bad, but I do believe we need something like this...

>
> What are the drivers that need this, and why can't those drivers just
> be fixed to not do braindead things?

Like what? Some devices do need to have firmware loaded so we know
their capabilities, so we really can't push the firmware loading into
"open". If your touch controller for some reason decided to crap over
it's nvram and comes in bootloader mode it is nice for it to slurp in
config data or firmware so use does not have to trigger update
manually. And while the controller is in bootloader mode we can't
create input device because we do not know what capabilities to
declare.

These devices we want to probe asynchronously and simply tell firmware
loader to wait for firmware to become available. The problem we do not
know when to give up, since we do not know where the firmware might
be. But userspace knows and can tel us.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1475770 — Re: [RFC] fs: add userspace critical mounts event support

On Fri, Sep 02, 2016 at 09:41:18PM -0700, Linus Torvalds wrote:
> On Sep 2, 2016 9:20 PM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
> >
> > Like what? Some devices do need to have firmware loaded so we know
> > their capabilities, so we really can't push the firmware loading into
> > "open".
> 
> So you
> (a) document that

Document that device may come up half-broken? Not sure how that would
help end user.

> (b) make the driver only build as a module

Unfortunately module loading and availability of firmware is very
loosely coupled. Of course, if you only load modules from the same
partition that your firmware is on you can get away with it, but if some
of the modules are in initramfs and firmware is on final root fs then
it still does not work. And populating also initramfs with firmware that
might be used once in a 1000 boots is somewhat wasteful. That is not
talking about systems that do not wish to use modules for one reason or
another, or even more esoteric setups where non-essential for boot
firmware can be mounted later over nfs, etc, etc.

> (c) make sure the module and the firmware go together

I do not think it is always possible. Quite often it is though, at the
expense of increasing kernel/initramfs size.

> 
> End of problem.
> 
> Why make up random interfaces for crazy stuff?

Because we want a solution that works well for all cases, simple and
complex. This includes allowing drivers to be built into the kernel but
allow them waiting for additional data (config/firmware) that may become
available later in the game. We just need to be able to tell them when
it does not make sense to wait anymore as the data they want is not
coming, and do it more reliably then simply declaring 10 or 30 or 300
seconds time out.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1475771 — Re: [RFC] fs: add userspace critical mounts event support

On Sat, Sep 3, 2016 at 11:01 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> Unfortunately module loading and availability of firmware is very
>> loosely coupled.
>
> The whole "let's add a new magical proc entry to say that all
> filesystems are mounted" is all about the user space coupling them.

I will be first to say that the proposed *implementation* is nowhere
near what should be accepted. I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace
could unlock these "conditions". One of them might be "firmware
available".

>
> I'm just saying that if user space must know about when firmware is
> ready to be loaded anyway, just do it rigth. Not with some hacky "you
> can now do random things" flag. But by having user space actually say
> "put this module together with this firmware".
>
> If you just put the two pieces together, then the module "will just work".
>
> And quite frankly, that sounds like a much better maintenance practice
> anyway. If some module doesn't work without the firmware, then dammit,
> the two *should* go together. Putting them in two different places
> would be *INSANE*.

Quite often it does until it does not. Most of the touch controllers
work just fine until some event (abrupt cutting of power for example)
where nvram gets corrupted and they come up in bootloader mode. It is
just an example.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1477838 — Re: [RFC] fs: add userspace critical mounts event support

On Sat, Sep 03, 2016 at 11:10:02AM -0700, Dmitry Torokhov wrote:
> On Sat, Sep 3, 2016 at 11:01 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> Unfortunately module loading and availability of firmware is very
> >> loosely coupled.
> >
> > The whole "let's add a new magical proc entry 

To be fair it was using a generic sysfs entry.

> > to say that all
> > filesystems are mounted" is all about the user space coupling them.
> 
> I was thinking if we kernel could post "conditions" (maybe simple strings)
> that it waits for, and userspace could unlock these "conditions". One of them
> might be "firmware available".

Dmitry, you seem to be suggesting a generic kernel-userspace "registry" for
intertwined dependencies that the kernel needs and only userspace can provide,
is that right?

If so it sounds overly complicated to resolve this. Do we have other uses
outside of kernel_read_file_from_path() you had in mind for this?

> > I'm just saying that if user space must know about when firmware is
> > ready to be loaded anyway, just do it right. Not with some hacky "you
> > can now do random things" flag.

Note, this isn't just about firmware since we now have a generic API to read
files from the kernel, so kernel_read_file_from_path(). Firmware is now just
*one* user case. Also a complexity here is this is not just for modules but
also for built-in drivers as well. And you want the option to update the
files without the driver.

The proposed solution provides a generic broad syfs entry for letting userspace
inform the kernel when files from the filesystems where it would typically read
from (I'm calling them critical filesystems for lack of a better term) can be
accessible. Something more specific requires a bit more thought given this is
not anymore about just firmware, must also address built-in drivers, and allow
for updates.

> >  But by having user space actually say
> > "put this module together with this firmware".

Keep in mind this isn't about just firmware anymore, and we have to consider
built-in drivers as well. But if we were to just consider firmware... for the
sake of following with examples.

How would this registry work?

We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or
something like it to help annotate the the driver was only functional with the
firmware, punt things to kmod to deal with the requirements.  kmod would only
load the driver if the firmware is present as well, otherwise it could just
return a new -ENOFIRMWARE and try to defer module loading for a later time, it
would load the driver once and if the firmware becomes available... This all
seems rather hacky.

For built-in drivers.. the vmlinux would have the associated firmware reqs, it
would still need a way to let userspace know when such firmware is ready. What
kernel <-> userspace API would we want to use for this ? Or as you note we
could just prevent such drivers to be built-in. I'd be happy with this, however
I don't think the distributions using non-modular kernels will be.

Now whatever we come up with recall this isn't just about firmware anymore.
Which is why I ended up bundling all these requirements up into one generic
"kernel reads file from filesystem" requirement. I can see the syfs approach
being considered hacky -- but I gladly welcome an actual alternative
suggestion.

> > If you just put the two pieces together, then the module "will just work".
> >
> > And quite frankly, that sounds like a much better maintenance practice
> > anyway. If some module doesn't work without the firmware, then dammit,
> > the two *should* go together. Putting them in two different places
> > would be *INSANE*.
> 
> Quite often it does until it does not. Most of the touch controllers
> work just fine until some event (abrupt cutting of power for example)
> where nvram gets corrupted and they come up in bootloader mode. It is
> just an example.

You want to also opt-in for updates, you don't want to require re-building
a driver for a firmware fix, for instance.

 Luis

[toc] | [prev] | [next] | [standalone]

#1477853 — Re: [RFC] fs: add userspace critical mounts event support

On Tue 06 Sep 14:52 PDT 2016, Luis R. Rodriguez wrote:

> We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or
> something like it to help annotate the the driver was only functional with the
> firmware, punt things to kmod to deal with the requirements.

That implies that a single driver will only use a single version of the
firmware. There are cases where we want a single driver to load firmware
depending on e.g. hardware revisions, or previous firmware version and
there are cases where we want to load firmware based on requested
use-cases.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1477882 — Re: [RFC] fs: add userspace critical mounts event support

On Tue, Sep 06, 2016 at 03:28:47PM -0700, Bjorn Andersson wrote:
> On Tue 06 Sep 14:52 PDT 2016, Luis R. Rodriguez wrote:
> 
> > We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or
> > something like it to help annotate the the driver was only functional with the
> > firmware, punt things to kmod to deal with the requirements.
> 
> That implies that a single driver will only use a single version of the
> firmware. 

Today firmware requests are done manually by the driver, in the future it should
be possible to specify just an array of firmwares and on that list specify
which firmware is optional, and perhaps if you need at last one. Then you treat
optional firmware upgrades as optional -- and only treat fatal conditions as
such. Today drivers manage all this on their own.  This is something we can
later do as we have the flexible firmware API in place, but for now -- you are
right. There is no clear way to extract the semantics of what firmware
requirements really are in an easy way.

> There are cases where we want a single driver to load firmware
> depending on e.g. hardware revisions,

Dynamic firmware names -- indeed. Good point.

> or previous firmware version and
> there are cases where we want to load firmware based on requested
> use-cases.

True.

  Luis

[toc] | [prev] | [next] | [standalone]

#1475773 — Re: [RFC] fs: add userspace critical mounts event support

On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Unfortunately module loading and availability of firmware is very
> loosely coupled.

The whole "let's add a new magical proc entry to say that all
filesystems are mounted" is all about the user space coupling them.

I'm just saying that if user space must know about when firmware is
ready to be loaded anyway, just do it rigth. Not with some hacky "you
can now do random things" flag. But by having user space actually say
"put this module together with this firmware".

If you just put the two pieces together, then the module "will just work".

And quite frankly, that sounds like a much better maintenance practice
anyway. If some module doesn't work without the firmware, then dammit,
the two *should* go together. Putting them in two different places
would be *INSANE*.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1477704 — Re: [RFC] fs: add userspace critical mounts event support

On Fri 02 Sep 21:11 PDT 2016, Linus Torvalds wrote:

Linus, I reversed the order of your questions/answers to fit my answer
better.

> On Fri, Sep 2, 2016 at 5:20 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >
> > Thoughts ?
> What are the drivers that need this, and why can't those drivers just
> be fixed to not do braindead things?
> 

I have several cases where remoteproc drivers are used boot DSPs upon
boot of the device, but the firmware files are way too big for being
stored in initramfs and all consumers of the provided services are
(semi-) probable as the remote processor is booted.

I.e. we need some way to figure out when these files become available so
we can bring these remote processors up.

> It's basically the kernel giving up, and relying on user space to give
> a single flag, and it's broken nasty crap.  Worse, it's broken nasty
> crap with a user interface, so we'll be stuck with it forever. Please
> no.
> 

There are several cases where there granularity of a single flag is not
enough and we do already have a working mechanism for relying on user
space to inform the kernel that firmware is available:
CONFIG_FW_LOADER_USER_HELPER_FALLBACK

Another available solution is, as you say, using kernel modules.  But I
really do not like the deployment issues that comes with kernel modules
during development. (The firmware and remoteproc driver normally does
not have the same flow through a development process)

> 
> I really think this is a horrible hack.
> 

I agree, but that said, I would appreciate a automagical mechanism that
would relieve user space from having to signal to the kernel that the
firmware partition has been mounted.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1477741 — Re: [RFC] fs: add userspace critical mounts event support

On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Linus, I reversed the order of your questions/answers to fit my answer
> better.

Nobody has actually answered the "why don't we just tie the firmware
and module together" question.

Really. If the driver doesn't work without the firmware, then why the
hell is it separated from it in the first place?

The hack is a hack, and it just sounds *stupid*.

               Linus

[toc] | [prev] | [next] | [standalone]

#1477827 — Re: [RFC] fs: add userspace critical mounts event support

On Tue 06 Sep 11:32 PDT 2016, Linus Torvalds wrote:

> On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > Linus, I reversed the order of your questions/answers to fit my answer
> > better.
> 
> Nobody has actually answered the "why don't we just tie the firmware
> and module together" question.
> 

The answer to this depends on the details of the suggestion; but
generally there's a much stronger bond between the kernel and the driver
than between the driver and the firmware in my cases.

E.g. we have a single remoteproc driver loading and controlling the
Hexagon DSP found in several Qualcomm platforms, so a single kernel
binary could (practically) load hundreds of variants of the firmware.

Both the kernel binary and the firmware in this example are side-loaded
onto the device during development - independently of each other, as
they are developed by different teams (or maybe even different
companies).

I assume that you're not suggesting to actually tie the module together,
as that would be practically difficult and a waste of resources.

Which leaves us with the suggestion that we should store the kernel
module with the firmware file, which is just infeasible from a few
practical reasons - again mostly related to the development flow and how
the files are contained on the devices.

> Really. If the driver doesn't work without the firmware, then why the
> hell is it separated from it in the first place?
> 

In several cases we have a single remoteproc driver controlling several
different co-processors. Further more with the aspiration of being able
to run the same kernel binary (including modules) on more than one
product this is simply not feasible.

As I said above, beyond development there are hundreds of variants of
these firmware files in products - each weighting in at 10-50MB.

The firmware loading part (remoteproc) doesn't care about these
differences and the functional drivers attaching to the services
provided by the firmware can handle the differences between them.

> The hack is a hack, and it just sounds *stupid*.
> 

This I totally agree with.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1477843 — Re: [RFC] fs: add userspace critical mounts event support

On Tue, Sep 6, 2016 at 2:11 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 06 Sep 11:32 PDT 2016, Linus Torvalds wrote:
>
>> On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
>> Nobody has actually answered the "why don't we just tie the firmware
>> and module together" question.
>
> The answer to this depends on the details of the suggestion; but
> generally there's a much stronger bond between the kernel and the driver
> than between the driver and the firmware in my cases.

I call BS.

Let me be very clear. I'm not applying that shit-for-brains stupid
patch, and will not be pulling it unless somebody tricks me into it.

Because all these arguments make no sense at all.

If the driver doesn't work without the firmware, then anybody who
distributes the driver binary without a firmware is just
*fundamentally* doing something insane. You may do it for
*development* purposes, but doing so for actual *use* would be
entirely pointless.

See my point? If a distribution is distributing the driver without the
firmware, then what the hell is the point of such a thing?

But even if you decide to do that for some odd reason, the patch is
still just stupid. Instead of adding some crazy infrastructure for
"now I've mounted everything", you could equally well just

 (a) make the driver fail the module load if it cannot find a firmware binary

 (b) after user space has mounted everything, just do "insmod -a"
again (or insmod just that driver).

See? The point is, this "generic" hacky interface is just stupid. It's
not adding any value. If you add user space "I'm ready now" points
anyway, you might as well make those points do the right thing and
just load the module that is now loadable.

We could mark such "late loading" modules explicitly if people want
to, so that you can automate the whole thing about delaying the
loading in user space.

At no point does it make sense to say "I have now mounted all the
important filesystems". Maybe the firmware is extracted later by user
space downloading it from the internet, and the module will then work
only after that point"./

This whole "I have mounted important filesystems" is just pure and
utter garbage. Stop pushing this shit.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1477872 — Re: [RFC] fs: add userspace critical mounts event support

On Tue, Sep 06, 2016 at 02:50:51PM -0700, Linus Torvalds wrote:
> On Tue, Sep 6, 2016 at 2:11 PM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Tue 06 Sep 11:32 PDT 2016, Linus Torvalds wrote:
> >
> >> On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
> >> Nobody has actually answered the "why don't we just tie the firmware
> >> and module together" question.
> >
> > The answer to this depends on the details of the suggestion; but
> > generally there's a much stronger bond between the kernel and the driver
> > than between the driver and the firmware in my cases.
> 
> I call BS.
> 
> Let me be very clear. I'm not applying that shit-for-brains stupid
> patch, and will not be pulling it unless somebody tricks me into it.

Great thanks. Please note that the only reason I proposed this was to
get the ball rolling in terms of finding a solution to the problem for why
some folks claim they *need* the firmware usermode helper. Granted, upstream
we only have 2 explicit users left, I'm told some out-of-tree users still
need and use the usermode helper.

They claim that without it there is the race between /lib/firmware being ready
and driver asking for the firmware. I was told there were quite a bit
of out-of-tree hacks to address this without using the usermode helper,
the goal of this patch was to create the discussion needed to a proper
resolution to this.

Given that upon inspection -- I've determined that even if you stuff the
firmware into initramfs you may still run into the race (the commit log of this
patch explains that) and that using initramfs was the alternative solution we
expected folks to use -- the only current deterministic sure way a driver can
depend on a firmware and avoid the race is to use CONFIG_EXTRA_FIRMWARE. This
is an issue.

> Because all these arguments make no sense at all.
> 
> If the driver doesn't work without the firmware, then anybody who
> distributes the driver binary without a firmware is just
> *fundamentally* doing something insane.

Some companies only redistribute firmware binaries to specific entities due to
avoid expanding to whom a patent grant is given to. That is, not every company
writing drivers or pushing out binary drivers is willing to dish out the
firmware as per the terms in linux-firmware.

> You may do it for *development* purposes, but doing so for actual *use* would
> be entirely pointless.

To be fair we haven't been explicit about our requirements for firmware_class
and expectations about what we expect for firmware for driver in Linux. This
has all been rather loose.

Furthermore the race issues we have found in firmware_class have only come about
through introspection, and I've been slowly documenting all this tribal knowledge.

> See my point? If a distribution is distributing the driver without the
> firmware, then what the hell is the point of such a thing?

Agreed.

> But even if you decide to do that for some odd reason, the patch is
> still just stupid. Instead of adding some crazy infrastructure for
> "now I've mounted everything", you could equally well just
> 
>  (a) make the driver fail the module load if it cannot find a firmware binary

Not sure if its clear but:

0) this is not just about firmware anymore
1) there is a race between using kernel_read_file_from_path() and
   having the filesystem that should be present on be ready;
2) Only *userspace* can know for sure what the real valid filesystem for
   files read from kernel_read_file_from_path() can be ready, so only userspace
   can tell the kernel if a read from kernel_read_file_from_path() is
   at a certain point in time valid.

>  (b) after user space has mounted everything, just do "insmod -a"
> again (or insmod just that driver).

I'm happy to document this as the resolution... I have a feeling some folks
will not like it. We also have built-in drivers to consider, what do we
advise for that? Keep in mind only CONFIG_EXTRA_FIRMWARE is deterministically
safe.

> See? The point is, this "generic" hacky interface is just stupid. It's
> not adding any value. If you add user space "I'm ready now" points
> anyway, you might as well make those points do the right thing and
> just load the module that is now loadable.

This is not about firmware anymore though, and we need to address built-in.

> We could mark such "late loading" modules explicitly if people want
> to, so that you can automate the whole thing about delaying the
> loading in user space.

Now *that* could actually help, for instance add another late
init call which would be called after initramfs and friends -- perhaps
way after prepare_namespace() -- by then we would ensure userspace has
all critical fs mounted. The problem though is we'd still need a way
for userspace to tell the kernel that all critical fs are mounted
as only userspace can know this for sure.

When is that done? How would the kernel know?

We do have PROBE_PREFER_ASYNCHRONOUS, but that does not provide any
guarantees over ready filesystems.

> At no point does it make sense to say "I have now mounted all the
> important filesystems". Maybe the firmware is extracted later by user
> space downloading it from the internet, and the module will then work
> only after that point"./
> 
> This whole "I have mounted important filesystems" is just pure and
> utter garbage. Stop pushing this shit.

Happy to do so, we do however need to document what we do expect users and
developers to do about this race for both drivers and built-in, keeping
in mind this is also for any users of kernel_read_file_from_path() as well.

  Luis

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web