linux-kernel - Re: A workaround for request_firmware() stuck in module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120905173011.7a1111f0@pyramind.ukuu.org.uk>
Date:	Wed, 5 Sep 2012 17:30:11 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Ming Lei <ming.lei@...onical.com>
Cc:	Takashi Iwai <tiwai@...e.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Kay Sievers <kay@...y.org>,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: A workaround for request_firmware() stuck in module_init

> Yes, deferring the load may fix the built in case, but which also
> introduces much work on changes of current drivers. In fact,
> there are few guys who complained the built in case.

It fixes the modular case too.

> The current complaint is from that udev may timeout request inside probe()
> when drivers are built as module. As pointed by Linus and Benjamin,
> it is better to fix it in udev, and looks not good to introduce great changes
> (such as Takashi's defer probe patch) in kernel to workaround the problem.

It's not about a workaround but about doing it properly for the long term
and doing it in one place. It's also not a "great change", its a small
change.

> Linus has said that he doesn't like to load firmware in probe(), but in some
> situation the drivers have to load firmware in its probe():

You don't want to load firmware in probe because of the locking problems
- you can trigger a load of another device on the same bus - the defer
  dodges that nicely
 
> In fact, it is better for drivers to load firmware just when user wants to use
> the device, and some drivers have already changed to load firmware in
> the open() callback.

For those devices sure but they are if anything a minority as far as I
can see.

> So looks loading firmware always before probe in driver core is
> against the above idea.

I never said "always"

> > firmware load off and only when the firmware had loaded would it call
> > ->probe with dev->firmware pointing at a refcounted firmware struct.
> 
> IMO, introduce refcount for the firmware doesn't make sense. The lifetime
> of firmware is completely different with lifetime of driver or device:

Exactly. Which is why the moment you have multiple devices you need
refcounts. It's also why the propsoal included a 

dev_discard_firmware()

so you an instance can drop its firmware reference if it doesn't need it
post probe.

>     - firmware needn't be kept in memory in the device/driver's lifetime, and
>       should be loaded just in need, and be released after downloading
>       it into device.

You broke suspend/resume for lots of devices.

>    - sometimes devices may disappear, but it is better to keep the
>      firmware in memory, for example, device may be disconnected
>      during resume but will come back later.

So the moment you have multiple instances of a device with their own
lifetime and you have the need to pin it sometimes you need a refcount

> As said above, ref/deref on probe/remove is not a good idea since
> we needn't to keep the firmware in memory during the whole device/driver
> lifetime.

Often you do. And in the case you don't you still have to deal with
multiple probes doing asynchronous loads of the same firmware so you want
to do matching and refcounting. It's pretty much essential.

The other big value apart from making it harder for driver writers to
screw up is that it takes some of the control and puts it in one place.
That means you can change it later easily not in each driver.

This is enabling for device drivers. With no intention of offending
driver authors the reality is that we should have driver interfaces that

- work the right way by default
- allow driver authors to do things themselves if they need to instead
  (ie opt out)
- are hard to f**k up

because we want it to just work.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/