linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
To: Ming Lei <ming.lei@canonical.com>
Cc: Takashi Iwai <tiwai@suse.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rjw@sisk.pl>, Kay Sievers <kay@vrfy.org>,
	linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>
Subject: Re: A workaround for request_firmware() stuck in module_init
Date: Wed, 5 Sep 2012 17:30:11 +0100	[thread overview]
Message-ID: <20120905173011.7a1111f0@pyramind.ukuu.org.uk> (raw)
In-Reply-To: <CACVXFVO+OgvLoFMcDD0gcQnbzUW=SyR=wiSMyT-FtpjWW5sWdw@mail.gmail.com>

> 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

  reply	other threads:[~2012-09-05 16:25 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai
2012-09-04 15:52 ` Ming Lei
2012-09-04 16:10   ` Takashi Iwai
2012-09-05  1:15     ` Ming Lei
2012-09-05  5:53       ` Takashi Iwai
2012-09-05 11:32         ` Ming Lei
2012-09-05 13:03           ` Alan Cox
2012-09-05 14:01             ` Takashi Iwai
2012-09-05 15:22             ` Ming Lei
2012-09-05 16:30               ` Alan Cox [this message]
2012-09-05 21:08                 ` Benjamin Herrenschmidt
2012-09-05 23:18                   ` Alan Cox
2012-09-06  5:06                     ` Benjamin Herrenschmidt
2012-09-06  2:47                 ` Linus Torvalds
2012-09-06  4:12                 ` Ming Lei
2012-09-06 12:59                   ` Alan Cox
2012-09-06 15:38                     ` Ming Lei
2012-09-05 16:59             ` Lucas De Marchi
2012-09-05 17:09               ` Takashi Iwai
2012-09-05 16:51 ` Lucas De Marchi
2012-09-05 17:08   ` Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120905173011.7a1111f0@pyramind.ukuu.org.uk \
    --to=alan@lxorguk.ukuu.org.uk \
    --cc=benh@kernel.crashing.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=kay@vrfy.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ming.lei@canonical.com \
    --cc=rjw@sisk.pl \
    --cc=tiwai@suse.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).