Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 25-06-2012 16:15, Antti Palosaari escreveu:
> On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
>> Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
>>> The firmware blob may not be available when the driver probes.
>>>
>>> Instead of blocking the whole kernel use request_firmware_nowait() and
>>> continue without firmware.
>>>
>>> This shouldn't be that bad on drx-k devices, as they all seem to have an
>>> internal firmware. So, only the firmware update will take a little longer
>>> to happen.
>>
>> While thinking on converting another DVB frontend driver, I just realized
>> that a patch like that won't work fine.
>>
>> As most of you know, there are _several_ I2C chips that don't tolerate any
>> usage of the I2C bus while a firmware is being loaded (I dunno if this is the
>> case of drx-k, but I won't doubt).
>>
>> The current approach makes the device probe() logic is serialized. So, there's
>> no chance that two different I2C drivers to try to access the bus at the same
>> time, if the bridge driver is properly implemented.
>>
>> However, now that firmware is loaded asynchronously, several other I2C drivers
>> may be trying to use the bus at the same time. So, events like IR (and CI) polling,
>> tuner get_status, etc can happen during a firmware transfer, causing the firmware
>> to not load properly.
>>
>> A fix for that will require to lock the firmware load I2C traffic into a single
>> transaction.
> 
> How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have.
> If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers
> that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral
> drivers can be left as those are currently because those are deferred by bus interface driver.

There are some issues with regards to it:

1) Currently, driver core doesn't allow a deferred probe. Drivers might implement
that, but they'll lie to the core that the driver were properly supported even when
probe fails. So, driver's core need an .async_probe() method;

2) The firmware load issue will still happen at resume. So, a lock like that is
still needed;

3) It can make some sense to async load the firmware for some drivers, especially
when the device detection may not be dependent on a firmware load.

I'm not fully convinced about (3), as the amount of changes are significant for
not much gain.

There's also another related issue: On devices where both bridge and sub-devices (like frontend)
needs firmware to be loaded, the load order is important at resume(), as the bridge
requires to get the firmware before the sub-devices.

That's said, IMO, the best approach is to do:

1) add support for asynchronous probe at device core, for devices that requires firmware
at probe(). The async_probe() will only be active if !usermodehelper_disabled.

2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface.

We can postpone or get rid of changing the I2C drivers to use request_firmware_async(),
if the request_firmware() core is not pedantic enough to complain and it is not gonna
to be deprecated.

Anyway, I'll open a thead c/c Greg KH (driver's core maintainer) with regards to the need
of an async_probe() callback.

Regards,
Mauro

> 
> regards
> Antti
>