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

From: Luis R. Rodriguez
Date: Thu Nov 10 2016 - 11:08:38 EST


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?

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

> 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?

Luis