On 08/02/17 14:16, Johan Hovold wrote: > 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. If you say so. Like I say it was surprising and from my perspective this is the first meaningful feedback from you on this. Let's not argue - it's acked. --- bod _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|