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

[Takashi Iwai]

At Sun, 12 May 2013 21:32:39 +0800,
Ming Lei wrote:
> 
> On Sun, May 12, 2013 at 3:20 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > At Sat, 11 May 2013 21:01:27 +0800,
> > Ming Lei wrote:
> >>
> >> On Fri, May 10, 2013 at 5:32 PM, Takashi Iwai <tiwai@xxxxxxx> 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
> 
> OK, sorry for missing the fact which user-space application may have
> been killed before calling sys_reboot(), even though the fact depends on
> implementation of user space 'reboot'.

Heh, the existence of f/w loading via user-space itself already
depends on the user-space behavior.


> > 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.
> 
> A well-implemented driver should make sure the synchronization,
> looks it isn't related with firmware loading.

Right, and that's my point, too.  Why the f/w loader needs to care
the completion of unprotected f/w loading?


> >> 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
> 
> The driver itself can make sure any synchronization it needed.(
> probe vs. suspend, or open vs. suspend), and nothing to do with
> request_firmware().
> 
> > before suspend.  It protects only the udev reaction.  But, it doesn't
> > protect the action after it.
> 
> Also suspend is different with shutdown since usermodehelper lock
> is required to be held before freezing processes, so firmware loading
> should be completed before suspending.

Yes, but the question is why it must not be aborted but completed.
That's my primary question.

(Though, the abort would require some automatic restart of f/w load
after resume, as long as we keep the current interface.  So, this
would lead to more codes, and thus it's a clear drawback...  If the
argument were in that way, I'd agree :)  Just giving the time limit to
NOHOTPLUG case would ease the problems in practice.  It's not perfect,
but maybe we should go for a better perfection, the removal of such
mess (userspace involvement) from the whole f/w loader code.)


> >> > 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.
> 
> Yes, it is bad, but killing FW_ACTION_NOHOTPLUG may break dell's BIOS
> update utility, also how can we meet the demand of drivers which need private
> firmwares?

Feeding the firmware data can be implemented pretty easily in a
different way, e.g. just by providing a misc driver interface with
read().  But, the biggest problem is the compatibility.  That's why I
wrote "if allowed" in the earlier mail :)

Seriously, _if allowed_, we should go for this direction: killing
usermode f/w loading.  Then the removal of NOHOTPLUG would be the
first step.


thanks,

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