[PATCH 0/6] drivers/staging/greybus: add async operations

[Johan Hovold]

On Mon, Nov 21, 2016 at 05:22:13PM +0000, Bryan O'Donoghue wrote:
> McCOY: You've got him, Jim! You've got him where you want him.

Hey, I ended up watching this one last night. Funny coincidence.

> This patchset adds async operations to greybus-core. Rather than have
> different drivers do variations on the same theme of launching and timing
> out asynchronous operations, it makes sense instead to provide this
> functionality via greybus-core.

A couple of meta comments: You are not adding support for asynchronous
operations, but rather a generic mechanism for handling timeouts of
asynchronous operations. All greybus operations are asynchronous, but we
have convenience helpers for implementing synchronous operations on top
of those.

Please update the commit messages to reflect this.

Also, please use the common

	staging: greybus: operation: ...

prefix-pattern for your patches.

> This patchset makes it possible to launch asynchronous operations and have
> the completion callback for an operation indicate -ETIMEDOUT for timeouts
> without drivers having to manage that locally. This set doesn't convert the
> existing synchronous operations away from
> wait_for_completion_interruptible_timeout() since that would involve adding
> an extra work-queue item to each synchronous call, with no added benefit
> for synchronous operations. Synchronous operations can already detect
> -ETIMEDOUT by blocking on the synchronous send operations, asynchronous
> operations and the driver associated with those operations OTOH must
> implement a completion handler. The main improvement this patchset makes is
> to pass the -ETIMEDOUT completion status into that completion handler -
> hiding the setup/timeout of operations away from a driver-level and into a
> core-level set of logic.
> 
> Loopback as an example can have lots and lots of redundant code removed and
> given we previously had some in-flight effort to add asynchronous
> operations to SDIO it makes sense to move the generic types of things we
> need to do on the asynchronous path into greybus-core so that SDIO and
> other bundle drivers can reuse instead of reimplement asynchronous
> operations.
>
> Note: we had approximately three internal versions of this on Project-Ara.
> Here's the log for completeness.
> 
> V3-V4:
> Fix bug in loopback conversion - Mitch Tasman
> 
> V2-V3:
> remotes/ara-main/sandbox/bodonoghue/ara-main-current+async_op3
> 
> Drop patch #6 converting sync operations to new t/o structure. Johan had a
> concern about doing an in-depth analysis on that change and considering our
> compressed timelines now, this analysis won't happen. OTOH the new async
> API is the right thing for loopback and for potential later greybus
> users/implementers to reuse.
> 
> Although it wasn't part of the motive for this change - I observe slightly
> better performance than baseline with loopback tests with this change in
> place - which shouldn't be surprising since we have less aggregate context
> switching for operation timeouts.
> 
> V1-V2:
> Rename async_private => private - Greg

So this makes me worried about which version you ended up posting here,
since this field is still named async_private in this series.

> 
> gb_operation_request_send_async_timeout ->
> gb_operation_request_send_async() - Greg

Same for this for this, but here I think you should indeed keep the
timeout suffix.

Perhaps gb_operation_request_send_timeout() would be a better name,
which clearly separates it from gb_operation_request_send() which is
also used to send an asynchronous request (but with the driver being
responsible for managing any timeouts if needed).

I'll hold off on commenting on the rest until you confirm this is the
version you intended to send.

Thanks,
Johan