Em 26-06-2012 18:27, Greg KH escreveu:

On Tue, Jun 26, 2012 at 06:21:02PM -0300, Mauro Carvalho Chehab wrote:

quoted

Em 26-06-2012 17:42, Greg KH escreveu:

quoted

On Tue, Jun 26, 2012 at 04:34:21PM -0300, Mauro Carvalho Chehab wrote:

quoted

New udev-182 seems to be buggy: even when usermode is enabled, it
insists on needing that probe would defer any firmware requests.
So, drivers with firmware need to defer probe for the first
driver's core request, otherwise an useless penalty of 30 seconds
happens, as udev will refuse to load any firmware.

Signed-off-by: Mauro Carvalho Chehab <redacted>
---

Note: this patch adds an ugly printk there, in order to allow testing it better.
This will be removed at the final version.

   drivers/media/video/em28xx/em28xx-cards.c |   39 +++++++++++++++++++++++++----
   1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 9229cd2..9a1c16c 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c

@@ -60,6 +60,8 @@ static unsigned int card[]     = {[0 ... (EM28XX_MAXBOARDS - 1)] = UNSET };
   module_param_array(card,  int, NULL, 0444);
   MODULE_PARM_DESC(card,     "card type");
   
+static bool is_em28xx_initialized;
+
   /* Bitmask marking allocated devices from 0 to EM28XX_MAXBOARDS - 1 */
   static unsigned long em28xx_devused;
   

@@ -3167,11 +3169,14 @@ static int em28xx_usb_probe(struct usb_interface *interface,
   	 * postponed, as udev may not be ready yet to honour firmware
   	 * load requests.
   	 */
+printk("em28xx: init = %d, userspace_is_disabled = %d, needs firmware = %d\n",
+	is_em28xx_initialized,
+	is_usermodehelp_disabled(), em28xx_boards[id->driver_info].needs_firmware);

debug code?

Yes, temporary debug code, for people @linux-media that might be interested
on testing the patch. It will be removed at the final version, of course.

quoted

Also, this doesn't seem wise.  probe() will be called and
is_em28xx_initialized will be 0 before it can be set if the device is
present when the module is loaded.  But, if a new device is added to the
system after probe() already runs, is_em28xx_initialized will be 1, yet
it isn't true for this new device.

Yes.

So you really want that?  That doesn't make any sense, sorry.

quoted

quoted

So this doesn't seem like a valid solution, even if you were wanting to
paper over a udev bug.

The problem with udev-182 is that it blocks firmware load while
mode_init() is happening. Only after the end of module_init(), udev will
handle request_firmware.

How does udev "know" that module_init() is completed, or even in the
picture at all?  Is this due to the change to use kmod?

Yes, I suspect so. It is probably running on a single thread mode. So,
while driver is loaading/probing, it is blocking request firmwares on
that driver.

And you really should be using the async firmware loading path, that
would solve this problem entirely, right?

As already explained, async firmware won't solve, as probe() cannot complete
without firmware. Em28xx is a good media device Citizen (and it might be possible
to use async firmware on this particular case), as the em28xx bridge
firmware is stored on a ROM memory inside the chip, but devices based
on Cypress FX2 CPU can only be probed after firmware load.

quoted

This is what happens before this patch series:

[    3.605783] tvp5150 0-005c: tvp5150am1 detected.
[    3.627674] tuner 0-0061: Tuner -1 found with type(s) Radio TV.
[    3.633695] xc2028 0-0061: creating new instance
[    3.638406] xc2028 0-0061: type set to XCeive xc2028/xc3028 tuner
[   64.422633] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
[   64.429090] em28xx #0: Config register raw data: 0xd0
[   64.434959] em28xx #0: AC97 vendor ID = 0xffffffff
[   64.440206] em28xx #0: AC97 features = 0x6a90
[   64.444654] em28xx #0: Empia 202 AC97 audio processor detected
[   64.607494] em28xx #0: v4l2 driver version 0.1.3
[  125.574760] xc2028 0-0061: Error: firmware xc3028-v27.fw not found.
[  125.645012] em28xx #0: V4L2 video device registered as video0
[  125.650851] em28xx #0: V4L2 VBI device registered as vbi0

The 60s delay is due to the bug (firmware doesn't load there just
because I didn't ask dracut to add it there).

Ok, if you ask for firmware that you don't have, stalling is normal.
Not good though, and one big reason you should switch to using the async
model of firmware loading (a long time ago I wanted that to be the only
model, but I lost that argument...)

quoted

After the patch series, the artificial delay introduced due to udev-182
goes away:

Wait, if the firmware isn't present, how could any delay go away?  Why
would it go away?

still confused,

Good point. I'll do more tests there, forcing dracut to store the firmware
for this device at initfs and see what happens with and without the patch.

Maybe there's something else happening here.

Thanks!
Mauro