Add missing braces to comply with MISRA C:2012 Rule 15.6 and
also following Zephyr's style guideline.
Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
When change unaware client send ATT request it shall get Database
Out of Sync error. Reading GATT database hash is an exception here.
This was affecting GATT/SR/GAS/BV-05-C qualification test case.
Signed-off-by: Szymon Janc <szymon.janc@codecoup.pl>
Convert users of net_buf_put() and net_buf_get() functions to use
non-wrapped putters and getters k_fifo_put() and k_fifo_get().
Special handling of net_bufs in k_fifos is no longer needed after commit
3d306c181f, since these actions are now
atomic regardless of any net_buf fragments.
Signed-off-by: Henrik Brix Andersen <henrik@brixandersen.dk>
When developing Bluetooth applications, you typically run into
some errors. If you are an experienced Bluetooth developer,
you would typically have an HCI error lookup table in your memory.
Others might not.
This commit utilizes defines CONFIG_BT_DEBUG_HCI_ERR_TO_STR
and utilizes bt_hci_err_to_str() to print out HCI error strings
when enabled to improve the user experience.
Several alternatives where considered. This approach was chosen
as it had the best balance between readability, code size, and
implementation complexity.
The alternatives are listed below as a reference.
1. Macro defined format specifier:
```c
#define HCI_ERR_FMT "%s"
#define BT_HCI_ERR_TO_STR(err) (err)
#define HCI_ERR_FMT "%d"
#define BT_HCI_ERR_TO_STR(err) bt_hci_err_to_str((err))
LOG_INF("The event contained " HCI_ERR_FMT " as status",
BT_HCI_ERR_TO_STR(err));
```
Advantage: Space efficient: Code size does not increase
Disadvantage: Code becomes hard to read
2. Format specifier to always include both integer and string:
```c
static inline const char bt_hci_err_to_str(err)
{
return "";
}
LOG_INF("The event contained %s(0x%02x) as status",
bt_hci_err_to_str(err), err);
```
Advantage: Simple to use, implement, and read,
Disadvantage: Increases code size when CONFIG_BT_DEBUG_HCI_ERR_TO_STR
is disabled. The compiler seems unable to optimize away the unused
format specifier. Note: The size increase is only present when
logging is enabled.
3. Always print as string, allocate a stack variable when printing:
```c
const char *bt_hci_err_to_str(char *dst, size_t dst_size, uint8_t err)
{
snprintf(dst, dst_size, 0x%02x, err);
return dst;
}
LOG_INF("The event contained %s as status", BT_HCI_ERR_TO_STR(err));
```
Advantage: Very easy to read.
Disadvantage: Printing error codes becomes slow as it involves calling
snprint.
4. Implement a custom printf specifier, for example E.
This requires a global CONFIG_ERR_AS_STR as I assume we cannot have
one specifier for each type of error code.
Also, I assume we cannot start adding specifiers for each subsystem.
```c
#define BT_HCI_ERR_TO_STR(err) (err)
#define BT_HCI_ERR_TO_STR(err) bt_hci_err_to_str((err))
LOG_INF("The event contained %E as status", BT_HCI_ERR_TO_STR(err));
```
Advantage: Both efficient code and readable code.
Disadvantage: This requires a global CONFIG_ERR_AS_STR as I assume
we cannot have one specifier for each type of error code.
Also, I assume we cannot start adding specifiers for each subsystem.
That is, this approach is hard to implement correctly in a scalable
way.
Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>
It was pointed out in a future PR that they should have
a corresponding experimental Kconfig entry.
See PR #73795.
This updates the APIs added in PR #73826 and PR #74295.
Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>
Utilize a code spell-checking tool to scan for and correct spelling errors
in all files within the subsys/bluetooth/host directory.
Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
This can be useful if application developers
want to print them in the applications.
Later we can also use them in
the host to improve debuggability.
Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>
This API replaces `bt_l2cap_send()` and `bt_l2cap_send_cb()`.
The difference is that it takes the `struct bt_l2cap_le_chan` object
directly instead of a connection + CID.
We need the channel object in order to put the PDU on the TX queue. It
is inefficient to do a search for every PDU when the caller knows the
channel object's address and can just pass it down.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
`att_req_send_process` is not thread safe.
In the case where the current context is pre-emptible (e.g. when a user
sends something over GATT from the main thread):
The iterator in `att_req_send_process` can get interrupted mid-processing,
breaking the logic and resulting in an assert/crash/data corruption.
Additionally, the connection state check in this fn is also not
thread-safe, the RX thread could pre-empt us and disconnect right before
we attempt to send on an ATT bearer that is on it.
This is a hotfix until we have a generalized solution for the host API
surface. It seems a lot of our logic assumes cooperative priority.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
This served no purpose for responces. The minimum MTU is sufficient for
all ATT headers.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
When EATT is enabled, bt_att_create_rsp_pdu used to reserve headroom for
the SDU header even when responding on the UATT bearer.
That subtracted from the room for the ATT payload in the buffers. The
remaining buffer size was insufficient to create a PDU of ATT MTU size,
since the exchanged local MTU is calculated the with the assumption that
the SDU header is not present.
This broke the ATT MTU promise, and e.g. our read response will have two
bytes fewer than promised. This caused a failure in PTS.
The new bt_att_create_rsp_pdu pays attention to the bearer type and only
allocates the SDU header on EATT bearers.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
Removed the "operation" infix from the bt_gatt_authorization_cb
callback structure in the Bluetooth GATT header.
Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
If an att buf is destroyed during bearer teardown, we want to avoid
using and passing invalid references to the application.
Double-check that the current bearer, the ATT object and the ACL conn
objects it is attached to are not NULL before proceeding.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Added the GATT authorization callback API that allows the user to
define application-specific authorization logic for GATT operations.
Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
This is just as arbitrary as what was before, but simpler.
Before this change, the callback were invoked upon receiving the num
complete packets event.
This did not necessarily work with all spec-compliant controllers.
Now the callback is invoked as soon as the lower layer destroys the
buffer. ATT shouldn't care whether L2CAP sends it over RFC1149 or
something else after that point.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Co-authored-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
Why?
- metadata is easier to manage as an array + index
- less error-prone -> less memory-management bugs
- we can. because of the previous refactor
- PDU allocations are more predictable
- ATT buffer size can be optimized by app
- isolates ATT from the rest of the ACL users
- decouples ATT PDU size from e.g. SMP w/ LESC
Drawbacks:
- higher memory usage
- kconfig change
The higher memory use is only temporary, as this will be followed-up
with more refactors that should bring it back down.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Co-authored-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
This fixes CCC subscriptions that were removed if the automatic
resubscription was aborted by ACL disconnection.
As the client renews subscriptions, there is no point of removing those
if the link is disconnected unexpectedly.
The API user won't be notified about the failure, as the automatic
resubscriptions are implicit, and after reconnection the subscriptions
will be still valid.
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
It's bad form, especially since in that case, it's always the same function
that is called `bt_att_sent()`.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Enforcing the peer's behavior is not strictly necessary. All the host
should do is make sure it is resilient to a spec-violating peer.
Moreover, a growing number of platforms were disabling the check, as the
spec allows "batching" HCI num complete packets events, stalling ATT RX.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Co-authored-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
EATT is the only user of `bt_l2cap_chan_send_cb`, and not necessary. We
can maintain the same functionality without it.
Instead of passing and storing the callback into l2cap, we can store it
in a callback queue in the ATT bearer struct.
This will allow us to remove that internal API later, in order to
streamline the l2cap API.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
If the conn security elevation is already in progress, retry the ATT
request if failed due to security reasons.
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
When EATT is established, the value returned from `bt_att_get_mtu` is
not useful to determine the ATT_MTU that applies to a ATT response. This
is because `bt_att_get_mtu` may return the value for a different bearer
than the request is serviced on.
To fix this, the params struct for the GATT read operation is given a
new field that will record the ATT_MTU that applies to this ATT
operation. This value is then used to determine if the GATT long read
operation is concluded.
Fixes: https://github.com/zephyrproject-rtos/zephyr/issues/61741
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
A race condition between ATT RX and the connection teardown can happen, as
the teardown is executed from a workqueue.
For example:
- connection is established
- `connected` cb is called (in BT RX context)
- user calls `bt_conn_disconnect` in that cb
- connection is marked as disconnecting
- ATT teardown & general conn cleanup is scheduled
- BT RX gets an ATT request, tries to handle it
- ATT bearer is still not GC'd, so it tries and fails to send it
-> results in error message "not connected" on log
- ATT teardown & general conn cleanup runs
To avoid that, we not only check the bearer state, but also its ACL conn
state.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
If the peer is a zephyr host, there is no problem, as the Zephyr
host limits sending parallel REQs and INDs.
But the spec allows sending those in parallel, and it may end up that
the re-used REQ buffer hasn't been destroyed when an indication comes.
Only re-use the buffer when enqueuing ATT responses.
This means that we may run out of buffers if the peer sends too many
indications and our application also sends a lot of commands/notifications.
The rationale for this is that having to handle a lot of requests is a
more plausible scenario (e.g. being discovered by multiple peers) than
handling lots of parallel indications.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Add a pointer to the associated server structure in the L2CAP accept()
callback. This allows the callee to know which server an incoming L2CAP
connection is associated with.
Signed-off-by: Donatien Garnier <donatien.garnier@blecon.net>
Modify the signature of the k_mem_slab_free() function with a new one,
replacing the old void **mem with void *mem as a parameter.
The following function:
void k_mem_slab_free(struct k_mem_slab *slab, void **mem);
has the wrong signature. mem is only used as a regular pointer, so there
is no need to use a double-pointer. The correct signature should be:
void k_mem_slab_free(struct k_mem_slab *slab, void *mem);
The issue with the current signature, although functional, is that it is
extremely confusing. I myself, a veteran Zephyr developer, was confused
by this parameter when looking at it recently.
All in-tree uses of the function have been adapted.
Fixes#61888.
Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Using a different channel for responding to a request is forbidden by spec.
The allocator was especially flawed as it iterated over all the EATT
channels to find one w/ a big enough MTU, but the sending was still done
over the same channel as the REQ.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
This to ensure we don't fail to send a response and never get an ATT
TIMEOUT due to ACL TX buffer starvation caused by other users of the stack.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
The ATT module has provisions to queue a packet/buffer for sending later if
it can't send it right away. For example if the conn.c tx context
allocation fails.
This unfortunately doesn't work if the buffer can't get allocated in the
first place, or if the ATT metadata can't also be allocated.
The metadata is allocated from a global pool set to the same number as
conn.c TX contexts. That can lead to a situation where other users of ATT
manage to queue a bunch of buffers (e.g. the app spamming GATT
notifications), depleting the number of ATT metadata slots so that none are
available.
When none are available, and we receive an ATT REQ, we try to allocate one,
fail, and drop the buffer (!). That pretty much guarantees an ATT timeout.
As a workaround for this, use a per-channel metadata slot, that is only
used for completing transactions.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
This is useful in multilink scenarios, especially since there is not user
callback when the ATT channel times out.
Adding a user-facing callback should ideally also be done, but just logging
the address already provides useful insight.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Easier to debug that way.
Ideally we'd have more error codes/logging instead of just returning
UNLIKELY for most errors.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Make `bt_eatt_count()` return what it says on the tin, ie. the number of
channels that are actually connected.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
This fixes missing `static` function specifier.
The bt_att_chan_create_pdu is not called outside of att.c.
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
If the remote responds with and security related error the stack tries
to increase the security level to satisfy the remote permissions.
This fixes missing ATT timer reset on security related ATT Error
Response as the ATT operation is considered as complete.
< ACL Data TX: Handle 0 flags 0x00 dlen 7
ATT: Read Request (0x0a) len 2
Handle: 0x0084
TMAS: Role
> ACL Data RX: Handle 0 flags 0x02 dlen 9
ATT: Error Response (0x01) len 4
Read Request (0x0a)
Handle: 0x0084
Error: Insufficient Authentication (0x05)
TMAS: Role
Error code: 0x05
< ACL Data TX: Handle 0 flags 0x00 dlen 6
SMP: Security Request (0x0b) len 1
Authentication requirement: Bonding, No MITM, SC, No Keypresses
= bt: bt_att: ATT Timeout
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>
The ATT_PENDING_SENT flag was still not being cleared in all cases.
Also reset `data->att_chan` when not able to send on a given channel.
Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Refactoring. All occurences of `atomic_test_bit((.*)->flags,
ATT_ENHANCED)` are replaced with `bt_att_is_enhanced($1)`.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
The EATT MTU in Zephyr is static. We know it at channel creation time,
so we should communicate the MTU as part of channel creation.
Side note: With this approach, it should no longer be neccessary or
useful to do a channel reconfigure.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
The previous approach with `cap_eatt` was flawed. It would overwrite
`le_chan->tx.mtu`, losing its true value. (It is supposed to be the
L2CAP MTU as reported by the remote side.)
The previous approach worked out for UATT because the locally triggered
exchange always carries the remote MTU in the response, so we did not
need to keep track of the remote MTU.
But, unlike the UATT MTU exchange, the L2CAP reconfigure only exchanges
the MTU in one direction. If the remote does the first reconfigure, we
would correctly cap the ATT MTU to our local MTU. But, we would
incorrectly store this as the remote's MTU. When we then increase our
local MTU using `bt_eatt_reconfigure`, we correctly set and send our
MTU. But we have an incorrect notion that the remote MTU is the value
that we ourselves limited. And mistake would incorrectly limit the
negotiated ATT MTU locally.
The simplest solution is to not mess with `le_chan->tx.mtu` and just
calculate the ATT MTU like Spec intended.
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
This is a refactor. There is no behavioral change because
`BT_LOCAL_ATT_MTU_UATT` is `BT_LOCAL_ATT_MTU_EATT + 2` is
`BT_ATT_BUF_SIZE` is the old `BT_ATT_MTU`.
The old `BT_ATT_MTU` was wrong for EATT bearers. EATT MTU is two bytes
less because of ECRED overhead.
Instead of the old `BT_ATT_MTU`, we define one for each bearer type. We
also define the max of them to use as a convenience for allocating
buffers that fit either.
To avoid confusion, 'LOCAL' has been added to the name. This is to
differentiate it from what the spec calls 'ATT MTU', which is a
negotiated property. (It is the minimum of the two side's local ATT
MTU.)
Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
