Johan Hovold [Sat, 26 Sep 2015 21:37:59 +0000 (14:37 -0700)]
greybus: es1/2: fix use-after-free in completion callback
Reset the hcpriv field before returning the message to greybus core in
the OUT-URB completion callback.
This fixes a use-after-free bug when sending responses to incoming
requests as the final reference is then dropped when the message is
returned.
Reported-by: Michael Scott <michael.scott@linaro.org> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
greybus: firmware: Don't send control-disconnected event for firmware protocol
After downloading the firmware for the next boot stage, module's
firmware (for current boot stage) jumps into it and the new firmware and
sends hotplug request to SVC. On hotplug request from the SVC, the AP
first removes the existing interface.
At this time, there is no point sending disconnected event for the
firmware protocol, for the firmware used in previous stage, as the new
firmware wouldn't be aware about it.
Set flags for firmware protocol to skip control-disconnected operations.
greybus: interface: gb_interface_create() isn't called for existing interfaces
The callers are ensuring that another interface doesn't exist with the
same interface id and so there is no need to check that from
gb_interface_create() anymore.
As per the module's boot sequence diagram, the AP needs to read/clear
T_TstSrcIncrement attribute on hotplug (svc) events.
Implement that.
FIXME: This is module-hardware dependent and needs to be extended for
every type of module we want to support.
[ Based on work by Marti & Eli to clear the attribute with DME set] 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>
greybus: svc: Allow consecutive hotplug events for the same module
There are two cases where the AP may receive hotplug event for an
existing interface, without first getting a hot-unplug request for it.
- bootrom loading the firmware image and booting into that, which
only generates a hotplug event. i.e. no hot-unplug event, as the
module never went away.
- Or the firmware on the module crashed and sent hotplug request again
to the SVC, which got propagated to AP.
Handle such cases by first removing the interface, with a clear print
message shown to the user. And then following the normal hotplug
sequence to add the interface.
greybus: loopback: drop redundant endo0 string from debugfs entry name
dev_name() will return the endo0 component of the string on it's own,
there's no need to include it in the snprintf() when construting the
debugfs name. This fixes 'endo0' appearing more than once in the debugfs
name - shamefully slipped through testing cb570c93783f
('greybus/loopback: use dev_name to populate sysfsname').
greybus: loopback: masked out threads should sleep
If a thread is masked out it should not consume CPU cycles during a test.
We set an arbitrary 100 millisecond sleep time for each masked out thread.
Reasonably blunt instrument to ensure threads with nothing to do don't end
up thrashing the acquisition/release of mutexes.
greybus: loopback: move sysfs entries to /sys/bus/greybus/devices/endo0
Currently we have sysfs entries that are created when the first incoming
connection is created as sub-nodes of the module associated with that
connection e.g. /sys/bus/grebus/devices/endo0:X where X is the module
identifier associated with the new connection. This is conceptually
incorrect since the sysfs entries we create actually aren't bound to a
module. Depending on the order connections are brought up we can also have
a situation where /sys/bus/greybus/devices/endo0:X has high-level control
sysfs data-points but /sys/bus/greybus/devices/endo0:Y does not. Rather
than needlessly replicate data-points across each endo0:X, endo0:Y, endo0:Z
sysfs directories it is more sensible to locate the entries in
/sys/bus/greybus/devices/endo0.
greybus: es1, es2: hook tracepoints to hardware send/recv operations
This patch hooks tracepoints for the handoff point to/from hardware. With
these tracepoints in place we can view the time between gb_message_send and
usb_submit_urb and similarly we can view the time between cport_in_callback
and gb_message_recv_response/gb_message_recv_request
greybus: tracepoints: add tracepoints for host_device tx/rx
This patch adds new tracepoint declarations to greybus_trace.h to allow for
capture of greybus host device tx and rx events. These two tracepoints
allow an observer to see the point where the hardware interface driver
performs the relevant read or write to receive or write the data it's been
given from the higher layer greybus driver.
The following two new tracepoints are declared:
- trace_gb_host_device_send
- trace_gb_host_device_recv
greybus: loopback: drop dependency on internal timestamps
This patch drops tracking of internal latencies, it's possible to derive
these times via kernel tracepoints and some user-space scripting. Since
there's no other use of the internal timestamp than the loopback driver we
remove the connection.c, connection.h, es1.c, es2.c and loopback.c
inter-dependency in one go.
The Qualcomm kernel builds with -Werror so the existing es2.c driver
breaks the build due to unused static functions. As we are still
hashing out exactly how to implement this logic at the moment, just
comment out the functions to make the build be clean, no logic changes
happen here at all.
Reported-by: Michael Scott <michael.scott@linaro.org> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
These host-driver callbacks were intended to allow host drivers to
prepare a cport, something which can now be handled by the cport
enable/disable callbacks instead.
The current create/destroy are somewhat confusingly named as they were
not supposed to create or destroy connections. They were however called
from the unrelated helper functions that do create and destroy SVC
connections.
Furthermore, no errors were returned should the create callback fail,
which should have caused the connection initialisation to fail.
Remove these unused callbacks for now, and let us use the cport
enable/disable callbacks that should be able handle all host cport
initialisation (possibly after also adding an interface to provide
information for endpoint-cport mapping).
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Johan Hovold [Thu, 17 Sep 2015 11:17:26 +0000 (13:17 +0200)]
greybus: hd: add optional cport enable and disable callbacks
Add optional cport enable and disable callbacks to the greybus host
drivers, that can be used to initialise and allocate/release resources
associated with a cport during connection setup/teardown (e.g. software
queues and hardware state).
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Johan Hovold [Thu, 17 Sep 2015 11:17:21 +0000 (13:17 +0200)]
greybus: connection: clean up svc-connection creation
Move SVC-connection creation to its own helper.
Note that the connection_create host-driver callback is really
unrelated to the SVC connection, and will be removed by a later patch.
It is is included for now as the connection_destroy callback is
currently made from the SVC-connection-destroy helper.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Add the missing version-request definition that was falsely claimed to
be empty.
Update the generic version-request helper and SVC version-request
handler to use the request definition rather than rely on the response
happening to have the same layout, something which also improves
readability.
Remove a misplaced "Control Protocol" header while at it.
Johan Hovold [Mon, 14 Sep 2015 18:19:04 +0000 (20:19 +0200)]
greybus: sdio: fix work-queue leak and use-after-free
A single global work-queue pointer was used for the per-connection
workqueue, something which would lead to memory leaks and all sorts of
bad things if there are ever more than one SDIO connection in a system.
Also add the missing error handling when allocating the queue.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Module's Bootrom needs a way to know (currently), when to start sending
requests to the AP. The version request is sent before connection_init()
routine is called, and if the module sends the request right after
receiving version request, the connection->private field will be NULL.
Fix this TEMPORARILY by sending an AP_READY request.
greybus: firmware: Fix incorrect firmware file's name
All the id-fields are 32 bit long instead of 16 bits and so we will need
8 characters per field instead of four. Also the stage field is only one
byte long and so needs just two characters to represent it.
greybus: loopback: use dev_name to populate sysfsname
dev_name() will give a nice string representing the end0:X:Y:Z:W name
mitigating the need to pick apart the various nested data structures and
print out their various identifiers.
In order to extract timestamps from gb_message instead of gb_connection we
will need access to the gb_operation structure. A first step to that is to
create our own gb_loopback_operation_sync which will call
gb_operation_request_send_sync() directly. Once loopback is using this
function internally it will be possible to convert to gb_message based
timestamps and drop gb_connection based timestamps in two seperate patches.
Current code assigns bitmask ids based on the order of discovery, however
user-space has no idea what the order of discovery is. This patch sorts the
linked list of connections associated with the loopback driver and assigns
a bit-id based on the sorted list - not the order of discovery. This
change therefore enforces the end-users idea that end0:3:3:1:1 above is
always denoted by bit 1 - even if from the AP's perspective it was the last
entry discovered.
greybus: loopback: ensure sysfs entries are cleaned up on exit
bdd4bba4 ('greybus/loopback: add module level sys/debug fs data points')
added a sysfs entry attached to gb_dev but missed the jump to out_sysfs_dev
This patchs fixes the missing jump to out_sysfs_dev.
greybus: loopback: ensure debugfs entires are cleaned up on exit
bdd4bba4 ('greybus/loopback: add module level sys/debug fs data points')
added a debugfs entry attached to gb_dev but omitted the cleanup on gb_init
error and gb_exit. This patchs fixes the missing debugfs_remove().
greybus: loopback: hold a coarse lock while init/exit run
This patch holds gb_dev.mutex for the duration of init and exit to reduce
complexity while ensuring that init and exit run atomically with respect
to slave threads @ gb_loopback_fn().
greybus: connection: Add sysfs 'ap_cport_id' file for connections
Its a very useful piece of information, i.e. the cport id of the AP to
which the cport of the module is connected, and is required lots of
times. It isn't known in advance as it is allocated at runtime.
This patch creates another file 'ap_cport_id', only for the connection
directories, which will give the cport id of the AP.
greybus: svc: destroy the route on module hot-unplug
We created two-way routes between the AP and module's interface on
hotplug, and forgot to remove them on hot-unplug. The same is also
required while handling errors in hotplug case.
greybus: connection: Call connection_destroy() from gb_connection_svc_connection_destroy()
connection_create() is called right after svc is requested to create the
connection and so connection_destroy() must be called just before we
request the SVC to destroy the connection.
Over that, this fixes the inconsistency where connection_create() is
called for all connections except SVC connection, but
connection_destroy() is called always.
Acked-by: Alexandre Bailon <abailon@baylibre.com> Fixes: 5313ca607afb ("Greybus driver: add a new callbacks to driver") 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>
greybus: connection: call gb_svc_connection_create() from gb_connection_init()
There are two operations which very much work together:
- AP asks the SVC to create a connection between a cport of AP and a
cport of module.
- AP tells the module that the connection is created.
Its better (logically) to do these two operations together and so call
gb_svc_connection_create() from gb_connection_init() instead. Also check
its return value properly.
greybus: manifest: release cport descriptors to avoid 'excess descriptors' warning
If we fail to initialize a cport of a bundle, we abort the entire
bundle. But that leads to following (unnecessary) warnings as few of the
cport descriptors, belonging to the aborted bundle were never parsed:
"greybus: excess descriptors in interface manifest"
Fix that by releasing all cport descriptors for the aborted bundle.
greybus: manifest: don't reject the interface on failing to initialize a cport
A 'bundle' represents a device in greybus. It may require multiple
cports for its functioning. If we fail to setup any cport of a bundle,
we better reject the complete bundle as the device may not be able to
function properly then.
But, failing to setup a cport of bundle X doesn't mean that the device
corresponding to bundle Y will not work properly. Bundles should be
treated as separate independent devices.
While parsing manifest for an interface, treat bundles as separate
entities and don't reject entire interface and its bundles on failing to
initialize a cport. But make sure the bundle which needs the cport, gets
destroyed properly.
We now release the bundle descriptor before parsing the cports, but that
shouldn't make any difference.
greybus: loopback: make sure to list_del on connection_exit
gb_loopback_connection_exit does a kfree on a data structure associated
with a loopback connection but fails to do a corresponding list_del(). On
subsequent enumerations this can lead to a NULL pointer dereference. Each
list_add in gb_loopback_connection_init() must have a corresponding
list_del in gb_loopback_connection_exit(), this patch adds the relevant
list_del() and ensures that an appropriate mutex protecting gb_dev.list is
held while doing so.
greybus: Greybus driver: add a new callbacks to driver
Add connection_create and connection_destroy callbacks.
ES2 can map a cport to a pair of endpoints.
Because ES2 have only a few pair of endpoints, ES2 need to have
access to some high level connection information such as protocol id
to effectively map the cports.
These callback will provide these information and help ES2 to map cports.
Viresh Kumar [Mon, 31 Aug 2015 11:51:15 +0000 (17:21 +0530)]
greybus: connection: protocol can be NULL in gb_connection_exit()
gb_connection_exit() is getting called from gb_connection_destroy() now,
which will get called from failure path of gb_connection_create_range()
(in a later commit). And at that point connection->protocol will be
NULL.
Don't print an error message if this happens in gb_connection_exit().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Viresh Kumar [Mon, 31 Aug 2015 11:51:13 +0000 (17:21 +0530)]
greybus: connection: call gb_connection_exit() from gb_connection_destroy()
Both the routines are always called together and in the same sequence.
Rather than duplicating this at different places, make
gb_connection_destroy() call gb_connection_exit().
This also makes it more sensible, as gb_connection_init() is never
called directly by the users and so its its counterpart shouldn't be
called directly as well.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Viresh Kumar [Mon, 31 Aug 2015 11:51:05 +0000 (17:21 +0530)]
greybus: svc: No need to return errors from [gb_]svc_connection_destroy()
These routines are responsible to destroy a connection that is going
away, the return value is of no use. At best, print an error message to
show that we got an error.
Make their return type void.
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
The request sequence for SVC protocol is fixed at least upto SVC_HELLO
request. The first request has to be Protocol Version, followed by
SVC_HELLO. Any other request can follow them, but these two.
Add another field in 'struct gb_svc' that keeps track of current state
of the protocol driver. It tracks only upto SVC_HELLO, as we don't need
to track later ones.
Also add a comment, about the order in which the requests are allowed
and why a race can't happen while accessing 'state'.
This removes the WARN_ON() in gb_svc_hello() as we track state
transition with 'state' field.
This also fixes a crash, when the hotplug request is received before
fully initializing the svc connection. The crash mostly happens while
accessing svc->connection->bundle, which is NULL, but can happen at
other places too, as svc connection isn't fully initialized.
Reported-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
[johan: add 0x-prefix to warning message ] Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
greybus: build: android: avoid building for targets that don't have kernel binary
For sdk related targets, the greybus build will error out due to missing
kernel dependency. Let's avoid those targets by checking TARGET_NO_KERNEL.
Signed-off-by: Vishal Bhoj <vishal.bhoj@linaro.org> Signed-off-by: Michael Scott <michael.scott@linaro.org> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Johan Hovold [Wed, 2 Sep 2015 16:03:21 +0000 (18:03 +0200)]
greybus: fix cport-id defines
The CPORT_ID_MAX define has been used by host drivers as a device limit,
but also for sanity checks when parsing manifests.
Now that it's only used for sanity checks we can increase it to the
specification maximum (4095) and get rid of the config-option that could
be used to override the previous limit (128).
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
There is no need to store the endpoint number of the control requests since
the default control endpoint is used and the USB standard defines for it a fixed
endpoint number of 0.
Remove every instance of the field control_endpoint and replace it with a
hardcoded 0 value.
Signed-off-by: Fabien Parent <fparent@baylibre.com> Reviewed-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
greybus: es2: dynamically allocate array for cport <-> ep mapping
In order to be able to dynamically determine the number of CPorts supported
by the UniPro IP instead of hardcoding the value we need to dynamically
allocate the array that is doing the cport-ep mapping.
Signed-off-by: Fabien Parent <fparent@baylibre.com> Reviewed-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
This commit is doing the preparation work in order to get the number of cports
supported from the UniPro IP instead of using a constant defined in a Kconfig
file.
Greybus host device is now holding the cport count, and all the code will now
use this value instead of the constant CPORT_ID_MAX when referring to an AP's
CPort ID.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
[johan: es1 supports 256 cports, minor style changes ] Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
greybus: es2: rename misnamed CPORT_MAX into CPORT_COUNT
Rename the misnamed macro CPORT_MAX into CPORT_COUNT. CPORT_MAX could let
people think that the macro is holding the value of the last CPort ID
usable.
Signed-off-by: Fabien Parent <fparent@baylibre.com> Reviewed-by: Johan Hovold <johan@hovoldconsulting.com> Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Use scnprintf in the generic attribute helper, which does not currently
check for buffer overflow.
The attribute helper is used to print generic strings, which could
potentially overflow the buffer. Note that the only strings currently
exported are taken from greybus string descriptors and should therefore
be limited to 255 chars.
projects / karo-tx-linux.git / log