Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Daniel Wagner]

On 09/08/2016 01:33 AM, Luis R. Rodriguez wrote:
> > The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
> > Fix freezer failures due to racy usermodehelper_is_disabled()").
>
> Thanks, this helps to give some perspective, I'll note that commit also refers
> to commit a144c6a (PM: Print a warning if firmware is requested when tasks are
> frozen) by Srivatsa a long time ago which added a warning print if a driver
> requested firmware when tasks are frozen. That commit log further clarifies
> that the issues is that some drivers erroneously use request_firmware() in
> their driver's ->resume() (or ->thaw(), or ->restore()) callbacks, it further
> clarifies that is not going to work unless the firmware has been built in.
> It did not explain *why* it wouldn't work though. But note it also mentioned
> how drivers that do have request_firmware() calls on resume stall resume --
> the reason for the stalling is the stupid usermode helper. The kernel now
> "fixed" these by returning an error in such cases, it does this by checking
> kernel user mode helper is disabled, this is why it would not work. But note
> that we should be disabling the usermode helper on suspend too, and likely
> the reason we never ran into an issue with the cache stuff is we would fail
> if the usermode helper was disabled anyway. This is a long winded way of
> saying that these commits further confirm removal of using the usermode helper
> from the firmware cache work for suspend/resume.

Okay, so let's finish this round of refactoring first. I prefer going in 
smaller steps and see if there are any regressions with those changes.


> > Signed-off-by: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> > Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> > Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/base/firmware_class.c | 52 +++++++++++++++++++++++--------------------
> >  1 file changed, 28 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 960f8f7..d4fee06 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware,
> >  				    unsigned int opt_flags, long timeout)
> >  {
> >  	struct firmware_priv *fw_priv;
> > +	int ret;
> > +
> > +	timeout = firmware_loading_timeout();
> > +	if (opt_flags & FW_OPT_NOWAIT) {
> > +		timeout = usermodehelper_read_lock_wait(timeout);
> > +		if (!timeout) {
> > +			dev_dbg(device, "firmware: %s loading timed out\n",
> > +				name);
> > +			return -EBUSY;
> > +		}
> > +	} else {
> > +		ret = usermodehelper_read_trylock();
> > +		if (WARN_ON(ret)) {
> > +			dev_err(device, "firmware: %s will not be loaded\n",
> > +				name);
> > +			return ret;
> > +		}
> > +	}
>
> fw_load_from_user_helper() no longer needs the timeout parameter then.

Updated the patch accordingly.

> Given this fact I'll chime in with some other, IMHO cosmetic things for
> this series. This however is the just the biggest issue for this series
> that I've found. That and testing this at run time didn't boot on my
> system, it could be an issue with linux-next next-20160907 booting
> on my system, I hadn't tried that yet. I did put your series through
> 0-day though and it went through fine though.

So far I have it tested with kvm. I'll give it a spin on real hardware. 
Good point.

> Since you will need a respin I'd appreciate if you can Cc Takashi,
> Bjorn, Daniel Vetter, and Arend van Spriel on these series as some
> of them have expressed interest in the umh stuff, so best to get wider
> review as well. While at it please Cc Rafael and Srivatsa.

Will do.

cheers,
daniel