Date: Wed, 26 Oct 2016 10:20:31 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Nicholas Mc Guire <der.herr@hofr.at>
Cc: Peter Zijlstra <peterz@infradead.org>,
        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: <20161026172031.GB3989@dtor-ws>
References: <20161025223054.GA22917@dtor-ws>
 <20161026084535.GX3102@twins.programming.kicks-ass.net>
 <20161026121001.GA19692@dtor-ws>
 <20161026154213.GD3117@twins.programming.kicks-ass.net>
 <20161026171236.GA3831@osadl.at>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161026171236.GA3831@osadl.at>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3376
Lines: 75

On Wed, Oct 26, 2016 at 05:12:36PM +0000, Nicholas Mc Guire wrote:
> 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.

That is correct. In the use case above the firmware is loaded only once
upon binding device and driver and is not reloaded unless device is
completely unbound and then re-bound again. So we do not want suspend()
to wait after the firmware is loaded.

Note that I said we are using request_firmware_nowait() so that firmware
loading is done by the thread other than probe() thread and probe()
returns before firmware is loaded. Which means that suspend() (however
unlikely it is) may start executing before firmware loading completes,
and we want suspend to pause in this case. Once firmware is loaded
suspend should no longer wait.

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

The fact that we only load firmware once in the case I described.

Thanks.

-- 
Dmitry