Re: [PATCH] firmware: fix async/manual firmware loading

[Bjorn Andersson]

On Thu 10 Nov 08:07 PST 2016, Luis R. Rodriguez wrote:

> On Thu, Nov 10, 2016 at 7:55 AM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Nov 09, 2016 at 09:39:21PM +0100, Luis R. Rodriguez wrote:
> >> On Sun, Oct 30, 2016 at 03:50:48PM +0100, Yves-Alexis Perez wrote:
> >> > From: Yves-Alexis Perez <corsac@xxxxxxxxxx>
> >> >
> >> > wait_for_completion_interruptible_timeout() return value is either
> >> > -ERESTARTSYS (in case it was interrupted), 0 (in case the timeout expired)
> >> > or the number of jiffies left until timeout. The return value is stored in
> >> > a long, but in _request_firmware_load() it's silently casted to an int,
> >> > which can overflow and give a negative value, indicating an error.
> >> >
> >> > Fix this by re-using the timeout variable and only set retval when it's
> >> > safe.
> >>
> >> Please amend the commit log as I noted in the previous response, and
> >> resend.
> >>
> >> > Signed-off-by: Yves-Alexis Perez <corsac@xxxxxxxxxx>
> >> > Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> >> > Cc: "Luis R. Rodriguez" <mcgrof@xxxxxxxxxx>
> >> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >>
> >> Other than the commit log you can add on you resend:
> >>
> >> Acked-by: Luis R. Rodriguez.
> >>
> >> Modulo I don't personally thing this this is sable material but I'll let
> >> Greg decide.
> >
> > Does it fix a regression?
> 

Yes

> Not that I am aware of, but if you consider the reported the developer
> then yes.
> 

I haven't verified that this particular use case actually worked before,
but this code works with lower timeout values (e.g. 60 in the fallback
case), so this looks isolated.

The bug was clearly introduced in v4.0 by:

68ff2a00dbf5 "firmware_loader: handle timeout via wait_for_completion_interruptible_timeout()"

So please add a Fixes: and

Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>

> > A reported issue with an older kernel version
> > that people have hit?
> 
> Definitely not.
> 
> >  It shouldn't be hard to figure out if a patch should be in stable or not...
> 
> Well with the only caveat now that I am suggesting we consider remove
> this logic completely as only 2 drivers were using it explicitly
> (second argument to request_firmware_nowait() set to false), it seems
> they had good reasons for it but ... this has been broken for ages and
> we seem to be happy to compartamentalize the UMH further, its unclear
> why we would want to expand and "fix" that instead of just removing
> crap that never worked. Thoughts?
> 

Please Luis, just stop your crusade on this code. You're grasping at
every straw of opportunity to get this code out of the kernel, but it
has not been broken for ages, it works just fine and it is ABI.

I'm very concerned about your mission to to "compartamentalize" this
code when you're so certain that it's "broken crap".

Regards,
Bjorn