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>
Add locking, and the implied barriers, to connection-state updates.
This will be used to fix a number of races in the operations and
connection-tear-down implementations.
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>
Fixing needless redefinition of operation types in gbsim reveals this typo
GB_UART_TYPE_SET_BREAK -> GB_UART_TYPE_SEND_BREAK. This patch should be
applied in lock-step to the patch to gbsim 'gbsim/uart: remove unnecessary
redefinition of operation types' since gbsim does not contain the typo.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Copy/paste of code from one file to another missed inappropriate use of tab
after define.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Throughput capture should account for the entire size of the data going out
on the wire. In addition throughput should be captured for each supported
loopback operation.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Get maximum payload by way of gb_operation_get_payload_size_max() and
truncate any requested loopback size greater than this value. RX of data
from firmware over the specified size will not be accepted.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Loopback sink command will operate as an amalgam of the ping and tranfer
operations. Sink will send an ACK'd variable size operation over greybus.
Unlike the transfer type which transmits the received data back, the sink
type will simply ACK without sending the received data back.
This patch adds the kernel side of the sink command.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Sepcifying loopback operation type with a type value that is internal to
the loopback driver is non-intunitive and requires reading code to
understand. Remove confusing duplicate definitions and update code to
accept the greybus-specification function identity defintiions as the
appropriate type values for initiating loopback operations.
See greybus-spec section 10.16.1 'Greybus Loopback Message Types' for a
full list of valid messages to set this type field to.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Magic number 2 used instead of define GB_LOOPBACK_TYPE_PING, remove and and
use the define instead.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
In order to have one point of type definition for gbsim move the loopback
operation definitions to greybus_protocols.h.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
This wouldn't happen normally, but in a buggy corner case it is possible
that all the connections aren't removed properly and they are still
present after the interfaces and endo are removed.
Warn in such a case and cleanup connections, so that /sys/bus/greybus/
can be removed while removing greybus modules.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
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>
idr and ida structures have internal memory allocated that needs to be
freed when modules are removed. So call the proper idr_destroy() or
ida_destroy() functions on the module exit path to free the memory.
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Its another special protocol (just like control protocol) and is
required to be accessed from other files, lets save a pointer to it in
interface structure.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Its an essential part of greybus core and shouldn't be present as a
separate module. Make it part of greybus.ko module.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
We failed to bind a protocol for the connection, not for bundle. And so
connection's 'dev' must be used for printing message.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Function's name is gb_interface_init(), not gb_interface_add().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
gb_connection_create() can initialize a connection if bundle->device id
is valid. And so the connection must be destroyed by calling
gb_connection_exit() before gb_connection_destroy(). This wasn't done in
the code that is parsing the manifest.
Fix it.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Fix copy to/from scatterlist destination buffer offset, fix
calculation of blocks to be transfer and make a more verbose out of
error when the blocks receive/send do not match.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
As we do not have, yet, a event callback to notify core about changes
we add the MMC_CAP_NEEDS_POLL capability to the rest of the caps
received from the module.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Before initiating a transfers, check if the command (for single block)
match the number of blocks in the request.
While at it, fix also a missing break.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
No need to pass the all request to the transfer related funtctions.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
If data is not available the stop command could dereference NULL.
Fetch the stop command directly from the request instead.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Functions were returning the wrong flag for the expected value. Swap
them.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Between the time connection with module is up and the host is added,
we can receive events (card inserted/removed, write protection
switch), so until the setup is complete we queue the events received
and handle them after.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Instead of using values in the command cmd_flags field use the real
flags in a bit mask.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
This reverts commit 22690d72ae145cf12ae3df033670ed8ad7ecdde7.
It was meant for the gbsim repo, not this one :(
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
The ccflags variable was spelled wrong, so no extra compilation flags
could be specified.
The proper flag is 'ccflags-y.'
Signed-off-by: Perry Hung <perry@leaflabs.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>
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>
Johan Hovold