[PATCH] staging: greybus: operation: add generic timeout support

[Bryan O'Donoghue]

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 at kernel.org>
>>>>>>>
>>>>>>> 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