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>
gb_connection_init() can fail and will return proper error code in that
case, but the caller is ignoring it currently.
Fix that by properly handling errors returned from gb_connection_init()
and propagating them to callers of gb_connection_bind_protocol().
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
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>
We just got an error, propagate the exact return value instead of 0.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
connection->protocol will always be valid in gb_connection_init() as it
is called only from a single routine, after initializing the 'protocol'
field.
No need to check it again.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Failures from control-connected operations are fatal errors and
must be reported with dev_err() instead of dev_warn().
Fix it.
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
[ johan: do not promote disconnected warnings, update summary ]
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Its not used by external users, mark it static. This required some
shuffling of the code.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Move the function to an earlier place, to kill the unnecessary forward
declaration.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
There are no external users of these, and probably would never be. Make
them static.
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Not sure why they were created, but there is no need for them. Kill
them.
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
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>
These structures are exchanged between the AP and the module and must be
packed to avoid any unwanted holes.
Its all working currently because compiler doesn't add any pad bytes for
these structures, as their elements are already aligned to their size.
But these structures can change in future and we better mark them
packed.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
These structures are expected to be packed by the module firmware code,
but the kernel wasn't following it until now.
Its all working currently because compiler doesn't add any pad bytes for
these structures, as their elements are already aligned to their size.
But these structures can change in future and we better mark them
packed.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
All request/responses either have a structure representing them or a
comment saying the request/response payload doesn't exist.
The comment was missing for route create response message, add it.
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>
System headers should get included before greybus.h. Its followed
everywhere except svc.c. Fix it.
Reported-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
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>
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>
The CPort count of es1 is now defined by CPORT_COUNT.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Make sure to return an errno when a host-device buffer-size check fails.
Fixes: 1f92f6404614 ("core: return error code when creating host device")
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>
Use the control request REQUEST_CPORT_COUNT in order to get the number of
CPorts supported by the UniPro IP.
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
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>
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>
There is no need to perform connection->bundle->intf->hd as the same can
be done with connection->hd.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Replace pr_err with the more descriptive dev_err. Also include the error
code on failure to register the PWM chip.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
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.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Use GFP_KERNEL for hot-plug state allocation in
gb_svc_intf_hotplug_recv, which is called from a request handler (i.e.
a work queue).
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Use GFP_KERNEL for device-id allocation in svc_process_hotplug, which is
called from a work queue.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Use GFP_KERNEL for endo ida allocation in gb_endo_register, which is not
called from atomic context.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Broadcast command with response and without response where swapped
related to what is defined in greybus specification.
Make it coherent with the document.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Use snprintf when generating the firmware name to avoid stack corruption
if the fixed-size buffer overflows.
Note that the current buffer size appears to expect 16-bit ids while
the they are actually 32-bit, something which could trigger the
corruption.
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Greybus messages with a multiple size of 512B generate timeouts
(any other message size doesn't).
512B is exactly the packet size of a bulk out endpoint.
Hence USB device is expecting a short (< 512B)
or zero-length packet to finish the transfer,
which is never generated and causes the timeout.
Set the transfer flag to send a zero-length packet in this situation.
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
Reviewed-by: Patrick Titiano <ptitiano@baylibre.com>
Reviewed-by: Johan Hovold <johan@hovoldconsulting.com>
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>
Alex previously post a patch to fix this typo. Somehow it fell through the
cracks in the meantime. Do it again 'stastic' is a word 'statistic' is not.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Code this far has used the first connection's sysfs entry to present
variables intended to control the entire test - across multiple
connections. This patch changes that so that the module level variables
only appear at the end0:x level in sysfs.
Example:
Total counts for errors over the entire set of connections will be here
/sys/bus/greybus/devices/endo0:x/error_dev
In contrast an error for each connection will be presented like this
/sys/bus/greybus/devices/endo0❌y:z:w/error_con
x = <module-id>
y = <interface-id>
z = <bundle-id>
w = <cport-id>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
checkpatch.pl is choking on a later change to dev_stats_attrs, where
checkpatch expects to see the values encapsulated in curly brackets.
Encapsulating in curly brackets will cause a compiler error. To resolve the
dichotomy this patch drops the macros and adds the arrary declarations
directly.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Feature add which enables the ability to select a bit-mask of connections
to run when executing a loopback test set. This is a feature add to
facilitate testing on the firmware side minus the necessity to recompile
firmware to support unicast (v) multicast (v) bitmask.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
This patch adds the ability to time the delta between all threads like this
t1 = timestmap();
thread1:gb_operation_sync();
thread2:gb_operation_sync();
t2 = timestamp();
In order to enable that behaviour without forcing an undesirable
checkpointing scheme this patch introduces a kfifo for each thread to store
the raw timestamps and calculate a time difference.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
The __gb_loopback_calc_latency will be useful in later patches. Provide it
here and use as intended. Later on we just want to use the timestamp
rollover detection, so split it out now.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
A helper function to convert from a nanosecond value to a latency value
expressed in mircoseconds. This will be used again in subsequent patches.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
The loopback code as currently implemented allows free running threads to
blast data to cports in isolation, however no overall stastics are gathered
for the aggregate throughput to a given module.
This patch moves derivation of stastics for all cports to one structure
so that 1 to n cports will report their overall stastics in one place.
Later patches update the sysfs/debugfs accessor functions to provide the
per-module and per-connection data separately.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Greg previously suggested switching over to debugfs instead of a char
interface to report raw samples to user-space. At the time we agreed not
to go for that change. However later patches in this series are made
simpler if debugfs is used instead of /dev, so it makes sense to make the
conversion now for that reason. This patch removes the char interface and
replaces it with a debugfs interface. Raw samples will be acquired from
/sys/kernel/debug/gb_loopback/raw_latency_endo0❌y:z:w where
x = <module-id>
y = <interface-id>
z = <bundle-id>
w = <cport-number>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
This patch adds the ability to graph the latency of the overhead introduced
by the greybus stack in the roundtrip time AP->Module->AP.
Since this is a small number it is reported in nanoseconds, not
mircoseconds. This data can also be derived with tracepoints but it's being
provided in this format to export to CSV file more easily than with
tracepoints.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
'user-replicable' means something that a user can replicate.
'user-replaceable' means something that a user can replace. We defintely
mean to say replaceable not replicable here.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Greybus core now handle the _INVALID and PROTOCOL_VERSION types, so no
need to have specific protocol macros for this. Just drop them.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Reviewed-by: Viresh Kumar <viresh.kumar@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>
[[, == and echo -e are bash/zsh-ism and not POSIX, so when using a POSIX
shell the kernel_cmp can issue some warnings and not work properly.
Use only POSIX operators for kernel version compare.
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
Tested-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Viresh Kumar