The cmsis implementations of osMutex was trying to inspect internal
k_mutex state (the owner and lock count) in the process of trying to
acquire the lock. This is unfixably racy, by definition other
contexts will be trying to do the same on the unsynchronized data.
As far as I can tell, the only purpose was to be able to synthesize
osMutex's specified error behavior, which we can do with the existing
return codes from k_mutex_lock(). Add similar logic to osSemaphore,
which didn't have the race but was likewise abusing access to kernel
internals.
Signed-off-by: Andy Ross <andyross@google.com>
The "limit" field of struct k_sem was in use in two places in the BTLE
host code, in one it was being used as a duplicate placeholder for the
"iso_max_num" field received in read_buffer_size_v2_complete[1]. In
another, it was just being tested for zero[2].
Those are pretty clear abuses of internal data, provide minimal value
beyond a few bytes of memory in struct bt_dev_le, and in any case
won't work with zync, where that field doesn't have the same name and
may not even exist depending on app configuration.
Copy the limit value into the struct where it belongs, and use it from
there.
[1] I strongly suspect there is a bug lurking there if the semaphore
maximum is being used to implement the kind of "packet buffer" this
code looks like. Calling k_sem_give() on a "full" semaphore WILL NOT
BLOCK. It will just drop the increment on the floor and return
synchronously. Semaphores aren't msgqs or ringbuffers! But disabling
the max value feature in zync does not result in test failures, so
maybe this usage is safe.
[2] Again, this seems suspicious; a valid k_sem should never have a
zero in that field. Presumably this is really a test for "is
initialized", and so implies there's a mixed up initialization path
somewhere?
Signed-off-by: Andy Ross <andyross@google.com>
The bridge subsystem was written with a ETH_BRIDGE_INITIALIZER that
assumed it could initialize a k_mutex with a zero-filled initializer.
That never worked. Unlike semaphores, mutexes have always required a
runtime call to k_mutex_init(). What happened instead is that
k_mutex_un/lock() returned error codes, which were ignored by the code
here. So no locking was happening.
This was discovered while migrating to zync, where an attempt to
unlock an unlocked mutex is a panic condition (and where zero-filled
initializers are legal, but represent an unfair semaphore and not a
mutex, so deadlock correctly).
Signed-off-by: Andy Ross <andyross@google.com>
This is a public API for the subsystem, can be called from app
context, unlocks the local k_mutex on one of its three exit paths, and
it's quite clear that nothing ever locks that mutex!
The code used to work because k_mutex simply returned an error if you
tried to unlock an unlocked object. Now zync will panic (when
CONFIG_ZYNC_VALIDATE=y) if you try that.
Signed-off-by: Andy Ross <andyross@google.com>
Zephyrs Host has by default enabled automatic resume of advertising
in case of disconnection when peripheral role is enabled.
The feature becomes a bit problematic in case of multirole usage.
For example assume a use case where a device is working as peripheral
and central, where it may establish single connection for each role.
In case there are two connections established and connection in
central role is dropped by peer, Host will automatically resume
advertising. After that an application can resume scanning, e.g.
in disconnected callback. That should not happen.
If one of connections was used for central role, it should not
be stolen by Host to run peripheral role. Host should verify
if a disconnected connection role was peripheral and then resume
advertising.
This approach will not break backward compatibility and change
correct resume behavior. What more, Host will follow an application
decisions about use of connection objects.
Signed-off-by: Piotr Pryga <piotr.pryga@nordicsemi.no>
The `audio init` should only initialize the audio (BAP)
stack. Furthermore, given that the `bt init` may do more
than just calling `bt_enable` it is generally for the best,
and more future proof, to let that command be the exclusive
way of calling `bt_enable` in the shell.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
It was possible to enter a state where setting the QoS
for a source stream after having released a sink stream
as the unicast client would cause a segmentation error.
This issue is related to how the unicast shell is using
the unicast group.
A fix has been added for it, and a check for
whether or not `audio init` (to register the needed
callback) has been added. This check has also been added
for the broadcast sink.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
If user has not specified any DNS servers in
CONFIG_DNS_SERVER_IP_ADDRESSES, then the DNS resolver will not be
initialized properly. So fix this by always calling dns_resolve_init()
so that DNS mutex get properly initialized.
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Adds proper support for having more than one PDU pool for the advertising
PDUs (to save memory)
- Introduces lll_adv_aux_data_init, lll_adv_aux_scan_rsp_alloc and
lll_adv_sync_data_init to indicate if the PDU is a primary/legacy PDU
or secondary/auxillary PDU
- Scan response PDUs are initialized later, since the correct pool to draw
from depends on whether the advertising is extended or legacy
Signed-off-by: Troels Nilsson <trnn@demant.com>
The device_service shell was missing the capability to list devices
registered in the EARLY init level.
Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Not all C compilers understands how to use the address of a return
value as an argument to a function call. At least not our compiler.
Fix the issue by adding a variable to hold the converted value
before passing it to the function call.
Signed-off-by: Kim Sekkelund <ksek@oticon.com>
This fixes the output below by moving the closing bracket to "<Security
type (optional: valid only for secure SSIDs)>" after the enumeration of
security types "0:None, 1:PSK, 2:PSK-256, 3:SAE".
Fixes:
uart:~$ wifi
wifi - Wi-Fi commands
Subcommands:
connect :Connect to a Wi-Fi AP
"<SSID>"
<channel number (optional), 0 means all>
<PSK (optional: valid only for secure SSIDs)>
<Security type (optional: valid only for secure SSIDs)>
v----------------------^
0:None, 1:PSK, 2:PSK-256, 3:SAE
(...)
Signed-off-by: Gaël PORTAY <gael.portay@gmail.com>
Return to line after the first parameter to:
- have the second parameter in a single line (not splitted) and
- start every parameters from a brand new line (easier to read)
Signed-off-by: Gaël PORTAY <gael.portay@gmail.com>
This fixes the output below by adding the missing closing bracket to
"<MFP (optional): 0:Disable, 1:Optional, 2:Required":
Fixes:
uart:~$ wifi
wifi - Wi-Fi commands
Subcommands:
connect :Connect to a Wi-Fi AP
"<SSID>"
<channel number (optional), 0 means all>
<PSK (optional: valid only for secure SSIDs)>
<Security type (optional: valid only for secure SSIDs)>
0:None, 1:PSK, 2:PSK-256, 3:SAE
<MFP (optional): 0:Disable, 1:Optional, 2:Required
^-------------------------------------------------^
(...)
Signed-off-by: Gaël PORTAY <gael.portay@gmail.com>
This fixes the output below by returning to line after "Connect to a
Wi-Fi AP":
Fixes:
uart:~$ wifi
wifi - Wi-Fi commands
Subcommands:
connect :Connect to a Wi-Fi AP"<SSID>"
^------^
<channel number (optional), 0 means all>
(...)
Signed-off-by: Gaël PORTAY <gael.portay@gmail.com>
When calling bt_audio_discover, the discovered PAC records
were just added to the pac_cache. If called multiple times,
the PAC records would just be duplicated.
This change reset the pac records for the direction when
called.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
bt_audio_stream_config should not be called with a stream
that is already configured to another connection.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
This caused the sink to attempt to sync to more stream
for each call to bt_audio_broadcast_sink_sync as the
stream count was stored in the struct bt_audio_broadcast_sink
but never reset.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
There is no reason for the previous limitation.
The limit of 255 for the ASEs is based on the ASE IDs
which is a uint8_t, so there is a maximum of 255 of
each ASE type.
The limit of 73 * 2 = 146 is based on the maximum size of an
GATT attribute (512), and the minimum size of a PAC record (7)
makes it a total of 73 PAC records for each direction.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
There is access to the procedure context after a potential release
of the context, which (in theory) can lead to incorrect behaviour
There is no good way of testing this with a unittest without adding
specal debug code to the ull_llcp_local module
After code-inspection no other location has been found with
potential access after context release
Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no>
This refactors how the BAP broadcast source handles the
extended and periodic advertising.
First it removes the start and stop of the extended
advertising, and instead expects the application
(or upper layers) to do this.
Second it exposes API functions to get the
necessary advertising data from BAP (service data and
the BASE), which the upper layers will then also
be responsible for setting and updating.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
The metadataa_updated callback has existed for a long
time, but was never actually used. The metadata is set
or updated in the enabling and streaming state.
Added checks to see if the state actually changes when going
into these states, and if not, then it means that the
metadata was updated.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Without pkt_unref(), the shell command "net udp bind" will fail to
receive after a few packets ("net allocs" shows that memory is still
allocated).
Signed-off-by: Andreas Müller <andreas.mueller@husqvarnagroup.com>
Adding the new Kconfig (enabled by default) to make a failed assumption
mark the final result as failed. This change has the following benefits
which have been asked for by the Zephyr community:
1. A failed assumption does not go silent. In this example, the failed
assumption will still mark the test as skipped, but the final result
will be to mark the full test run as failed. This would allow
blocking the CI when an assume fails.
2. Normal test skipping via the ztest_test_skip() is unaffected by this
change. Those tests will be marked as skipped, but the binary will
still pass.
Signed-off-by: Yuval Peress <peress@google.com>
If bt_audio_stream_connect didn't fail, we would just return
without sending the receive start ready command.
Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Since the net cache is accumulated in positive order, the net
cache uses reverse order traversal, which can filter duplicate
messages more effectively.
Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
Since in bt_mesh_net_recv will call msg_cache_add
and if lpn reject this message, should remove this cache.
But this function sequentially and will not be interrupt
by any thread so, we no need a copy var to save this value.
Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
Our application (posix-naive/debug) ran into segmentaion faults,
because the socket was closed between the "wait for sockets" and the
second if in the first loop. Added the check also to line 690
and 720.
The other uses of sock_ctx[i] are already checked against NULL
Signed-off-by: Peter Tönz <peter.tonz@husqvarnagroup.com>
According to Core 5.3 Vol 4, Part E, section 7.8.82 slot_durations,
switch_pattern_len and ant_ids are used only for AoA and do not affect
reception of AoD CTE.
To improve interoperability checking of correctness of these parameters
is not required. It will not affect lower link layer in case the AoA
CTE reception feature is not enabled in Kconfig.
Signed-off-by: Piotr Pryga <piotr.pryga@nordicsemi.no>
This function exposes list pointer, so that it allows the user to modify
the internal list. This adds bt_audio_foreach_capability iterator finction
that can be used instead.
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
The LLDN frame has been obsoleted in IEEE 802.15.4-2015f. This change
removes it from the code, introduces frame types from current spec
levels and updates the frame validation rules in accordance with the
spec.
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
Several attributes in the ieee802154_context struct may potentially be
accessed from different threads and/or ISR context. Only some of these
attributes were properly guarded against race conditions.
This may not have been to problematic in the past but as other changes
in this PR introduce additional attributes and mutate several attributes
in a single atomic transaction, leaving such changes unprotected seems
dangerous.
This change therefore introduces systematic locking of the
ieee802154_context structure.
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
IEEE 802.15.4 short address support is incomplete in several places.
This change improves short address support without claiming to fix
it everywhere. Future iterations will have to continue where this change
leaves off.
The purpose of this change was to:
* use the short address returned by association responses,
* automatically bind IEEE 802.15.4 datagram sockets to the short
address if available,
* use the short address in outgoing packages where applicable,
* improve validation of association/disassociation frames,
* model association more closely to the spec by tying it to the
existence of a short address in the MAC PIB thereby removing
redundancy in the PIB (which makes race conditions less probable),
* keep both, the short and extended addresses, of the coordinator.
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
This changes fixes several bugs and inconsistencies in the IEEE 802.15.4
L2 implementation. These bugs were revealed while documenting intended
endianness of driver, IP, socket and L2 attributes (see previous
changes).
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
This is a preparatory change that fixes one aspect of short address
handling before fixing endianness so that the endianness fix can be
applied consistently in this method.
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
The IEEE 802.15.4 L2 code stores representation of attributes like
PAN id, short address and extended address in different encodings:
* big endian for extended address and CPU byte order for everything
else whenever such attributes enter user space (except for IP/socket
link layer addresses which are always big endian - even in case of
short addresses - to maintain POSIX compatibility).
* little endian for everything that is close to the radio driver as
IEEE 802.15.4 frames are little endian encoded.
Endianness was almost nowhere documented which led to several bugs and
inconsistencies where assignments of different byte order were not
converted (or sometimes converted, sometimes not).
This change documents endianness wherever possible within the realm of
the IEEE 802.15.4 L2 code. Conversion bugs and inconsistencies that were
revealed by the improved documentation will be fixed in a separate
commit.
Signed-off-by: Florian Grandel <jerico.dev@gmail.com>
Some minor housekeeping prior to adding an http server
implementation. There are already a number of http headers
and that number will likely increase with subsequent work.
Moving them into a common directory cleans up the
`include/net` directory a bit.
Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Changes in device_service have triggered MISRA 5.7 violation CI error
(Tag name should be unique). Renamed shell to sh, same as some other
modules.
Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
The _SYS_INIT_LEVEL* definitions were used to indicate the index entry
into the levels array defined in init.c (z_sys_init_run_level). init.c
uses this information internally, so there is no point in exposing this
in a public header. It has been replaced with an enum inside init.c. The
device shell was re-using the same defines to index its own array. This
is a fragile design, the shell needs to be responsible of its own data
indexing. A similar situation happened with some unit tests.
Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
