Hi Greg, Em Mon, 25 Jun 2012 22:55:41 -0300 Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> escreveu: > Em 25-06-2012 19:33, Greg KH escreveu: > > On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote: > >> Greg, > >> > >> Basically, the recent changes at request_firmware() exposed an issue that > >> affects all media drivers that use firmware (64 drivers). > > > > What change was that? How did it break anything? > > https://bugzilla.redhat.com/show_bug.cgi?id=827538 > > Basically, userspace changes and some pm-related patches, ... ... As I pinged you back in June, the recent changes on udev made lots of regression on all media drivers that require firmware. The news is that the fixes are also causing more regressions... Btw, Linus also complained about that: https://lkml.org/lkml/2012/8/25/136 I basically tried a few different approaches, including deferred probe(), as you suggested, and request_firmware_async(), as Kay suggested. The deferred probe() doesn't work as-is, as it will re-probe the driver only if a new driver is modprobed. It should likely not be that hard to modify the drivers core to allow it to re-probe after returning the probe status to udev, but I failed to find a way of doing that (mostly because of the lack of time, as I'm being too busy those days). I tried a few other things, like using request_firmware_async() at a few drivers that require firmware. That caused a big regression on Kernel 3.6 that we're currently discussing how to fix, but basically, media devices are very complex, as they have lots of different blocks there: tuners, analog video demod, digital video demod, audio demod, etc. On most cases, each of those "extra" blocks are implemented via I2C, and the device initialization happens serialized. By letting an I2C driver to do asynchronous initialization, other dependent drivers broke, because there are some signals used by the other blocks that are dependent on a proper initialization of a block previously initialized. One of the specific case is a device very popular in Europe nowadays: the PCTV 520e. This device uses an em28xx USB bridge chip, a tda18271 i2c tuner and a drx-k DVB i2c demod (among other things). The DRX-K chipset requires a firmware; the other chipsets there don't. The drx-k driver is used in combination with several other drivers, like em28xx, az6027, xc5000, tda18271, tda18271dd, etc. On my tests, with the devices I have available (Terratec H5, H6, H7, HTC; Hauppauge HVR 530C), the usage of request_firmware_async() worked as expected. So, such patch that made DRX-K firmware load to be deferred were applied at Kernel 3.6 on this changeset: commit bd02dbcd008f92135b2c7a92b6 Author: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> Date: Thu Jun 21 09:36:38 2012 -0300 [media] drxk: change it to use request_firmware_nowait() 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. Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> It should be noticed that, for the DVB demodulation to work, the DVB demod should be attached with the tuner module, via this code snippet[1]: fe = drxk_attach(&pctv_520e_drxk_cfg, &i2c_adap); tda18271_attach(fe, i2c_tuner_address, &i2c_adap, &pctv_520e_tda18271_cfg); The evil are in the details: the DRX-K chip produces some clocks needed by the tuner, and it has some generic GPIO pins that allow to control random things, with are device-dependent. Also, there is an I2C switch at the board, used to control I2C access to the tuner: when the switch is off, the tuner is not visible at the I2C bus[2]. The tda18271 tuner driver needs to read one device register, in order to identify the chipset version, as there are a few different setups there, depending if the silicon is version 1 or 2. As this driver doesn't require any firmware, such read happens at driver's attach time, with should be fine. Well, before the patch: 1) drxk_attach() would synchronously read the firmware and initialize the device, setting the needed GPIO pins and clock signals; 2) tda18271_attach() would read the version ID register and initialize the device. However, after the patch, drxk_attach() does nothing but filling some internal data structures and defer the device init job to happen after the firmware load. So, when tda18271_attach() is called, there wasn't enough time yet to initialize the DRX-K device. The end result is that the tda18271 version is not identified and the I2C driver read fails: tda18271_read_regs: [5-0060|M] ERROR: i2c_transfer returned: -19 Unknown device (16) detected @ 5-0060, device not supported. Ok, there are lots of ways to fix it, but I suspect that we'll just push the problem to happen on some place else. The fact is that coordinating the initialization between the several parts of those devices is already complex enough when done serialized; doing it asynchronously will make the initialization code complex and won't bring any benefit, as the I2C bus will serialize the initialization anyway. Also, this is just one of the 495 media drivers. Several of them require firmware during probe() and are currently broken. Fixing all of them will likely require years of hard work. So, we need to do something else. For Kernel 3.6, we'll likely apply some quick hack. However, for 3.7 or 3.8, I think that the better is to revert changeset 177bc7dade38b5 and to stop with udev's insanity of requiring asynchronous firmware load during device driver initialization. If udev's developers are not willing to do that, we'll likely need to add something at the drivers core to trick udev for it to think that the modules got probed before the probe actually happens. What do you think? [1] This is a simplified version of the code: I removed a macro and the error logic from it, to make the code cleaner for reading. [2] The I2C switch is there to prevent I2C traffic to generate noise that might interfere at the tuner functions. Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
