| Age | Commit message (Collapse) | Author |
|---|
|
The invalid request type has been redefined as 0x7f.
Also remove the redundant redefinition of the invalid type from the
operation header.
Note that operation type 0x00 has been repurposed for the new generic
ping operation, which will be used to implement proper connection tear
down.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add helper functions for initiating unidirectional operations and
waiting for them to have been acknowledged as sent by the host device.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add support for initiating unidirectional operations, that is, sending
requests that do not require responses.
Note that we already handle incoming unidirectional operations.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Fix and update gb_operation_request_send() documentation and add
kernel-doc formatting.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Update the function name, which has gained a timeout suffix.
Also fix the kernel-doc formatting.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add support for operations with short responses.
So far we have assumed that the initiator of an operation always knows
the exact size of the expected response. This is however not always the
case and we've worked around this limitation in a couple of places by,
for example, first requesting the size of a resource before fetching the
actual data.
To avoid such workarounds and simplify our protocols, add a
short-response flag that can be set when allocating an operation. When
this flag is set on an operation, core will accept a response that is
shorter than the size of the (pre-allocated) response payload buffer.
For now, we update the response-message payload_size field to reflect
the actual length of the response received.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
As a preparatory clean up, add a temporary variable to point to the
response message header.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Use dev_err and dev_warn where appropriate and remove now unused pr_fmt
defines.
Testing Done:
Tested on DB3.5 with the generic bridge firmware on APB2.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add a new connection state ENABLED_TX in which only outgoing operations
are allowed.
This allows drivers to query the device during probe before allocating
their state containers without having to worry about racing incoming
requests.
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add a connection request-handler field to struct gb_connection that is
set when the connection is enabled.
This is a step towards removing the legacy protocol abstraction from
core, and will also be used to implement unidirectional connection
states (e.g. only outgoing operations are allowed).
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Because the width of our fields is already known, we can use %0Nx (for
hex) to print N bytes and %u (for unsigned decimal), instead of using %h
and %hh, which isn't that readable.
This patch makes following changes:
- s/%hx/%04x
- s/%04hx/%04x
- s/%hhx/%02x
- s/%02hhx/%02x
- s/%hhu/%u
- s/%hu/%u
- s/%x/%02x for u8 value (only at a single place)
Suggested-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Use the host-device device and connection name for error messages, as
the operation code can not assume that a connection has a bundle.
Note that the "initial" svc connection has never had a bundle.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Rename host-device structure gb_host_device to match our other
structures.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
We are removing struct device from the gb_connection structure in the
near future. The gb_bundle structure's struct device should be used as
a replacement.
This patch moves the operation code to use to use the bundle pointer
instead of the connection pointer when printing out error messages.
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Reviewed-by: Alex Elder <elder@linaro.org>
|
|
Clean up and improve error messages.
Demote a warning message to warning level.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Drop FIXME about sending responses in OOM situations.
If we fail to allocate an operation for an incoming request, we have
bigger problems than to worry about sending a response.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Remove legacy interface to "destroy" operations, which is now just a
wrapper for gb_operation_put.
The old interface name hides the fact that all operations are refcounted
and may live on even after having "destroyed" them.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
This patch hooks tracepoints for greybus messages
- trace_gb_message_send
- trace_gb_message_recv_request
- trace_gb_message_recv_response
- trace_gb_message_cancel_outgoing
- trace_gb_message_cancel_incoming
It provides standard tracepoints at
/sys/kernel/debug/tracing/events/greybus/gb_message_send
/sys/kernel/debug/tracing/events/greybus/gb_message_recv_response
/sys/kernel/debug/tracing/events/greybus/gb_message_recv_request
/sys/kernel/debug/tracing/events/greybus/gb_message_cancel_outgoing
/sys/kernel/debug/tracing/events/greybus/gb_message_cancel_incoming
Giving outputs like
gb_message_recv_request: greybus:1-1.1:0 op=0001 if_id=0000 hd_id=0000 l=2
gb_message_send: greybus:1-1.1:0 op=0001 if_id=0000 hd_id=0000 l=2
Similarly perf events can be viewed with standard perf tools e.g.
root@beaglebone:~# perf list 'greybus:*'
greybus:gb_message_send [Tracepoint event]
greybus:gb_message_recv_request [Tracepoint event]
greybus:gb_message_recv_response [Tracepoint event]
greybus:gb_message_cancel_outgoing [Tracepoint event]
greybus:gb_message_cancel_incoming [Tracepoint event]
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
The order of arguments is wrong and that shows up as a warning only on
64 bit machines.
Fixes: cb0ef0c019ab ("operation: print message type on errors")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
This can be very useful debug information, print it.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add dedicated bound work queue for operation completions and use the
connection work queues for incoming requests only.
There is no need to keep responses ordered internally or with respect to
requests. Instead allow operations to complete as soon as possible when
a response arrives (or the operation is cancelled).
Note that this also allows synchronous requests to be submitted from
request handlers as responses will no longer be blocked on the same
single-threaded work queue. Similarly, operations can now also be
cancelled from a request handler.
Tested-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Replace the global operation work queue with per-connection work queues.
There is no need to keep operations strictly ordered across connections,
something which only adds unnecessary latency.
Tested-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add connection variable to greybus_message_sent.
This will be put to more use by a follow-up up patch.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Tested-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make the operation work queue single threaded.
The operation work queue was meant to be single threaded, but due to a
missing flag instead allowed one active task per CPU, something which
could lead to requests being processed out of order on SMP systems.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Tested-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
The recently added GFP-flags argument to gb_message_send was never used.
Fixes: 9218fac2a24d ("operation: allow atomic request submissions")
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Response allocation also needs a GFP-flags argument as a response is
allocated as part of an outgoing operation.
Fixes: 9aa174d202e5 ("operation: allow atomic operation allocations")
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add gb_operation_sync_timeout convenience function, which allows drivers
to configure the operation timeout.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add new interface gb_operation_request_send_sync_timeout, which allows
drivers to define a custom operation timeout instead of the default
one-second timeout.
The timeout is expected to depend on protocol and operation and
therefore needs to be configurable.
Note that that a timeout of zero is used to wait indefinitely.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Split incoming and outgoing operation-cancellation helpers.
Incoming operations are only cancelled as part of connection tear down
and is specifically not needed in the driver API.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure the request handler has submitted the response before
cancelling it during operation cancellation.
This prevents cancelling not-yet-submitted messages. It currently also
avoids us ending up with an active message on a stalled connection (e.g.
due to E2EFC).
Note that the call to gb_operation_result_set() is now redundant but is
kept as a precaution to guarantee that a response has indeed been
allocated as part of response submission.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure to fully initialise the operation before adding it to the
active list when sending a request.
The operation should be fully initialised before adding it to the active
list to avoid racing with operation look up when receiving a response,
something which could potentially lead to a match against some earlier
(or intermediate) value of the id field.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Fix connection tear down, which was done in an unsafe way that could
result in use-after-free as the per-connection list of operations was
iterated without any locking or refcounting.
Specifically, the operations list was iterated without holding any locks or
operation refcounts even though operations were being both removed from
the list and deallocated during per-operation cancellation. Any
operation completing during tear down could also cause corruption.
Change the per-connection operation list to only contain active
operations and use the recently introduced active counter to maintain
the list.
Add new helper that is called on connection tear down to cancel all
outstanding operations in a safe way by using proper locks and making
sure to hold a reference to any operation being cancelled.
Note that by verifying the connection state before incrementing the
active count we can make sure that all active operations have been
cancelled and that no new ones have been started when the helper
returns.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Suppress response submission on connection tear down as we do with
requests.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Call request handler helper explicitly from the work function rather
than overload the operation completion callback.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure to call the operation completion callback also when the
operation is being cancelled.
The completion callback may need to release resources allocated at
submission and the driver should be informed that the operation has
failed due to cancellation.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure to wait for the operation to become inactive before returning
after having cancelled an operation.
This makes sure that any ongoing operation completion callbacks have
finished.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Add active counter to track operations that are in use.
Note that the active count is always less than the reference count.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Clean up gb_operation_create_incoming error path by returning
immediately on allocation failures.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
An incoming operation could already be scheduled even if
gb_operation_result_set succeeds as its initial status is -EINPROGRESS.
Avoid potential use-after-free by never dropping the reference count for
incoming operations as part of cancellation.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure not to update the response message buffer for an operation
that is already scheduled for completion.
Currently if we get two incoming responses with the same id, the second
one would overwrite the response message buffer.
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Fix potential corruption of outgoing responses by verifying that the
operations is indeed outgoing when receiving a response.
Failure to do so could lead to an incoming response corrupting an
outgoing response that uses the same operation id.
Reported-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Fix potential use-after-free in response receive path, due to lack of
reference counting when looking up operations on a connection.
Make sure to acquire a reference to the operation while holding the
connection-list lock.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure to acquire the connection-list lock atomically when releasing
the final reference.
This allows the list to be traversed and references to be acquired
(while holding the lock) without racing with the destructor.
Suggested-by: Greg Kroah-Hartman <gregkh@google.com>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
An operation with a response in-flight will already have set the
operation result and would therefore never be cancelled by the current
implementation.
Note that the reference taken when sending the result will be dropped
in the message completion callback.
Also note that an incoming, non-unidirectional messages will always have
an allocated response if its result has been set.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Make sure to allocate a response message before setting the operation
result.
This is needed to handle cancellation of incoming operations.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
Drop redundant OOM message, which would already have been logged by the
memory subsystem.
Also remove a FIXME about possibly sending a pre-allocated response,
which is neither possible or desirable. If we ever run out of memory we
have bigger problems then getting a response back to firmware.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
|
|
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>
|
|
The final reference of an operation will be put after its completion
handler has run, so we must not drop the reference if it has already
been scheduled to avoid use-after-free.
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>
|
