Date:	Wed, 10 Aug 2016 20:52:54 +0200
From:	"Luis R. Rodriguez" <mcgrof@...nel.org>
To:	Daniel Wagner <wagi@...om.org>
Cc:	"Luis R. Rodriguez" <mcgrof@...nel.org>,
	Ming Lei <ming.lei@...onical.com>,
	linux-kernel@...r.kernel.org,
	Daniel Wagner <daniel.wagner@...-carit.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Takashi Iwai <tiwai@...e.de>,
	Kees Cook <keescook@...omium.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Julia Lawall <julia.lawall@...6.fr>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>,
	Miroslav Benes <mbenes@...e.cz>,
	Petr Mladek <pmladek@...e.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v1] firmware_class: encapsulate firmware loading status

On Wed, Aug 10, 2016 at 09:02:08AM +0200, Daniel Wagner wrote:
> 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@...-carit.de>
> > 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.

OK so even for RT this is not needed then. Is that right ?

If this is true there must be some gains of swait over old wait
API even if its not important for -rt, what are the selling points,
in summary ?

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

In that case, even when -rt, this is not needed. However the compartamentalizing
of usermode sleep crap to usermode helper only seems worthy endeavor and I
wonder if we can split the work in this patch to 2, one which splits the
stuff, and the other one that makes then the conversion from old wait to
the new swait. If this is possible there are three gains:

 o makes code easier to review
 o makes each change atomically justifiable
 o once you have only a conversion from old wait to new swait you can
   inspect the delta and try to write SmPL grammar to see if you can
   generalize the change, so grammar can do the change for other
   use cases. Of course, you'd need first to look for the IRQ context,
   and I wonder if that's possible. If there are however generic
   benefits of swait over old wait when complete_all() is used (is
   live patching one?) then this will be very handy.

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

OK thanks do we have a kselftest for swait ?

> > 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 ;)

OK thanks.

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

OK great. Thanks. In that case just the above observations are pending.

  Luis

Powered by blists - more mailing lists