Re: udev breakages - was: Re: Need of an ".async_probe()" type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()

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

Em 02-10-2012 19:23, Greg KH escreveu:
> On Tue, Oct 02, 2012 at 03:12:39PM -0700, Greg KH wrote:
>> On Tue, Oct 02, 2012 at 09:33:03AM -0700, Linus Torvalds wrote:
>>> I don't know where the problem started in udev, but the report I saw
>>> was that udev175 was fine, and udev182 was broken, and would deadlock
>>> if module_init() did a request_firmware(). That kind of nested
>>> behavior is absolutely *required* to work, in order to not cause
>>> idiotic problems for the kernel for no good reason.
>>>
>>> What kind of insane udev maintainership do we have? And can we fix it?
>>>
>>> Greg, I think you need to step up here too. You were the one who let
>>> udev go. If the new maintainers are causing problems, they need to be
>>> fixed some way.
>>
>> I've talked about this with Kay in the past (Plumbers conference I
>> think) and I thought he said it was all fixed in the latest version of
>> udev so there shouldn't be any problems anymore with this.
>>
>> Mauro, what version of udev are you using that is still showing this
>> issue?
>>
>> Kay, didn't you resolve this already?  If not, what was the reason why?
> 
> Hm, in digging through the udev tree, the only change I found was this
> one:
> 
> commit 39177382a4f92a834b568d6ae5d750eb2a5a86f9
> Author: Kay Sievers <kay@vrfy.org>
> Date:   Thu Jul 19 12:32:24 2012 +0200
> 
>     udev: firmware - do not cancel requests in the initrd
> 
> diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
> index 56dc8fc..de93d7b 100644
> --- a/src/udev/udev-builtin-firmware.c
> +++ b/src/udev/udev-builtin-firmware.c
> @@ -129,7 +129,13 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>                                  err = -errno;
>                  } while (err == -ENOENT);
>                  rc = EXIT_FAILURE;
> -                set_loading(udev, loadpath, "-1");
> +                /*
> +                 * Do not cancel the request in the initrd, the real root might have
> +                 * the firmware file and the 'coldplug' run in the real root will find
> +                 * this pending request and fulfill or cancel it.
> +                 * */
> +                if (!in_initrd())
> +                        set_loading(udev, loadpath, "-1");
>                  goto exit;
>          }
>  
> 
> which went into udev release 187 which I think corresponds to the place
> when people started having problems, right Mauro?

I'm using here udev-182. 

> If so, Mauro, is the solution just putting the firmware into the initrd?

I don't think that putting firmware on initrd is something that we want to
for media drivers. None of the webcam drivers currently need a firmware;
those are required on more complex devices (typically digital TV ones).

While there are a number of PCI devices that require firmware, in practice,
we're seeing more people using USB devices. IMO, it doesn't make any sense
that a hot-pluggable USB device to require a firmware at initrd/initramfs,
with is available only at boot time.

> No wait, it looks like this change was trying to fix the problem where
> firmware files were not in the initrd, so it would stick around for the
> real root to show up so that they could be loaded.
>
> So this looks like it was fixing firmware loading problems for people?

I'll run some tests with this patch applied and see what happens.

> Kay, am I just looking at the totally wrong place here, and this file in
> udev didn't have anything to do with the breakage?
> 
> thanks,
> 
> greg k-h
>