Add flag to identify unidirectional operations.
Use convenience helper rather than open coding the identification when
suppressing response messages.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add flag field to struct gb_operation, and a first flag
GB_OPERATION_FLAG_INCOMING to identify incoming operations.
Pass an initial set of flags when allocating new operations, and use
these to identify incoming operations rather than overloading the
meaning of GB_OPERATION_TYPE_INVALID. This also allows us to set the
type for all operations during allocation.
Also add convenience helper to identify incoming operations.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Since commit 46ce118a2678 ("operation: refactor response handling")
sending operation responses is handled by greybus core so there is
currently no need to export the response helper.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add gfp mask argument to gb_operation_request_send in order to allow
submissions from atomic context.
Note that responses are currently always sent from non-atomic context as
incoming requests are processed in a work queue.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add gfp mask argument to gb_operation_create to allow operations to be
allocated in atomic context.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
The current host-controller message-cancellation implementation suffer
from a lifetime bug as dynamically allocated URBs would complete and be
deallocated while being unlinked as part of cancellation.
The current locking is also insufficient to prevent the related race
where the URB is deallocated before being unlinked.
Fix this by pushing the cancellation implementation from greybus core
down to the host-controller drivers, and replace the "cookie" pointer
with a hcpriv field that those drivers can use to maintain their state
with the required locking and reference counting in place.
Specifically the drivers need to acquire a reference to the URB under a
lock before calling usb_kill_urb as part of cancellation.
Note that this also removes the insufficient gb_message_mutex, which
also effectively prevented us from implementing support for submissions
from atomic context.
Instead the host-controller drivers must now explicitly make sure that
the pre-allocated URBs are not reused while cancellation is in progress.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
__init does not belong in a .h file, as it does not do anything there,
so remove all instances of it.
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Define init and exit functions to do one-time setup and teardown
of endo-related functionality. Currently they're place holders;
the next patch will populate them.
Note that we now call gb_operation_exit() from gb_init(), so
we can no longer mark that function with __exit.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
The top-level functions gb_init() and gb_exit() are tagged with
__init and __exit, respectively. These functions call a few
other functions that are similarly used only at initialization
and termination time. So mark those functions accordingly.
Note that, because gb_ap_exit() and gb_debugfs_cleanup()
are called by gb_init() in error paths, these functions
cannot be declared with the __exit attribute.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add helper to retrieve the maximum payload size for operations on a
specific connection.
Note that the helper is not inlined due to how the header files are
currently organised, but it is not expected to be called after a
connection has been initialised either.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Make sure we never end up with a host device with maximum buffer size
smaller than the shortest Greybus message.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Increase the maximum allowed buffer size to the full 16-bit range
supported by the protocol.
Note that host devices will generally use smaller buffers than the
maximum.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Host devices impose buffer-size constraints on Greybus core which are
taken into account when allocating messages.
Make sure to verify these constraints when the host device is allocated,
rather than when the first message is allocated.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
All protocols use the same value to distinguish between request and
response message types. This is a requirement.
Use GB_MESSAGE_TYPE_RESPONSE rather than GB_OPERATION_TYPE_RESPONSE
for the name of the flag used to distiguish between request and
response messages.
Get rid of the redundant response flag definitions that are
associated with specific protocols.
Describe the symbolic values as "operation types" rather than
"message types" where they are defined. The message type for a
request is the same as the operation type; the message type for a
response is the operation type OR'd with GB_MESSAGE_TYPE_RESPONSE.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add explicit pad bytes to the message header.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Make sure to allocate the message transfer-buffer separately from the
containing message structure to avoid data corruption on systems without
DMA-coherent caches.
The message structure contains state that is updated while the buffer
may be used for DMA, something which could lead to data corruption due
to cache-line sharing on some architectures.
Use the (renamed) message cache for the message structure itself and
allocate the buffer separately.
If the additional allocation is a concern, the message structures
could eventually be allocated as part of the operation structure.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Pass structured greybus messages rather than buffers to the host
drivers.
This will allow us to separate the transfer buffers from the message
structures.
Rename the related functions to reflect the new interface.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Move operation message-header to operation.h so that it can be used
by host drivers.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
If an operation is issued and the response never comes back,
gb_operation_timeout() cancels the operation but never wakes up the
waiter in gb_operation_request_send().
This patch removes the timeout workqueue and changes the request wait to
wait_for_completion_interruptible_timeout(), with timeout set to
OPERATION_TIMEOUT_DEFAULT.
Signed-off-by: Perry Hung <perry@leaflabs.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
I was asked to add a Linaro copyright to all Greybus source files
that anyone at Linaro has modified. This patch does that.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
The i2c protocol needs a way to indicate an i2c device doesn't exist
(which is not necessarily an error). Define GB_OP_NONEXISTENT to
indicate this, and updating the status<->errno mapping functions
accordingly.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
I've gone back and forth on this, but now that I'm looking at
asynchronous operations I know that the asynchronous callback will
want to know what type of operation it is handling, and right now
that's only available in the message header.
So record an operation's type in the operation structure, and use
it in a few spots where the header type was being used previously.
Pass the type to gb_operation_create_incoming() so it can fill
it in after the operation has been created.
Clean up the crap comments above the definition of the operation
structure.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
An asynchronous operation will want to know how big the response
message it receives is. Rather than require the sender to record
that information, expose a new field "payload_size" available to
the protocol code for this purpose.
An operation message consists of a header and a payload. The size
of the message can be derived from the size of the payload, so
record only the payload size and not the size of the whole message.
Reorder the fields in a message structure.
Update the description of the message header structure.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define a new function used to initiate a synchronous operation.
It sends the operation request message and doesn't return until
the response has been received and/or the operation's result
has been set.
This gets rid of the convention that a null callback pointer
signifies a synchronous operation.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
A connection has two lists of operations, and an operation is always
on one or the other of them. One of them contains the operations
that are currently "in flight".
We really don't expect to have very many in-flight operations on any
given connection (in fact, at the moment it's always exactly one).
So there's no significant performance benefit to keeping these in a
separate list. An in-flight operation can also be distinguished by
its errno field holding -EINPROGRESS.
Get rid of the pending list, and search all operations rather than
the pending list when looking up a response message's operation.
Rename gb_pending_operation_find() accordingly.
There's no longer any need to remove operations from the pending
list, and the insertion function no longer has anything to do with a
pending list. Just open code what was the insertion function (it
now has only to do with assigning the operation id).
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Use a symbolic constant (rather than just "0") to represent an
explicitly invalid operation type. The protocols have all reserved
that value for that purpose--this just makes it explicit in the core
code (since we now leverage its existence). Fix the code so it uses
the new symbolic value.
Define it in "operation.h" for all to see. Move the common
definition of the GB_OPERATION_TYPE_RESPONSE flag mask there
as well.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define a helper function gb_operation_response_alloc() and use it
to allocate the response buffer for outgoing operations in
gb_operation_create_common(.
Use it also in gb_operation_response_send() if the caller has not
allocated a response buffer.
Once a response buffer is allocated, fill in its result code and
send it.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define gb_operation_errno_map(), which maps an operation->errno
into the u8 value that represents it in the status field of an
operation response header. It'll be used in an upcoming patch.
Make gb_operation_status_map() a private function. It's not used
outside "operation.c" and I don't believe it ever should be.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Change gb_operation_response_send() so it takes an errno to assign
as an operation's result. This emphasizes that setting the result
should be the last thing done to an incoming operation before
sending its response.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define a new operation status GB_OP_MALFUNCTION, which will be used
to represent that something unexpected happened while handling an
operation. This is intended as an indication similar to a BUG()
call--whatever went wrong should *never* happen and because it's
unexpected we need to treat it as a fatal error.
Define another new operation status GB_OP_UNKNOWN_ERROR, which
will represent the case where an operation ended in error, but
the error was not recognized to be properly represented by one
of the other status values.
Renumber the operation status values, defining those that are
produced by core operations code ahead of those that are more
likely to come from operation handlers. Represent the values in
hexadecimal to emphasize that they must be represented with 8 bits.
The Use 0xff for GB_OP_MALFUNCTION instead of GB_OP_TIMEOUT; the
latter is special, but a malfunction is in a class by itself.
Reorder the cases in gb_operation_status_map() to match their
numeric order.
Map GB_OP_UNKNOWN_ERROR to -EIO in gb_operation_status_map(). Map
GB_OP_MALFUNCTION to -EILSEQ in gb_operation_status_map(), since
that value is used to represent an implementation error.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Hide the setting and getting of the operation result (stored in
operation->errno) behind a pair of accessor functions. Only the
operation core should be setting the result, but operations that
complete asynchronously will need access to the result so expose
the function that provides that.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Everyone keeps doing the same create/send/destroy logic all over the
place, so abstract that out to a simple function that can handle any
arbritrary request and/or response. This will let us save lots of
duplicated logic in the protocol drivers.
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Cancel the operation--not just the request message--if waiting
for a synchronous operation to complete is interrupted. Return
the operation result (which in that case will be -EINTR). The
cancelation will result in the normal operation completion path
being taken before returning.
Make gb_operation_wait() private, since it's only ever used for
for synchronous operations.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
If an operation times out, we need to cancel whatever message it
has in-flight. Do that instead of completing the operation, in the
timeout handler. When the in-flight request message is canceled its
completion function will lead to the proper completion of the
operation.
Change gb_operation_cancel() so it takes the errno that it's
supposed to assign as the result of the operation.
Note that we want to preserve the original -ETIMEDOUT error, so
don't overwrite the operation result value if it has already been
set.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Grab an extra reference to an operation before sending it. Drop
that reference at the end of its completion handling.
It turns out gb_operation_get() got deleted along the way, so this
re-introduces it. We're assuming we only get a reference when
there's at least one in existence so we don't need a semaphore to
protect it. Emphasize this by *not* returning a pointer to
the referenced operation.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Change the operation "receive workqueue" to be just the operation
"workqueue". All it does is complete an operation in non-atomic
context. This is all that's required for an outgoing request.
Similarly, ignore any notion that a response will only exist
for outgoing requests in gb_operation_cancel().
I'm doing this in the interest of getting the outgoing request path
verified without the encumbrance of any preconceptions about how
incoming requests need to work. When I finally turn my full
attenion to incoming requests I'll adapt the code as needed.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
An in-core operation structure tracks the progress of an operation.
Currently it holds a result field that was intended to take the
status value that arrives in an operation response message header.
But operations can fail for reasons other than that, and it's
inconvenient to try to represent those using the operation status
codes.
So change the operation->result field to be an int, and switch to
storing negative errno values in it. Rename it "errno" to make
it obvious how to interpret the value.
This patch makes another change, which simplifies the protocol drivers
a lot. It's being done as part of this patch because it affects all
the same code as the above change does. If desired I can split this
into two separate patches.
If a caller makes a synchronous gb_operation_request_send() request
(i.e., no callback function is supplied), and the operation request
and response messages were transferred successfully, have
gb_operation_request_send() return the result of the request (i.e.,
operation->errno). This allows the caller (or more generally, any
caller of gb_request_wait() to avoid having to look at this field
for every successful send.
Any caller that does an asynchronous request will of course need
to look at request->errno in the callback function to see the
result of the operation.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define greybus_data_sent(), which is a callback the host driver
makes when a buffer send request has completed. The main use for
this is to actively detect errors that can occur while sending.
(Something like this existed at one time and was removed.)
This also defines gb_hd_message_find(), which looks up a message
pointer associated with a buffer sent over a given host device.
This is now a pretty trival mapping.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Embed the buffer for message data into the message structure itself.
This allows us to use a single allocation for each message, and
more importantly will allow us to derive the message structure
describing a message from the buffer itself.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Have an operation's request and response messages be dynamically
allocated rather than embedded in an operation.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
The beginning of an operation message always contains the message
header. Rename the "buffer" field in an operation message to
be "header" to reflect this. Change its type as well.
The size of a message is the combined size of its header and its
payload. Rename the "buffer_size" field in a message header to
be simply "size", so message->size describes exactly that.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Define a common function that maps an operation status value to a
Linux negative errno.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
We (sort of) maintain the status of each message, but we shouldn't
need to. Right now we're not using it consistently in any case.
If a message fails to send, the caller will know to destroy the
operation that contained it.
If a message has been sent (i.e., handed to the host device layer)
it'll have a non-null cookie pointer.
If a does complete in error, we can update the status of the
operation that contains it. That isn't happening right now but
it will soon.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
We can derive the destination CPort id of any (outbound) message
from the connection it's operation is associated with. So we don't
need to store that information in every message.
As a result, we no longer need to record it at message initialization
time.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
The host device pointer doesn't have to be stored in every message.
It can be derived by following up the chain of pointers back to
the operation's connection.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
All the code has now been adjusted such that we can do away with the
old gbuf structure.
Three unused references remained in "greybus.h", so those are deleted.
Other than that most of the changes were done by simple global
substitution. The gb_message structure incorporates the fields that
were previously found its embedded gbuf structure. A few names have
been changed in the process:
gbuf->transfer_buffer message->buffer
gbuf->transfer_buffer_size message->buffer_size
gbuf->hcd_data; message->cookie
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Rework gb_connection_operation_recv() to be more oriented toward an
operation message, and to no longer use a struct gbuf local variable.
Rename it to be a little more wieldy.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
Embed the gbuf structures for operation messages into the message
structure rather than pointing to a dynamically allocated one.
Use a null gbuf->transfer_buffer pointer rather than a null gbuf
pointer to indicate an unused gbuf.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
A Greybus buffer (gbuf) is a generic buffer used for data transfer
over a Greybus interconnect. We only ever use gbufs in operations,
which always involve exactly two of them. The lifetime of a gbuf is
therefore directly connected to the lifetime of an operation, so
there no real need to manage gbufs separate from operations.
This patch begins the process of removing the gbuf abstraction, on
favor of a new data type, gb_message. The purpose of a gb_message
is--like a gbuf--to represent data to be transferred over Greybus.
However a gb_message is oriented toward the more restrictive way
we do Greybus transfers--as operation messages (either a request or
a response).
This patch simply defines the structure in its initial form, and
defines the request and response fields in a Greybus operation
structure as embedded instances of that type. The gbuf pointer
is defined within the gb_message structure, and as a result lots
of code needs to be tweaked to reference the request and response
gbufs as subfields of the request and response structures.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>
