Re: [PATCH v1] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

On 10.08.2016 03:10, Luis R. Rodriguez wrote:
> On Thu, Aug 04, 2016 at 02:27:16PM +0200, Daniel Wagner wrote:
>> From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>

>> In [0] we have a discussion on how the firmware_class API might be
>> changed to improve the current handling of firmware loading. This
>> patch was part of the orignal RFC which triggered the discussion.
>>
>> I think it is worth taking this one anyway. Maybe as I suggested it
>> could be part of the series from Luis.
> 
> I'm happy to queue it in on my end however your changes are a bit orthogonal
> as you help optimize us away from the usermode helper, I just compartamentalize
> that whole API away into a new one so this can go in separately. 

Okay.

> In terms of
> coordination -- sure order will help to get right so I can queue it in, in
> that sense. But we're not yet sure if sysdsata will go in first, and I'm happy
> for this to go in first as it does not conflict as its slightly orthogonal.
> 
> So order here does not interfere with my series -- lets just review this and
> its good lets let it go in.

Alright, let's do this then first.

> What you do is strip us further from the user mode helper and that
> is a good thing.
> 
> My review below.
> 
>> It cleans up the code base (okay my opinion) 
> 
> You do little to sell this. In fact, if this is OK, it does a good
> compartamentalization of a completion and a lock and implicates the
> wait stuff only onto the usermoder helper, indeed that's a win
> worth documenting on the commit log.

My selling skill has still a lot of room to grow. I'll add your points
to the commit message.

>> and removes the
>> complete_all() call which is problematic for -rt. complete_all() can
>> be used in any context including IRQ.
> 
> I see. But in this case the code in question should never run in IRQ context?

No, this code is not running in IRQ context. See below.

>> That could lead to unbounded
>> work in the IRQ context and that is a no go for -rt.
> 
> Is the fear of the call to be used in IRQ context or the waiters to
> somehow work in IRQ context somehow. The waiters were sleeping.. so
> that I think leaves only the call site of the complete_all() to worry
> about, but I can't see that happening in IRQ context. Please
> correct me if I'm wrong.

The only problem for -rt is if the complete_all() happens in IRQ
context. If that happens the waker wakes up all waiters in one go (in
IRQ). That leads to the 'unbounded work' which can't be preempted. There
is no further restriction for -rt on waiters or wakers.

>> So here the
>> attempt to reduce the number of complete_all() calls where possible.
> 
> OK so this is the real motivation.

Yes, this is more ore less a clean up work :)

>> I have left this argument out in the commit message because I was told '-rt'
>> arguments don't count for inclusion.
> 
> Sure, but I appreciate this explanation, thanks for that !
> 
> Can you provide a set of commits accepted upstream or on linux-next
> where such conversion has been done and accepted as well elsewhere
> in the kernel ?

Not so far. I have started to send out patches last week. It seems most
people are enjoying holiday.

https://lkml.org/lkml/2016/8/4/264
https://patchwork.kernel.org/project/linux-amlogic/list/?submitter=47731

> I know its just pending patches for review but this has me thinking, is
> the use of async functionality in the sysdata patches kosher for RT ?

I haven't looked into deeply but from what I saw there is no obvious
show stopper. I'll give it another look next week. I am AFK till next
week ;)


> 
>> cheers,
>> daniel
>>
>> [0] http://www.spinics.net/lists/linux-wireless/msg153005.html
>>
>> drivers/base/firmware_class.c | 154 ++++++++++++++++++++++++------------------
>>  1 file changed, 89 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 22d1760..33eb372 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/syscore_ops.h>
>>  #include <linux/reboot.h>
>>  #include <linux/security.h>
>> +#include <linux/swait.h>
>>  
>>  #include <generated/utsrelease.h>
>>  
>> @@ -91,19 +92,83 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>>  }
>>  #endif
>>  
>> +static int loading_timeout = 60;	/* In seconds */
>> +
>> +static inline long firmware_loading_timeout(void)
>> +{
>> +	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
>> +}
> 
> Seems like we can wrap the above loading_timeout and firmware_loading_timeout onto
> CONFIG_FW_LOADER_USER_HELPER -- or provide a helper that returns some
> static nonsense value that works for !CONFIG_FW_LOADER_USER_HELPER.
> 
> The move of the code above also makes this change harder to review.

Yeah, I moved it up because in one version I had a dependency to
firmware_loading_timeout. Hmm, maybe it can be removed or integrated a
bit more tightly.

[...]

> Other than that this looks nice so far. Can you please run the  tests
> 
> tools/testing/selftests/firmware/fw_filesystem.sh
> tools/testing/selftests/firmware/fw_userhelper.sh
> 
> against lib/test_firmware.c

I've done this. After this change, they are still passing. Obviously,
after patching those two tests. I came up with almost the identical
patches as you have in your queue.

cheers,
daniel