Re: [PATCH 1/3] firmware: Avoid superfluous usermodehelper lock

[Takashi Iwai <tiwai@suse.de>]

At Sat, 11 May 2013 21:01:27 +0800,
Ming Lei wrote:
> 
> On Fri, May 10, 2013 at 5:32 PM, Takashi Iwai <tiwai@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