Re: A workaround for request_firmware() stuck in module_init

[Ming Lei]

On Thu, Sep 6, 2012 at 12:30 AM, Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> wrote:
>> 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.

Sorry, I don't see anyone explained clearly why request_firmware()
can't be called inside module_init() in module case, so maybe it is
a bit early to say it is a fix on 'bug', :-)

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

Yes, it is alike with the patches from Takashi and me.

Also it is a kind of async probe, all these drivers may convert to
async probe to fix the problem for built-in case, I guess. If it is doable,
it may be a easier approach.

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

Sorry for misunderstanding your idea.

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

This kind of mechanism has been implemented already: request_firmware()
and release_firmware() will get and put a refcount.  And, the reference
count is associated with firmware name, and it should be so, IMO.

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

The firmware cache mechanism will keep the firmware during suspend/resume
cycle to address the problem.

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

For drivers, I understand request_firmware()/request_firmware_nowait()
and release_firmware() are enough. If many devices share one firmware,
there is only one firmware kept in memory for their requests if one holds
the firmware, and there is a refcount for it already, :-)

So I don't see why it is difficult to use request/release_firmware() inside
drivers, :-)

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

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