At Sat, 11 May 2013 21:01:27 +0800, Ming Lei wrote: > > On Fri, May 10, 2013 at 5:32 PM, Takashi Iwai <ti...@suse.de> wrote: > > At Fri, 10 May 2013 09:25:51 +0800, > >> > >> Anyway, if you want to force killing loader, please only kill these > >> FW_ACTION_NOHOTPLUG before suspend and reboot, and do > >> not touch FW_ACTION_HOTPLUG. Is it OK for you? > > > > Note that, as with my patch, only the shutdown case is handled. Let's > > not mixing up suspend and shutdown behavior for now. > > > > I see no reason why we need to wait at shutdown even for > > FW_ACTION_HOTPLUG. At that point, there should be no longer > > user-space action. It means that the driver shouldn't get any more > > data to finish the f/w loading upon that point. Thus the only > > For example, when one ethernet driver is requesting its firmware, > the loader is forced to be killed before shutdown, so the driver can't set the > WoL any more in its shutdown(), then users start to complain it is a > regression.
First off, this can't happen because, as mentioned earlier, the point we're discussing is already the moment after user-space is supposed to have been finished by init, i.e. there is already no udev. Thus the pending f/w request via usermode helper can never finish. So, it makes no sense to wait for any user-space action at that point. It just locks up. Secondly, if this would ever work, it's still just a luck. The protection is only about the usermode helper lock in the f/w loading code. This doesn't mean that the whole pending driver work would be finished. In other words, such a driver design is just broken. > That is why I suggest you to only kill requests of FW_ACTION_NOHOTPLUG. > > Also it isn't good to affect all drivers just for fixing two insane drivers. > > > possible consequence is the timeout, which is equivalent with the > > immediate abort of the operation. > > > > As mentioned earlier, the suspend behavior may be different. We want > > to retry the f/w load. Ideally, the f/w loader should abort and > > automatically retry after resume. In this case, also there is no big > > I don't think it is good idea since suspend() may need firmware to > change the device's power state. Considered that only two insane > drivers use FW_ACTION_NOHOTPLUG, we can kill the two before > suspend just like the case of shutdown. The same argument can be applied. The protection in f/w loading doesn't guarantee that the pending driver action will be finished before suspend. It protects only the udev reaction. But, it doesn't protect the action after it. > > reason to distinguish FW_ACTION_* types. Even for udev case, the > > action can be easily retried. > > > > Or, a cleaner solution would be to get rid of FW_ACTION_NOHOTPLUG > > completely. As Kay mentioned, this was a big mistake from the very > > It is still not a good idea, hackers may need FW_ACTION_NOHOTPLUG > to debug its driver with private firmwares, or examples like dell's BIOS > update. Oh no, it's a badly designed interface. It should be never used. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
