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. > 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. Acked-by: Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx> -- bod _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|