Date: Wed, 26 Oct 2016 17:12:36 +0000
From: Nicholas Mc Guire <der.herr@hofr.at>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>, Tejun Heo <tj@kernel.org>,
        computersforpeace@gmail.com, Ingo Molnar <mingo@kernel.org>
Subject: Re: complete_all and "forever" completions
Message-ID: <20161026171236.GA3831@osadl.at>
References: <20161025223054.GA22917@dtor-ws>
 <20161026084535.GX3102@twins.programming.kicks-ass.net>
 <20161026121001.GA19692@dtor-ws>
 <20161026154213.GD3117@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161026154213.GD3117@twins.programming.kicks-ass.net>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2492
Lines: 57

On Wed, Oct 26, 2016 at 05:42:13PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 26, 2016 at 05:10:01AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 26, 2016 at 10:45:35AM +0200, Peter Zijlstra wrote:
> > > On Tue, Oct 25, 2016 at 03:30:54PM -0700, Dmitry Torokhov wrote:
> 
> > > > Or do we need something like this in
> > > > do_wait_for_common():
> > > > 
> > > > 	if (x->done < UINT_MAX/2)
> > > > 		x->done--;
> > > 
> > > Depends a bit, do you really want this? Seems a bit daft to keep asking
> > > if its done already, seems like a waste of cycles to me.
> > > 
> > 
> > The use case I am after is:
> > 
> > 1. There is a device that is extremely dumb without firmware
> > 2. The driver uses request_firmware_nowait() and signals completion from
> > the firmware loading callback to let the reset of the driver know that
> > firmware has been done loading (successfully or otherwise)
> > 3. The driver uses wait_for_completion() in both remove() and suspend()
> > methods to wait for the firmware to finish loading.
> > 
> > While remove() happens at most once per device instance, suspend() may
> > happen unbound number of times (theoretically).
> > 
> > So the question is: should complete_all have this "forever" semantic
> > (IOW is documentation right about the intent) or do we need a new
> > primitive for this? From the cursory glance of users of complete_all()
> > all of them expect completion to stay in signalled state either forever,
> > or until they call reinit_completion() explicitly.
> 
> Nah, if we need this we should fix this one. Adding similar but slightly
> different primitives is a pain.
> 
> But I think you might need slightly more than the proposed change, the
> case I worry about is doing complete_all() when done != 0 (which isn't
> all that strange).

I do not quite see how that would work out for the Use-Case noted
if completion calls complete_all() once at the first firmware load
and at that point that completion object is "completed" forever then
the suspend() would not wait for any completion in this model.

Im probably simply misunderstanding something here - but a 
complete_all() seems to have been intended for a logically single
case of concurrent initialization but not for this use-case
I do think that this is a broken design - if the suspend()/resume()
needed to reload the firware then it also would need to wait for
the same and that would not occure without reinitializing the
completion object.

what am I missing ?

thx!
hofrat