On Wed, Feb 08, 2017 at 02:05:15PM +0000, Bryan O'Donoghue wrote: > On 08/02/17 11:55, Johan Hovold wrote: > > On Wed, Feb 08, 2017 at 11:43:57AM +0000, Bryan O'Donoghue wrote: > >> On 08/02/17 09:43, Johan Hovold wrote: > >>> On Tue, Feb 07, 2017 at 02:31:04PM +0000, Bryan O'Donoghue wrote: > >>>> On 07/02/17 14:19, Johan Hovold wrote: > >>>>> On Mon, Jan 23, 2017 at 01:04:14PM +0100, Johan Hovold wrote: > >>>>>> Add a struct timer_list to struct gb_operation and use that to implement > >>>>>> generic operation timeouts. > >>>>>> > >>>>>> This simplifies the synchronous operation handling somewhat while also > >>>>>> providing a generic timeout mechanism that drivers can use for > >>>>>> asynchronous operations. > >>>>>> > >>>>>> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx> > >>>>> > >>>>> Greg, > >>>>> > >>>>> I believe you can apply this one now. This is the right way to implement > >>>>> operation timeouts, and it is independent of Bryan's patches converting > >>>>> loopback to use the new interface, which can be applied on top when they > >>>>> are ready. > > > >>> My approach using a single timer which either times out or is cancelled > >>> upon completion is about as efficient as this can get, and therefore > >>> also allows the current synchronous-operation implementation to be built > >>> on top of this generic mechanism. > >> > >> I'm just wondering what impact it has instead of > >> wait_event_interruptible() more/less overhead (I suspect more) - and I > >> reckoned you'd not be on for that change - so only made a change on the > >> asynchronous path. > > > > Yes, your approach with unconditional scheduling of a work struct for > > every operation was needlessly inefficient and I did not want that for > > the synchronous path as it would definitely be a regression (even if we > > would have gone with your patches as an intermediate step for the > > asynchronous case). > > Yes, I'm wondering what impact switching away from wait_for_completion* > in favour of add/cancel timer has for every operation. > > It's probably irrelevant. I think so, yes. There is still a timer hidden in wait_for_completion_timeout as mentioned below. > > My approach does not suffer from such overhead so can therefore be used > > as a generic mechanism (there's exactly one timer involved whether we > > use wait_event_interruptible_timer or not). > > Hmm yes, I was surprised that the first feedback from you on this was an > alternative patch but, since you feel strongly about doing it this way, > it's fine with me. Why would we go with a suboptimal approach, which in its present form still had issues that were unresolved? I told you from the start (August?) that I did not like the deferred- work approach and that a proper solution was planned for v2. Project got cancelled, but I now spent some time working out an implementation instead of spending more time on your patch. > Acked-by: Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx> Thanks. Johan _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|