linux-kernel - Re: [RFC] firmware load: defer request_firmware during early boot and resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 22 Jul 2012 04:09:26 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Oliver Neukum <oneukum@...e.de>
Subject: Re: [RFC] firmware load: defer request_firmware during early boot and resume

On Sun, Jul 22, 2012 at 1:49 AM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> On Friday, July 20, 2012, Ming Lei wrote:

>> +     if (system_state != SYSTEM_RUNNING)
>> +             return -EPROBE_DEFER;
>
> You can't just return here, _request_firmware_cleanup() has to be done still.

Good catch, thanks.

>
>> +
>>       ret = usermodehelper_read_trylock();
>
> So why don't you do this here, actually, like:
>
>         if (ret) {
>                 ret = -EPROBE_DEFER;

The problem is that the 'ret' is zero for early boot situation.

>         } else {
>
> instead of the WARN_ON()?
>
> Arguably, all cases in which usermodehelper_read_trylock() returns error
> codes will require deferred probing.

Yes, looks !SYSTEM_RUNNING has covered all the cases already.

>
>>       if (WARN_ON(ret)) {
>>               dev_err(device, "firmware: %s will not be loaded\n", name);
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index d0e4d99..a63d3171 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -244,7 +244,7 @@ extern struct device_driver *driver_find(const char *name,
>>                                        struct bus_type *bus);
>>  extern int driver_probe_done(void);
>>  extern void wait_for_device_probe(void);
>> -
>> +extern void driver_deferred_probe_trigger(void);
>>
>>  /* sysfs interface for exporting driver attributes */
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index e07f5e0..c8d74c6 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -378,6 +378,7 @@ extern enum system_states {
>>       SYSTEM_POWER_OFF,
>>       SYSTEM_RESTART,
>>       SYSTEM_SUSPEND_DISK,
>> +     SYSTEM_SUSPEND,
>
> First off, SYSTEM_SUSPEND_DISK is not used and probably should be removed.
> Second, both SYSTEM_SUSPEND and SYSTEM_SUSPEND_DISK would require the same
> kind of handling in the respect of device probing, so it is not sufficient
> to change the state in suspend_devices_and_enter().

suspend_devices_and_enter is used by hibernation too, and the state is just
updated to RUNNING after suspend_finish.

>
> Moreover, there are other situations in which tasks are frozen and
> request_firmware() won't work just as well, so I don't think using
> system_state for that is going to work in general.

Looks system_state becoming SYSTEM_RUNNING means all tasks has
been thawed completely.



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