On Saturday, July 21, 2012, Ming Lei wrote:
> On Sun, Jul 22, 2012 at 1:49 AM, Rafael J. Wysocki <rjw@xxxxxxx> 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.
If you don't use SYSTEM_SUSPEND, you could just leave your !SYSTEM_RUNNING
check above and use this one to cover the suspend/resume case.
However, this is all moot in the face of the Linus' objection.
> > } 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.
Well, not really.
> >
> >> 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,
No now look for the second time and then tell me what you got wrong. OK?
> and the state is just updated to RUNNING after suspend_finish.
No, it is not.
> > 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.
No, it doesn't.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
