* RE: gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching.
2026-01-05 10:38 [PATCH v2 1/1] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching Mengshi Wu
@ 2026-01-05 10:54 ` bluez.test.bot
0 siblings, 0 replies; 9+ messages in thread
From: bluez.test.bot @ 2026-01-05 10:54 UTC (permalink / raw)
To: linux-bluetooth, mengshi.wu
[-- Attachment #1: Type: text/plain, Size: 6737 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1038430
---Test result---
Test Summary:
CheckPatch PENDING 0.32 seconds
GitLint PENDING 0.42 seconds
BuildEll PASS 18.16 seconds
BluezMake FAIL 15.57 seconds
MakeCheck FAIL 27.17 seconds
MakeDistcheck PASS 226.21 seconds
CheckValgrind FAIL 13.03 seconds
CheckSmatch FAIL 16.42 seconds
bluezmakeextell FAIL 11.01 seconds
IncrementalBuild PENDING 0.36 seconds
ScanBuild FAIL 13.37 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: BluezMake - FAIL
Desc: Build BlueZ
Output:
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7873: src/shared/libshared_mainloop_la-gatt-client.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7600: src/shared/libshared_glib_la-gatt-client.lo] Error 1
make: *** [Makefile:11022: check] Error 2
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind
Output:
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7873: src/shared/libshared_mainloop_la-gatt-client.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:11022: check] Error 2
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
src/shared/crypto.c:271:21: warning: Variable length array is used.
src/shared/crypto.c:272:23: warning: Variable length array is used.
src/shared/gatt-helpers.c:768:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:846:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1339:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1370:23: warning: Variable length array is used.
src/shared/gatt-server.c:278:25: warning: Variable length array is used.
src/shared/gatt-server.c:618:25: warning: Variable length array is used.
src/shared/gatt-server.c:716:25: warning: Variable length array is used.
src/shared/bap.c:312:25: warning: array of flexible structures
src/shared/bap.c: note: in included file:
./src/shared/ascs.h:88:25: warning: array of flexible structures
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7873: src/shared/libshared_mainloop_la-gatt-client.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: bluezmakeextell - FAIL
Desc: Build Bluez with External ELL
Output:
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7873: src/shared/libshared_mainloop_la-gatt-client.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
##############################
Test: ScanBuild - FAIL
Desc: Run Scan Build
Output:
src/shared/gatt-client.c: In function ‘db_hash_check_read_cb’:
src/shared/gatt-client.c:2028:29: error: unused variable ‘op’ [-Werror=unused-variable]
2028 | struct service_changed_op *op;
| ^~
src/shared/gatt-client.c: In function ‘process_db_out_of_sync’:
src/shared/gatt-client.c:2172:29: error: unused variable ‘op’ [-Werror=unused-variable]
2172 | struct service_changed_op *op;
| ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7873: src/shared/libshared_mainloop_la-gatt-client.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 0/2] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching.
@ 2026-01-21 8:38 Mengshi Wu
2026-01-21 8:38 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Mengshi Wu
2026-01-21 8:38 ` [PATCH v3 2/2] gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism Mengshi Wu
0 siblings, 2 replies; 9+ messages in thread
From: Mengshi Wu @ 2026-01-21 8:38 UTC (permalink / raw)
To: luiz.dentz
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz, Mengshi Wu
shared/att: Implement ATT error retry mechanism with callback support
gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism
Here are the btmon logs showing the automatic recovery of a failed
ATT request caused by a database out‑of‑sync error, when recovery
is possible:
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 7 #1 [hci1] 3.4439
ATT: Read Request (0x0a) len 2
Handle: 0x000b
> HCI Event: Number of Completed Packets (0x13) plen 5 #2 [hci1] 3.4588
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 6 #3 [hci1] 3.4948
ATT: Read Response (0x0b) len 1
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 7 #4 [hci1] 23.7473
ATT: Read Request (0x0a) len 2
Handle: 0x000b
> HCI Event: Number of Completed Packets (0x13) plen 5 #5 [hci1] 23.7722
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #6 [hci1] 23.8068
ATT: Error Response (0x01) len 4
Read Request (0x0a)
Handle: 0x000b
Error: Database Out of Sync (0x12)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #7 [hci1] 23.8093
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0001-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #8 [hci1] 23.8297
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 24 #9 [hci1] 23.8668
ATT: Read By Type Response (0x09) len 19
Attribute data length: 18
Attribute data list: 1 entry
Handle: 0x000d
Value: a01f96d239c187f8ba218f084501dad9
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #10 [hci1] 23.8693
ATT: Read By Type Request (0x08) len 6
Handle range: 0x000e-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #11 [hci1] 23.8914
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #12 [hci1] 23.9268
ATT: Error Response (0x01) len 4
Read By Type Request (0x08)
Handle: 0x000e
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 7 #13 [hci1] 23.9293
ATT: Read Request (0x0a) len 2
Handle: 0x000b
> HCI Event: Number of Completed Packets (0x13) plen 5 #14 [hci1] 23.9497
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 6 #15 [hci1] 23.9908
ATT: Read Response (0x0b) len 1
Here are the btmon logs showing the automatic rediscovery triggered by
a database out‑of‑sync error:
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 7 #22 [hci1] 114.579 784
ATT: Read Request (0x0a) len 2
Handle: 0x000b
> HCI Event: Number of Completed Packets (0x13) plen 5 #23 [hci1] 114.610 893
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #24 [hci1] 114.646 637
ATT: Error Response (0x01) len 4
Read Request (0x0a)
Handle: 0x000b
Error: Database Out of Sync (0x12)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #25 [hci1] 114.647 233
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0001-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #26 [hci1] 114.670 946
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 24 #27 [hci1] 114.706 865
ATT: Read By Type Response (0x09) len 19
Attribute data length: 18
Attribute data list: 1 entry
Handle: 0x000d
Value: 9eada072929f475ffa51d09c55f5e178
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #28 [hci1] 114.709 230
ATT: Read By Type Request (0x08) len 6
Handle range: 0x000e-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #29 [hci1] 114.730 974
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #30 [hci1] 114.766 849
ATT: Error Response (0x01) len 4
Read By Type Request (0x08)
Handle: 0x000e
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #31 [hci1] 114.769 215
ATT: Read By Group Type Request (0x10) len 6
Handle range: 0x0001-0xffff
Attribute group type: Primary Service (0x2800)
> HCI Event: Number of Completed Packets (0x13) plen 5 #32 [hci1] 114.791 091
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 24 #33 [hci1] 114.826 851
ATT: Read By Group Type Response (0x11) len 19
Attribute data length: 6
Attribute group list: 3 entries
Handle range: 0x0001-0x0005
UUID: Generic Access Profile (0x1800)
Handle range: 0x0006-0x000f
UUID: Generic Attribute Profile (0x1801)
Handle range: 0x0010-0x0012
UUID: Device Information (0x180a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #34 [hci1] 114.829 308
ATT: Read By Group Type Request (0x10) len 6
Handle range: 0x0013-0xffff
Attribute group type: Primary Service (0x2800)
> HCI Event: Number of Completed Packets (0x13) plen 5 #35 [hci1] 114.850 958
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 26 #36 [hci1] 114.886 942
ATT: Read By Group Type Response (0x11) len 21
Attribute data length: 20
Attribute group list: 1 entry
Handle range: 0x0027-0x002a
UUID: Vendor specific (12345678-1234-5678-1234-56789abcdef0)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #37 [hci1] 114.887 480
ATT: Read By Group Type Request (0x10) len 6
Handle range: 0x002b-0xffff
Attribute group type: Primary Service (0x2800)
> HCI Event: Number of Completed Packets (0x13) plen 5 #38 [hci1] 114.910 960
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #39 [hci1] 114.946 605
ATT: Error Response (0x01) len 4
Read By Group Type Request (0x10)
Handle: 0x002b
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #40 [hci1] 114.949 145
ATT: Read By Group Type Request (0x10) len 6
Handle range: 0x0001-0xffff
Attribute group type: Secondary Service (0x2801)
> HCI Event: Number of Completed Packets (0x13) plen 5 #41 [hci1] 114.970 937
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #42 [hci1] 115.006 864
ATT: Error Response (0x01) len 4
Read By Group Type Request (0x10)
Handle: 0x0001
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #43 [hci1] 115.007 315
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0013-0x002a
Attribute type: Include (0x2802)
> HCI Event: Number of Completed Packets (0x13) plen 5 #44 [hci1] 115.031 117
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #45 [hci1] 115.066 838
ATT: Error Response (0x01) len 4
Read By Type Request (0x08)
Handle: 0x0013
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #46 [hci1] 115.067 316
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0013-0x002a
Attribute type: Characteristic (0x2803)
> HCI Event: Number of Completed Packets (0x13) plen 5 #47 [hci1] 115.092 808
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 27 #48 [hci1] 115.126 872
ATT: Read By Type Response (0x09) len 22
Attribute data length: 21
Attribute data list: 1 entry
Handle: 0x0028
Value: 1a2900f1debc9a785634127856341278563412
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #49 [hci1] 115.127 341
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0029-0x002a
Attribute type: Characteristic (0x2803)
> HCI Event: Number of Completed Packets (0x13) plen 5 #50 [hci1] 115.150 892
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #51 [hci1] 115.186 863
ATT: Error Response (0x01) len 4
Read By Type Request (0x08)
Handle: 0x0029
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #52 [hci1] 115.238 433
ATT: Read By Type Request (0x08) len 6
Handle range: 0x0001-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #53 [hci1] 115.270 923
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 24 #54 [hci1] 115.310 644
ATT: Read By Type Response (0x09) len 19
Attribute data length: 18
Attribute data list: 1 entry
Handle: 0x000d
Value: 9eada072929f475ffa51d09c55f5e178
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 11 #55 [hci1] 115.311 066
ATT: Read By Type Request (0x08) len 6
Handle range: 0x000e-0xffff
Attribute type: Database Hash (0x2b2a)
> HCI Event: Number of Completed Packets (0x13) plen 5 #56 [hci1] 115.332 821
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 9 #57 [hci1] 115.366 878
ATT: Error Response (0x01) len 4
Read By Type Request (0x08)
Handle: 0x000e
Error: Attribute Not Found (0x0a)
bluetoothd[650]: < ACL Data TX: Handle 1 flags 0x00 dlen 9 #58 [hci1] 115.367 532
ATT: Write Request (0x12) len 4
Handle: 0x0009
Data: 0200
> HCI Event: Number of Completed Packets (0x13) plen 5 #59 [hci1] 115.392 826
Num handles: 1
Handle: 1
Count: 1
> ACL Data RX: Handle 1 flags 0x02 dlen 5 #60 [hci1] 115.426 845
ATT: Write Response (0x13) len 0
> HCI Event: LE Meta Event (0x3e) plen 6 #61 [hci1] 123.431 741
LE PHY Update Complete (0x0c)
Status: Success (0x00)
Handle: 1
TX PHY: LE 1M (0x01)
RX PHY: LE 2M (0x02)
Changes from v2:
- Detects DB_OUT_OF_SYNC errors during GATT operations
- Extracts affected handles from the original request PDU
- Checks if Service Changed indications overlap with those handles
- Verifies database consistency using Database Hash characteristic
- Automatically retries the original request if DB is consistent
- Automatically retries the original request if handle is not affected
- Link to v2
https://lore.kernel.org/all/20260105103828.105346-1-mengshi.wu@oss.qualcomm.com/
Changes from v1:
- Implement automatic recovery when ATT_ECODE_DB_OUT_OF_SYNC error is
received from the remote device.
- Link to v1
https://lore.kernel.org/all/20251208101915.247459-1-mengshi.wu@oss.qualcomm.com/
Mengshi Wu (2):
shared/att: Implement ATT error retry mechanism with callback support
gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism
src/shared/att.c | 182 ++++++++++++++++++++++++++++++++++++--
src/shared/att.h | 16 ++++
src/shared/gatt-client.c | 168 +++++++++++++++++++++++++++++++++++
src/shared/gatt-helpers.c | 16 ++++
src/shared/gatt-helpers.h | 3 +
5 files changed, 378 insertions(+), 7 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support
2026-01-21 8:38 [PATCH v3 0/2] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching Mengshi Wu
@ 2026-01-21 8:38 ` Mengshi Wu
2026-01-21 8:59 ` gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching bluez.test.bot
2026-01-21 18:15 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Luiz Augusto von Dentz
2026-01-21 8:38 ` [PATCH v3 2/2] gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism Mengshi Wu
1 sibling, 2 replies; 9+ messages in thread
From: Mengshi Wu @ 2026-01-21 8:38 UTC (permalink / raw)
To: luiz.dentz
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz, Mengshi Wu
Add a retry mechanism for ATT operations that allows upper layers to
decide whether to retry failed requests. This includes:
- Add retry callback registration (bt_att_set_retry_cb) to allow
applications to handle retry decisions
- Implement pending_retry state tracking in att_send_op to store
operations awaiting retry approval
- Add bt_att_retry_request() and bt_att_cancel_retry() functions to
approve or reject retry attempts
- Store error PDUs during retry_pending state for callback inspection
- Modify handle_error_rsp() to return retry decision codes instead of
boolean values
- Add BT_ATT_RETRY_* constants for retry decision handling
- Update GATT helpers to support retry callbacks for operations like
discovery and read/write requests
This enables more robust error handling by allowing the application
layer to implement custom retry logic based on ATT error codes.
Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
---
src/shared/att.c | 182 +++++++++++++++++++++++++++++++++++++++++++++--
src/shared/att.h | 16 +++++
2 files changed, 191 insertions(+), 7 deletions(-)
diff --git a/src/shared/att.c b/src/shared/att.c
index 77ca4aa24..4ae97530a 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -47,6 +47,7 @@ struct bt_att_chan {
struct att_send_op *pending_req;
struct att_send_op *pending_ind;
+ struct att_send_op *pending_retry;
bool writer_active;
bool in_req; /* There's a pending incoming request */
@@ -78,6 +79,10 @@ struct bt_att {
bt_att_destroy_func_t timeout_destroy;
void *timeout_data;
+ bt_att_retry_func_t retry_callback;
+ bt_att_destroy_func_t retry_destroy;
+ void *retry_data;
+
uint8_t debug_level;
bt_att_debug_func_t debug_callback;
bt_att_destroy_func_t debug_destroy;
@@ -194,6 +199,9 @@ struct att_send_op {
void *pdu;
uint16_t len;
bool retry;
+ bool retry_pending; /* Waiting for approval to retry */
+ uint8_t *error_pdu; /* Stored error PDU for retry_pending */
+ uint16_t error_pdu_len;
bt_att_response_func_t callback;
bt_att_destroy_func_t destroy;
void *user_data;
@@ -210,6 +218,7 @@ static void destroy_att_send_op(void *data)
op->destroy(op->user_data);
free(op->pdu);
+ free(op->error_pdu);
free(op);
}
@@ -644,6 +653,9 @@ static void bt_att_chan_free(void *data)
if (chan->pending_ind)
destroy_att_send_op(chan->pending_ind);
+ if (chan->pending_retry)
+ destroy_att_send_op(chan->pending_retry);
+
queue_destroy(chan->queue, destroy_att_send_op);
io_destroy(chan->io);
@@ -682,6 +694,11 @@ static bool disconnect_cb(struct io *io, void *user_data)
chan->pending_ind = NULL;
}
+ if (chan->pending_retry) {
+ disc_att_send_op(chan->pending_retry);
+ chan->pending_retry = NULL;
+ }
+
bt_att_chan_free(chan);
/* Don't run disconnect callback if there are channels left */
@@ -777,16 +794,17 @@ static bool change_security(struct bt_att_chan *chan, uint8_t ecode)
return bt_att_chan_set_security(chan, security);
}
-static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
+static int handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
ssize_t pdu_len, uint8_t *opcode)
{
struct bt_att *att = chan->att;
const struct bt_att_pdu_error_rsp *rsp;
struct att_send_op *op = chan->pending_req;
+ int should_retry = BT_ATT_RETRY_NO;
if (pdu_len != sizeof(*rsp)) {
*opcode = 0;
- return false;
+ return should_retry;
}
rsp = (void *) pdu;
@@ -797,11 +815,43 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
* the security again.
*/
if (op->retry)
- return false;
+ return should_retry;
/* Attempt to change security */
- if (!change_security(chan, rsp->ecode))
- return false;
+ if (change_security(chan, rsp->ecode)) {
+ should_retry = BT_ATT_RETRY_YES;
+ } else if (att->retry_callback) {
+ should_retry = att->retry_callback(op->opcode, rsp->ecode,
+ op->pdu + 1, op->len - 1,
+ op->id, att->retry_data);
+
+ /* Check if callback wants to defer the retry decision */
+ if (should_retry == BT_ATT_RETRY_PENDING) {
+ op->retry_pending = true;
+
+ /* Store error PDU for later use */
+ op->error_pdu = malloc(pdu_len);
+ if (op->error_pdu) {
+ memcpy(op->error_pdu, pdu, pdu_len);
+ op->error_pdu_len = pdu_len;
+ }
+
+ /* Remove timeout since we're waiting for approval */
+ if (op->timeout_id) {
+ timeout_remove(op->timeout_id);
+ op->timeout_id = 0;
+ }
+
+ /* Move from pending_req to pending_retry */
+ chan->pending_retry = op;
+
+ DBG(att, "(chan %p) Retry pending for operation %p",
+ chan, op);
+ }
+ }
+
+ if (should_retry != BT_ATT_RETRY_YES)
+ return should_retry;
/* Remove timeout_id if outstanding */
if (op->timeout_id) {
@@ -815,7 +865,8 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
op->retry = true;
/* Push operation back to channel queue */
- return queue_push_head(chan->queue, op);
+ return queue_push_head(chan->queue, op) ?
+ BT_ATT_RETRY_YES : BT_ATT_RETRY_NO;
}
static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
@@ -845,9 +896,15 @@ static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
*/
if (opcode == BT_ATT_OP_ERROR_RSP) {
/* Return if error response cause a retry */
- if (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
+ switch (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
+ case BT_ATT_RETRY_PENDING:
+ /* Operation moved to pending_retry, clear pending_req */
+ chan->pending_req = NULL;
+ case BT_ATT_RETRY_YES:
wakeup_chan_writer(chan, NULL);
return;
+ default:
+ break;
}
} else if (!(req_opcode = get_req_opcode(opcode)))
goto fail;
@@ -1142,6 +1199,9 @@ static void bt_att_free(struct bt_att *att)
if (att->timeout_destroy)
att->timeout_destroy(att->timeout_data);
+ if (att->retry_destroy)
+ att->retry_destroy(att->retry_data);
+
if (att->debug_destroy)
att->debug_destroy(att->debug_data);
@@ -1473,6 +1533,23 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
return true;
}
+bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
+ void *user_data,
+ bt_att_destroy_func_t destroy)
+{
+ if (!att)
+ return false;
+
+ if (att->retry_destroy)
+ att->retry_destroy(att->retry_data);
+
+ att->retry_callback = callback;
+ att->retry_destroy = destroy;
+ att->retry_data = user_data;
+
+ return true;
+}
+
unsigned int bt_att_register_disconnect(struct bt_att *att,
bt_att_disconnect_func_t callback,
void *user_data,
@@ -2051,6 +2128,97 @@ bool bt_att_has_crypto(struct bt_att *att)
return att->crypto ? true : false;
}
+bool bt_att_retry_request(struct bt_att *att, unsigned int id)
+{
+ const struct queue_entry *entry;
+ struct bt_att_chan *chan = NULL;
+ struct att_send_op *op;
+
+ if (!att || !id)
+ return false;
+
+ /* Find the channel with the pending retry operation */
+ for (entry = queue_get_entries(att->chans); entry;
+ entry = entry->next) {
+ struct bt_att_chan *c = entry->data;
+
+ if (c->pending_retry && c->pending_retry->id == id &&
+ c->pending_retry->retry_pending) {
+ chan = c;
+ op = c->pending_retry;
+ break;
+ }
+ }
+
+ if (!chan || !op)
+ return false;
+
+ DBG(att, "(chan %p) Approving retry for operation %p", chan, op);
+
+ /* Clear pending retry state and mark for retry */
+ op->retry_pending = false;
+ op->retry = true;
+ chan->pending_retry = NULL;
+
+ /* Free stored error PDU as we're retrying */
+ free(op->error_pdu);
+ op->error_pdu = NULL;
+ op->error_pdu_len = 0;
+
+ /* Push operation back to channel queue for retry */
+ if (!queue_push_head(chan->queue, op))
+ return false;
+
+ /* Wake up writer to send the retry */
+ wakeup_chan_writer(chan, NULL);
+
+ return true;
+}
+
+bool bt_att_cancel_retry(struct bt_att *att, unsigned int id)
+{
+ const struct queue_entry *entry;
+ struct bt_att_chan *chan = NULL;
+ struct att_send_op *op;
+
+ if (!att || !id)
+ return false;
+
+ /* Find the channel with the pending retry operation */
+ for (entry = queue_get_entries(att->chans); entry;
+ entry = entry->next) {
+ struct bt_att_chan *c = entry->data;
+
+ if (c->pending_retry && c->pending_retry->id == id &&
+ c->pending_retry->retry_pending) {
+ chan = c;
+ op = c->pending_retry;
+ break;
+ }
+ }
+
+ if (!chan || !op)
+ return false;
+
+ DBG(att, "(chan %p) Canceling retry for operation %p", chan, op);
+
+ /* Clear pending retry state */
+ op->retry_pending = false;
+ chan->pending_retry = NULL;
+
+ /* Call the callback with stored error PDU to notify upper layer */
+ if (op->callback)
+ op->callback(BT_ATT_OP_ERROR_RSP, op->error_pdu,
+ op->error_pdu_len, op->user_data);
+
+ destroy_att_send_op(op);
+
+ /* Wake up writer in case there are other operations */
+ wakeup_chan_writer(chan, NULL);
+
+ return true;
+}
+
bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry)
{
struct att_send_op *op;
diff --git a/src/shared/att.h b/src/shared/att.h
index 53a3f7a2a..6dc9944bb 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -46,6 +46,15 @@ typedef void (*bt_att_disconnect_func_t)(int err, void *user_data);
typedef void (*bt_att_exchange_func_t)(uint16_t mtu, void *user_data);
typedef bool (*bt_att_counter_func_t)(uint32_t *sign_cnt, void *user_data);
+/* Return values for bt_att_retry_func_t */
+#define BT_ATT_RETRY_NO 0 /* Don't retry */
+#define BT_ATT_RETRY_YES 1 /* Retry immediately */
+#define BT_ATT_RETRY_PENDING 2 /* Defer retry decision */
+
+typedef int (*bt_att_retry_func_t)(uint8_t opcode, uint8_t error_code,
+ const void *pdu, uint16_t length,
+ unsigned int att_id, void *user_data);
+
bool bt_att_set_debug(struct bt_att *att, uint8_t level,
bt_att_debug_func_t callback, void *user_data,
bt_att_destroy_func_t destroy);
@@ -58,6 +67,13 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
void *user_data,
bt_att_destroy_func_t destroy);
+bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
+ void *user_data,
+ bt_att_destroy_func_t destroy);
+
+bool bt_att_retry_request(struct bt_att *att, unsigned int id);
+bool bt_att_cancel_retry(struct bt_att *att, unsigned int id);
+
unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
const void *pdu, uint16_t length,
bt_att_response_func_t callback,
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism
2026-01-21 8:38 [PATCH v3 0/2] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching Mengshi Wu
2026-01-21 8:38 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Mengshi Wu
@ 2026-01-21 8:38 ` Mengshi Wu
1 sibling, 0 replies; 9+ messages in thread
From: Mengshi Wu @ 2026-01-21 8:38 UTC (permalink / raw)
To: luiz.dentz
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz, Mengshi Wu
Implement automatic retry logic for GATT operations that fail with
DB_OUT_OF_SYNC error. When this error occurs, the client now:
- Reads and compares the remote Database Hash with the local cache
- Retries the failed operation if hashes match (database is in sync)
- Triggers service discovery if hashes differ or hash read fails
- Handles Service Changed indications during retry by checking if
the error handle falls within the affected range
Add retry callback infrastructure in gatt-helpers to support deferred
retry decisions, allowing async verification before retrying requests.
Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
---
src/shared/gatt-client.c | 168 ++++++++++++++++++++++++++++++++++++++
src/shared/gatt-helpers.c | 16 ++++
src/shared/gatt-helpers.h | 3 +
3 files changed, 187 insertions(+)
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index f8ebab3fa..bc8138e16 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -114,6 +114,10 @@ struct bt_gatt_client {
struct bt_gatt_request *discovery_req;
unsigned int mtu_req_id;
+
+ /* Pending retry operation for DB out of sync handling */
+ unsigned int pending_retry_att_id;
+ uint16_t pending_error_handle;
};
struct request {
@@ -2342,6 +2346,167 @@ static void att_disconnect_cb(int err, void *user_data)
notify_client_ready(client, false, 0);
}
+static bool is_handle_out_of_range(uint16_t handle, struct bt_gatt_client *client)
+{
+ bool handle_out_of_range = false;
+ uint16_t start_handle, end_handle;
+
+ if (handle) {
+ start_handle = bt_gatt_req_get_start_handle(
+ client->discovery_req);
+ end_handle = bt_gatt_req_get_end_handle(
+ client->discovery_req);
+
+ if (start_handle != 0 && end_handle != 0 &&
+ (handle < start_handle || handle > end_handle))
+ handle_out_of_range = true;
+ }
+
+ return handle_out_of_range;
+}
+
+static void db_hash_check_cb(bool success, uint8_t att_ecode,
+ struct bt_gatt_result *result,
+ void *user_data)
+{
+ struct bt_gatt_client *client = user_data;
+ struct gatt_db_attribute *hash_attr = NULL;
+ const uint8_t *local_hash = NULL;
+ const uint8_t *remote_hash;
+ uint16_t length, handle;
+ struct bt_gatt_iter iter;
+ bt_uuid_t uuid;
+ unsigned int att_id = client->pending_retry_att_id;
+ uint16_t pending_error_handle = client->pending_error_handle;
+ bool handle_out_of_range;
+
+ client->pending_retry_att_id = 0;
+ client->pending_error_handle = 0;
+
+ /* If a Service Changed indication is received at this stage, the
+ * pending request may be retried once we have verified that the
+ * affected attribute handle is not within the range impacted by
+ * the service change.
+ */
+ if (client->in_svc_chngd) {
+ handle_out_of_range =
+ is_handle_out_of_range(pending_error_handle, client);
+
+ if (handle_out_of_range) {
+ DBG(client, "Error handle not effected, approving retry");
+ bt_att_retry_request(client->att, att_id);
+ } else {
+ DBG(client, "Error handle is in range of svc chngd");
+ bt_att_cancel_retry(client->att, att_id);
+ }
+ return;
+ }
+
+ if (!att_id) {
+ DBG(client, "No pending retry operation");
+ return;
+ }
+
+ if (!success) {
+ DBG(client,
+ "Failed to read remote DB Hash, triggering full discovery");
+ goto trigger_discovery;
+ }
+
+ /* Extract hash value from result */
+ if (!result || !bt_gatt_iter_init(&iter, result))
+ goto trigger_discovery;
+
+ if (!bt_gatt_iter_next_read_by_type(&iter, &handle, &length,
+ &remote_hash))
+ goto trigger_discovery;
+
+ if (length != 16) {
+ DBG(client, "Invalid DB Hash length: %u", length);
+ goto trigger_discovery;
+ }
+
+ /* Get local hash from database */
+ bt_uuid16_create(&uuid, GATT_CHARAC_DB_HASH);
+ gatt_db_find_by_type(client->db, 0x0001, 0xffff, &uuid,
+ get_first_attribute, &hash_attr);
+
+ if (hash_attr) {
+ gatt_db_attribute_read(hash_attr, 0, BT_ATT_OP_READ_REQ, NULL,
+ db_hash_read_value_cb, &local_hash);
+ }
+
+ /* Compare hashes */
+ if (local_hash && !memcmp(local_hash, remote_hash, 16)) {
+ /* Hashes match - safe to retry */
+ DBG(client, "DB Hash matches, approving retry");
+ bt_att_retry_request(client->att, att_id);
+ return;
+ }
+
+ /* Hashes differ - need service discovery */
+ DBG(client, "DB Hash differs, canceling retry and triggering discovery");
+
+trigger_discovery:
+ bt_att_cancel_retry(client->att, att_id);
+
+ if (!client->in_svc_chngd)
+ process_service_changed(client, 0x0001, 0xffff);
+}
+
+static int gatt_client_retry_cb(uint8_t opcode, uint8_t error_code,
+ const void *pdu, uint16_t length,
+ unsigned int att_id, void *user_data)
+{
+ struct bt_gatt_client *client = user_data;
+ bt_uuid_t uuid;
+ uint16_t error_handle = 0;
+ const struct bt_att_pdu_error_rsp *error_pdu;
+ bool handle_out_of_range = false;
+
+ assert(client);
+
+ /* Only handle DB_OUT_OF_SYNC errors */
+ if (error_code != BT_ATT_ERROR_DB_OUT_OF_SYNC)
+ return BT_ATT_RETRY_NO;
+
+ if (pdu && length >= sizeof(struct bt_att_pdu_error_rsp)) {
+ error_pdu = (void *)pdu;
+ error_handle = get_le16(&error_pdu->handle);
+ client->pending_error_handle = error_handle;
+ }
+
+ /* If a Service Changed indication is received at this stage, the
+ * pending request may be retried once we have verified that the
+ * affected attribute handle is not within the range impacted by
+ * the service change.
+ */
+ if (client->in_svc_chngd) {
+ handle_out_of_range =
+ is_handle_out_of_range(error_handle, client);
+
+ if (handle_out_of_range)
+ return BT_ATT_RETRY_YES;
+ else
+ return BT_ATT_RETRY_NO;
+ }
+
+ /* Store the att_id for later use */
+ client->pending_retry_att_id = att_id;
+
+ /* Read remote DB Hash to compare */
+ bt_uuid16_create(&uuid, GATT_CHARAC_DB_HASH);
+ if (!bt_gatt_read_by_type(client->att, 0x0001, 0xffff, &uuid,
+ db_hash_check_cb, client, NULL)) {
+ DBG(client, "Failed to read DB Hash, canceling retry");
+ client->pending_retry_att_id = 0;
+ client->pending_error_handle = 0;
+ return BT_ATT_RETRY_NO;
+ }
+
+ return BT_ATT_RETRY_PENDING;
+}
+
static struct bt_gatt_client *gatt_client_new(struct gatt_db *db,
struct bt_att *att,
uint8_t features)
@@ -2382,6 +2547,9 @@ static struct bt_gatt_client *gatt_client_new(struct gatt_db *db,
client->db = gatt_db_ref(db);
client->features = features;
+ /* Register retry callback for DB out of sync handling */
+ bt_att_set_retry_cb(att, gatt_client_retry_cb, client, NULL);
+
return client;
fail:
diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index c1cbbdc91..8dee34a9e 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -790,6 +790,22 @@ done:
discovery_op_complete(op, success, att_ecode);
}
+uint16_t bt_gatt_req_get_start_handle(struct bt_gatt_request *req)
+{
+ if (!req)
+ return 0;
+
+ return req->start_handle;
+}
+
+uint16_t bt_gatt_req_get_end_handle(struct bt_gatt_request *req)
+{
+ if (!req)
+ return 0;
+
+ return req->end_handle;
+}
+
static struct bt_gatt_request *discover_services(struct bt_att *att,
bt_uuid_t *uuid,
uint16_t start, uint16_t end,
diff --git a/src/shared/gatt-helpers.h b/src/shared/gatt-helpers.h
index 7623862e9..2bf5aad46 100644
--- a/src/shared/gatt-helpers.h
+++ b/src/shared/gatt-helpers.h
@@ -101,3 +101,6 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
bt_gatt_request_callback_t callback,
void *user_data,
bt_gatt_destroy_func_t destroy);
+
+uint16_t bt_gatt_req_get_end_handle(struct bt_gatt_request *req);
+uint16_t bt_gatt_req_get_start_handle(struct bt_gatt_request *req);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching.
2026-01-21 8:38 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Mengshi Wu
@ 2026-01-21 8:59 ` bluez.test.bot
2026-01-21 18:15 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Luiz Augusto von Dentz
1 sibling, 0 replies; 9+ messages in thread
From: bluez.test.bot @ 2026-01-21 8:59 UTC (permalink / raw)
To: linux-bluetooth, mengshi.wu
[-- Attachment #1: Type: text/plain, Size: 5208 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1045032
---Test result---
Test Summary:
CheckPatch PENDING 0.34 seconds
GitLint PENDING 0.32 seconds
BuildEll PASS 19.85 seconds
BluezMake FAIL 13.55 seconds
MakeCheck FAIL 25.40 seconds
MakeDistcheck PASS 240.00 seconds
CheckValgrind FAIL 11.82 seconds
CheckSmatch FAIL 14.07 seconds
bluezmakeextell FAIL 11.37 seconds
IncrementalBuild PENDING 0.36 seconds
ScanBuild FAIL 14.03 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: BluezMake - FAIL
Desc: Build BlueZ
Output:
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7859: src/shared/libshared_mainloop_la-att.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7586: src/shared/libshared_glib_la-att.lo] Error 1
make: *** [Makefile:11022: check] Error 2
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind
Output:
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7859: src/shared/libshared_mainloop_la-att.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:11022: check] Error 2
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
src/shared/crypto.c:271:21: warning: Variable length array is used.
src/shared/crypto.c:272:23: warning: Variable length array is used.
src/shared/gatt-helpers.c:768:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:846:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1339:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1370:23: warning: Variable length array is used.
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7859: src/shared/libshared_mainloop_la-att.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: bluezmakeextell - FAIL
Desc: Build Bluez with External ELL
Output:
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7859: src/shared/libshared_mainloop_la-att.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
##############################
Test: ScanBuild - FAIL
Desc: Run Scan Build
Output:
src/shared/att.c: In function ‘handle_rsp’:
src/shared/att.c:902:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
902 | chan->pending_req = NULL;
| ^
src/shared/att.c:903:3: note: here
903 | case BT_ATT_RETRY_YES:
| ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7859: src/shared/libshared_mainloop_la-att.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4243: all] Error 2
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support
2026-01-21 8:38 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Mengshi Wu
2026-01-21 8:59 ` gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching bluez.test.bot
@ 2026-01-21 18:15 ` Luiz Augusto von Dentz
2026-01-26 10:44 ` Mengshi Wu
1 sibling, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2026-01-21 18:15 UTC (permalink / raw)
To: Mengshi Wu
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz
Hi Mengshi,
On Wed, Jan 21, 2026 at 3:38 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
>
> Add a retry mechanism for ATT operations that allows upper layers to
> decide whether to retry failed requests. This includes:
>
> - Add retry callback registration (bt_att_set_retry_cb) to allow
> applications to handle retry decisions
> - Implement pending_retry state tracking in att_send_op to store
> operations awaiting retry approval
> - Add bt_att_retry_request() and bt_att_cancel_retry() functions to
> approve or reject retry attempts
> - Store error PDUs during retry_pending state for callback inspection
> - Modify handle_error_rsp() to return retry decision codes instead of
> boolean values
> - Add BT_ATT_RETRY_* constants for retry decision handling
> - Update GATT helpers to support retry callbacks for operations like
> discovery and read/write requests
>
> This enables more robust error handling by allowing the application
> layer to implement custom retry logic based on ATT error codes.
>
> Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
> ---
> src/shared/att.c | 182 +++++++++++++++++++++++++++++++++++++++++++++--
> src/shared/att.h | 16 +++++
> 2 files changed, 191 insertions(+), 7 deletions(-)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 77ca4aa24..4ae97530a 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -47,6 +47,7 @@ struct bt_att_chan {
>
> struct att_send_op *pending_req;
> struct att_send_op *pending_ind;
> + struct att_send_op *pending_retry;
> bool writer_active;
>
> bool in_req; /* There's a pending incoming request */
> @@ -78,6 +79,10 @@ struct bt_att {
> bt_att_destroy_func_t timeout_destroy;
> void *timeout_data;
>
> + bt_att_retry_func_t retry_callback;
> + bt_att_destroy_func_t retry_destroy;
> + void *retry_data;
Why do we need these fields? The bt_att_chan already have this data as
part of pending_retry, so this sound duplicated to me, or is this this
just to register for retry callback?
> +
> uint8_t debug_level;
> bt_att_debug_func_t debug_callback;
> bt_att_destroy_func_t debug_destroy;
> @@ -194,6 +199,9 @@ struct att_send_op {
> void *pdu;
> uint16_t len;
> bool retry;
> + bool retry_pending; /* Waiting for approval to retry */
> + uint8_t *error_pdu; /* Stored error PDU for retry_pending */
> + uint16_t error_pdu_len;
These fields are definitely not needed, juist reuse pdu, len and retry
fields, you are already adding a dedicated field for them in
bt_att_chan.pending_retry.
> bt_att_response_func_t callback;
> bt_att_destroy_func_t destroy;
> void *user_data;
> @@ -210,6 +218,7 @@ static void destroy_att_send_op(void *data)
> op->destroy(op->user_data);
>
> free(op->pdu);
> + free(op->error_pdu);
> free(op);
> }
>
> @@ -644,6 +653,9 @@ static void bt_att_chan_free(void *data)
> if (chan->pending_ind)
> destroy_att_send_op(chan->pending_ind);
>
> + if (chan->pending_retry)
> + destroy_att_send_op(chan->pending_retry);
> +
> queue_destroy(chan->queue, destroy_att_send_op);
>
> io_destroy(chan->io);
> @@ -682,6 +694,11 @@ static bool disconnect_cb(struct io *io, void *user_data)
> chan->pending_ind = NULL;
> }
>
> + if (chan->pending_retry) {
> + disc_att_send_op(chan->pending_retry);
> + chan->pending_retry = NULL;
> + }
> +
> bt_att_chan_free(chan);
>
> /* Don't run disconnect callback if there are channels left */
> @@ -777,16 +794,17 @@ static bool change_security(struct bt_att_chan *chan, uint8_t ecode)
> return bt_att_chan_set_security(chan, security);
> }
>
> -static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> +static int handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> ssize_t pdu_len, uint8_t *opcode)
> {
> struct bt_att *att = chan->att;
> const struct bt_att_pdu_error_rsp *rsp;
> struct att_send_op *op = chan->pending_req;
> + int should_retry = BT_ATT_RETRY_NO;
>
> if (pdu_len != sizeof(*rsp)) {
> *opcode = 0;
> - return false;
> + return should_retry;
> }
>
> rsp = (void *) pdu;
> @@ -797,11 +815,43 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> * the security again.
> */
> if (op->retry)
> - return false;
> + return should_retry;
>
> /* Attempt to change security */
> - if (!change_security(chan, rsp->ecode))
> - return false;
> + if (change_security(chan, rsp->ecode)) {
> + should_retry = BT_ATT_RETRY_YES;
> + } else if (att->retry_callback) {
> + should_retry = att->retry_callback(op->opcode, rsp->ecode,
> + op->pdu + 1, op->len - 1,
> + op->id, att->retry_data);
> +
> + /* Check if callback wants to defer the retry decision */
> + if (should_retry == BT_ATT_RETRY_PENDING) {
> + op->retry_pending = true;
> +
> + /* Store error PDU for later use */
> + op->error_pdu = malloc(pdu_len);
> + if (op->error_pdu) {
> + memcpy(op->error_pdu, pdu, pdu_len);
> + op->error_pdu_len = pdu_len;
> + }
> +
> + /* Remove timeout since we're waiting for approval */
> + if (op->timeout_id) {
> + timeout_remove(op->timeout_id);
> + op->timeout_id = 0;
> + }
> +
> + /* Move from pending_req to pending_retry */
> + chan->pending_retry = op;
> +
> + DBG(att, "(chan %p) Retry pending for operation %p",
> + chan, op);
> + }
> + }
> +
> + if (should_retry != BT_ATT_RETRY_YES)
> + return should_retry;
>
> /* Remove timeout_id if outstanding */
> if (op->timeout_id) {
> @@ -815,7 +865,8 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> op->retry = true;
>
> /* Push operation back to channel queue */
> - return queue_push_head(chan->queue, op);
> + return queue_push_head(chan->queue, op) ?
> + BT_ATT_RETRY_YES : BT_ATT_RETRY_NO;
> }
>
> static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
> @@ -845,9 +896,15 @@ static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
> */
> if (opcode == BT_ATT_OP_ERROR_RSP) {
> /* Return if error response cause a retry */
> - if (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
> + switch (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
> + case BT_ATT_RETRY_PENDING:
> + /* Operation moved to pending_retry, clear pending_req */
> + chan->pending_req = NULL;
> + case BT_ATT_RETRY_YES:
> wakeup_chan_writer(chan, NULL);
> return;
> + default:
> + break;
> }
> } else if (!(req_opcode = get_req_opcode(opcode)))
> goto fail;
> @@ -1142,6 +1199,9 @@ static void bt_att_free(struct bt_att *att)
> if (att->timeout_destroy)
> att->timeout_destroy(att->timeout_data);
>
> + if (att->retry_destroy)
> + att->retry_destroy(att->retry_data);
> +
> if (att->debug_destroy)
> att->debug_destroy(att->debug_data);
>
> @@ -1473,6 +1533,23 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
> return true;
> }
>
> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
> + void *user_data,
> + bt_att_destroy_func_t destroy)
> +{
> + if (!att)
> + return false;
> +
> + if (att->retry_destroy)
> + att->retry_destroy(att->retry_data);
> +
> + att->retry_callback = callback;
> + att->retry_destroy = destroy;
> + att->retry_data = user_data;
> +
> + return true;
> +}
> +
> unsigned int bt_att_register_disconnect(struct bt_att *att,
> bt_att_disconnect_func_t callback,
> void *user_data,
> @@ -2051,6 +2128,97 @@ bool bt_att_has_crypto(struct bt_att *att)
> return att->crypto ? true : false;
> }
>
> +bool bt_att_retry_request(struct bt_att *att, unsigned int id)
> +{
> + const struct queue_entry *entry;
> + struct bt_att_chan *chan = NULL;
> + struct att_send_op *op;
> +
> + if (!att || !id)
> + return false;
> +
> + /* Find the channel with the pending retry operation */
> + for (entry = queue_get_entries(att->chans); entry;
> + entry = entry->next) {
> + struct bt_att_chan *c = entry->data;
> +
> + if (c->pending_retry && c->pending_retry->id == id &&
> + c->pending_retry->retry_pending) {
> + chan = c;
> + op = c->pending_retry;
> + break;
> + }
> + }
> +
> + if (!chan || !op)
> + return false;
> +
> + DBG(att, "(chan %p) Approving retry for operation %p", chan, op);
> +
> + /* Clear pending retry state and mark for retry */
> + op->retry_pending = false;
> + op->retry = true;
> + chan->pending_retry = NULL;
> +
> + /* Free stored error PDU as we're retrying */
> + free(op->error_pdu);
> + op->error_pdu = NULL;
> + op->error_pdu_len = 0;
> +
> + /* Push operation back to channel queue for retry */
> + if (!queue_push_head(chan->queue, op))
> + return false;
> +
> + /* Wake up writer to send the retry */
> + wakeup_chan_writer(chan, NULL);
> +
> + return true;
> +}
> +
> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id)
> +{
> + const struct queue_entry *entry;
> + struct bt_att_chan *chan = NULL;
> + struct att_send_op *op;
> +
> + if (!att || !id)
> + return false;
> +
> + /* Find the channel with the pending retry operation */
> + for (entry = queue_get_entries(att->chans); entry;
> + entry = entry->next) {
> + struct bt_att_chan *c = entry->data;
> +
> + if (c->pending_retry && c->pending_retry->id == id &&
> + c->pending_retry->retry_pending) {
> + chan = c;
> + op = c->pending_retry;
> + break;
> + }
> + }
> +
> + if (!chan || !op)
> + return false;
> +
> + DBG(att, "(chan %p) Canceling retry for operation %p", chan, op);
> +
> + /* Clear pending retry state */
> + op->retry_pending = false;
> + chan->pending_retry = NULL;
> +
> + /* Call the callback with stored error PDU to notify upper layer */
> + if (op->callback)
> + op->callback(BT_ATT_OP_ERROR_RSP, op->error_pdu,
> + op->error_pdu_len, op->user_data);
> +
> + destroy_att_send_op(op);
> +
> + /* Wake up writer in case there are other operations */
> + wakeup_chan_writer(chan, NULL);
> +
> + return true;
> +}
> +
> bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry)
> {
> struct att_send_op *op;
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 53a3f7a2a..6dc9944bb 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -46,6 +46,15 @@ typedef void (*bt_att_disconnect_func_t)(int err, void *user_data);
> typedef void (*bt_att_exchange_func_t)(uint16_t mtu, void *user_data);
> typedef bool (*bt_att_counter_func_t)(uint32_t *sign_cnt, void *user_data);
>
> +/* Return values for bt_att_retry_func_t */
> +#define BT_ATT_RETRY_NO 0 /* Don't retry */
> +#define BT_ATT_RETRY_YES 1 /* Retry immediately */
> +#define BT_ATT_RETRY_PENDING 2 /* Defer retry decision */
> +
> +typedef int (*bt_att_retry_func_t)(uint8_t opcode, uint8_t error_code,
> + const void *pdu, uint16_t length,
> + unsigned int att_id, void *user_data);
> +
> bool bt_att_set_debug(struct bt_att *att, uint8_t level,
> bt_att_debug_func_t callback, void *user_data,
> bt_att_destroy_func_t destroy);
> @@ -58,6 +67,13 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
> void *user_data,
> bt_att_destroy_func_t destroy);
>
> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
> + void *user_data,
> + bt_att_destroy_func_t destroy);
> +
> +bool bt_att_retry_request(struct bt_att *att, unsigned int id);
> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id);
Hmm, why are you not reusing bt_att_resend and bt_att_cancel? We may
need to adapt bt_att_resend to locate the id in the pending_retry and
then force pushing to the queue head but other than that it looks
pretty similar to me, also I don't think it really needs to be sent
over the same channel, since over the air it really is another request
it can be enqueued in the session queue, rather than the channel
queue, just as done with bt_att_resend.
> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> const void *pdu, uint16_t length,
> bt_att_response_func_t callback,
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support
2026-01-21 18:15 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Luiz Augusto von Dentz
@ 2026-01-26 10:44 ` Mengshi Wu
2026-01-27 15:35 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Mengshi Wu @ 2026-01-26 10:44 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz
Hi Luiz,
Thanks for your comments.
On 1/22/2026 2:15 AM, Luiz Augusto von Dentz wrote:
> Hi Mengshi,
>
> On Wed, Jan 21, 2026 at 3:38 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
>>
>> Add a retry mechanism for ATT operations that allows upper layers to
>> decide whether to retry failed requests. This includes:
>>
>> - Add retry callback registration (bt_att_set_retry_cb) to allow
>> applications to handle retry decisions
>> - Implement pending_retry state tracking in att_send_op to store
>> operations awaiting retry approval
>> - Add bt_att_retry_request() and bt_att_cancel_retry() functions to
>> approve or reject retry attempts
>> - Store error PDUs during retry_pending state for callback inspection
>> - Modify handle_error_rsp() to return retry decision codes instead of
>> boolean values
>> - Add BT_ATT_RETRY_* constants for retry decision handling
>> - Update GATT helpers to support retry callbacks for operations like
>> discovery and read/write requests
>>
>> This enables more robust error handling by allowing the application
>> layer to implement custom retry logic based on ATT error codes.
>>
>> Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
>> ---
>> src/shared/att.c | 182 +++++++++++++++++++++++++++++++++++++++++++++--
>> src/shared/att.h | 16 +++++
>> 2 files changed, 191 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/shared/att.c b/src/shared/att.c
>> index 77ca4aa24..4ae97530a 100644
>> --- a/src/shared/att.c
>> +++ b/src/shared/att.c
>> @@ -47,6 +47,7 @@ struct bt_att_chan {
>>
>> struct att_send_op *pending_req;
>> struct att_send_op *pending_ind;
>> + struct att_send_op *pending_retry;
>> bool writer_active;
>>
>> bool in_req; /* There's a pending incoming request */
>> @@ -78,6 +79,10 @@ struct bt_att {
>> bt_att_destroy_func_t timeout_destroy;
>> void *timeout_data;
>>
>> + bt_att_retry_func_t retry_callback;
>> + bt_att_destroy_func_t retry_destroy;
>> + void *retry_data;
>
> Why do we need these fields? The bt_att_chan already have this data as
> part of pending_retry, so this sound duplicated to me, or is this this
> just to register for retry callback?
These fields serve different purposes. The callback in att_send_op
(pending_retry in bt_att_chan) is triggered too late to handle a deferred
retry. By the time this callback is executed, the retry opportunity has
already been lost. Therefore, I added this new callback that checks whether
a retry is pending beforehand.
>
>> +
>> uint8_t debug_level;
>> bt_att_debug_func_t debug_callback;
>> bt_att_destroy_func_t debug_destroy;
>> @@ -194,6 +199,9 @@ struct att_send_op {
>> void *pdu;
>> uint16_t len;
>> bool retry;
>> + bool retry_pending; /* Waiting for approval to retry */
>> + uint8_t *error_pdu; /* Stored error PDU for retry_pending */
>> + uint16_t error_pdu_len;
>
> These fields are definitely not needed, juist reuse pdu, len and retry
> fields, you are already adding a dedicated field for them in
> bt_att_chan.pending_retry.
These fields store the received error‑response PDU, not the original ATT
request PDU, which is already kept in the pdu/len fields. We need to keep
the error information because, if recovery fails, the error will be
propagated to the upper layer.
>
>> bt_att_response_func_t callback;
>> bt_att_destroy_func_t destroy;
>> void *user_data;
>> @@ -210,6 +218,7 @@ static void destroy_att_send_op(void *data)
>> op->destroy(op->user_data);
>>
>> free(op->pdu);
>> + free(op->error_pdu);
>> free(op);
>> }
>>
>> @@ -644,6 +653,9 @@ static void bt_att_chan_free(void *data)
>> if (chan->pending_ind)
>> destroy_att_send_op(chan->pending_ind);
>>
>> + if (chan->pending_retry)
>> + destroy_att_send_op(chan->pending_retry);
>> +
>> queue_destroy(chan->queue, destroy_att_send_op);
>>
>> io_destroy(chan->io);
>> @@ -682,6 +694,11 @@ static bool disconnect_cb(struct io *io, void *user_data)
>> chan->pending_ind = NULL;
>> }
>>
>> + if (chan->pending_retry) {
>> + disc_att_send_op(chan->pending_retry);
>> + chan->pending_retry = NULL;
>> + }
>> +
>> bt_att_chan_free(chan);
>>
>> /* Don't run disconnect callback if there are channels left */
>> @@ -777,16 +794,17 @@ static bool change_security(struct bt_att_chan *chan, uint8_t ecode)
>> return bt_att_chan_set_security(chan, security);
>> }
>>
>> -static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>> +static int handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>> ssize_t pdu_len, uint8_t *opcode)
>> {
>> struct bt_att *att = chan->att;
>> const struct bt_att_pdu_error_rsp *rsp;
>> struct att_send_op *op = chan->pending_req;
>> + int should_retry = BT_ATT_RETRY_NO;
>>
>> if (pdu_len != sizeof(*rsp)) {
>> *opcode = 0;
>> - return false;
>> + return should_retry;
>> }
>>
>> rsp = (void *) pdu;
>> @@ -797,11 +815,43 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>> * the security again.
>> */
>> if (op->retry)
>> - return false;
>> + return should_retry;
>>
>> /* Attempt to change security */
>> - if (!change_security(chan, rsp->ecode))
>> - return false;
>> + if (change_security(chan, rsp->ecode)) {
>> + should_retry = BT_ATT_RETRY_YES;
>> + } else if (att->retry_callback) {
>> + should_retry = att->retry_callback(op->opcode, rsp->ecode,
>> + op->pdu + 1, op->len - 1,
>> + op->id, att->retry_data);
>> +
>> + /* Check if callback wants to defer the retry decision */
>> + if (should_retry == BT_ATT_RETRY_PENDING) {
>> + op->retry_pending = true;
>> +
>> + /* Store error PDU for later use */
>> + op->error_pdu = malloc(pdu_len);
>> + if (op->error_pdu) {
>> + memcpy(op->error_pdu, pdu, pdu_len);
>> + op->error_pdu_len = pdu_len;
>> + }
>> +
>> + /* Remove timeout since we're waiting for approval */
>> + if (op->timeout_id) {
>> + timeout_remove(op->timeout_id);
>> + op->timeout_id = 0;
>> + }
>> +
>> + /* Move from pending_req to pending_retry */
>> + chan->pending_retry = op;
>> +
>> + DBG(att, "(chan %p) Retry pending for operation %p",
>> + chan, op);
>> + }
>> + }
>> +
>> + if (should_retry != BT_ATT_RETRY_YES)
>> + return should_retry;
>>
>> /* Remove timeout_id if outstanding */
>> if (op->timeout_id) {
>> @@ -815,7 +865,8 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>> op->retry = true;
>>
>> /* Push operation back to channel queue */
>> - return queue_push_head(chan->queue, op);
>> + return queue_push_head(chan->queue, op) ?
>> + BT_ATT_RETRY_YES : BT_ATT_RETRY_NO;
>> }
>>
>> static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
>> @@ -845,9 +896,15 @@ static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
>> */
>> if (opcode == BT_ATT_OP_ERROR_RSP) {
>> /* Return if error response cause a retry */
>> - if (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
>> + switch (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
>> + case BT_ATT_RETRY_PENDING:
>> + /* Operation moved to pending_retry, clear pending_req */
>> + chan->pending_req = NULL;
>> + case BT_ATT_RETRY_YES:
>> wakeup_chan_writer(chan, NULL);
>> return;
>> + default:
>> + break;
>> }
>> } else if (!(req_opcode = get_req_opcode(opcode)))
>> goto fail;
>> @@ -1142,6 +1199,9 @@ static void bt_att_free(struct bt_att *att)
>> if (att->timeout_destroy)
>> att->timeout_destroy(att->timeout_data);
>>
>> + if (att->retry_destroy)
>> + att->retry_destroy(att->retry_data);
>> +
>> if (att->debug_destroy)
>> att->debug_destroy(att->debug_data);
>>
>> @@ -1473,6 +1533,23 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
>> return true;
>> }
>>
>> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
>> + void *user_data,
>> + bt_att_destroy_func_t destroy)
>> +{
>> + if (!att)
>> + return false;
>> +
>> + if (att->retry_destroy)
>> + att->retry_destroy(att->retry_data);
>> +
>> + att->retry_callback = callback;
>> + att->retry_destroy = destroy;
>> + att->retry_data = user_data;
>> +
>> + return true;
>> +}
>> +
>> unsigned int bt_att_register_disconnect(struct bt_att *att,
>> bt_att_disconnect_func_t callback,
>> void *user_data,
>> @@ -2051,6 +2128,97 @@ bool bt_att_has_crypto(struct bt_att *att)
>> return att->crypto ? true : false;
>> }
>>
>> +bool bt_att_retry_request(struct bt_att *att, unsigned int id)
>> +{
>> + const struct queue_entry *entry;
>> + struct bt_att_chan *chan = NULL;
>> + struct att_send_op *op;
>> +
>> + if (!att || !id)
>> + return false;
>> +
>> + /* Find the channel with the pending retry operation */
>> + for (entry = queue_get_entries(att->chans); entry;
>> + entry = entry->next) {
>> + struct bt_att_chan *c = entry->data;
>> +
>> + if (c->pending_retry && c->pending_retry->id == id &&
>> + c->pending_retry->retry_pending) {
>> + chan = c;
>> + op = c->pending_retry;
>> + break;
>> + }
>> + }
>> +
>> + if (!chan || !op)
>> + return false;
>> +
>> + DBG(att, "(chan %p) Approving retry for operation %p", chan, op);
>> +
>> + /* Clear pending retry state and mark for retry */
>> + op->retry_pending = false;
>> + op->retry = true;
>> + chan->pending_retry = NULL;
>> +
>> + /* Free stored error PDU as we're retrying */
>> + free(op->error_pdu);
>> + op->error_pdu = NULL;
>> + op->error_pdu_len = 0;
>> +
>> + /* Push operation back to channel queue for retry */
>> + if (!queue_push_head(chan->queue, op))
>> + return false;
>> +
>> + /* Wake up writer to send the retry */
>> + wakeup_chan_writer(chan, NULL);
>> +
>> + return true;
>> +}
>> +
>> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id)
>> +{
>> + const struct queue_entry *entry;
>> + struct bt_att_chan *chan = NULL;
>> + struct att_send_op *op;
>> +
>> + if (!att || !id)
>> + return false;
>> +
>> + /* Find the channel with the pending retry operation */
>> + for (entry = queue_get_entries(att->chans); entry;
>> + entry = entry->next) {
>> + struct bt_att_chan *c = entry->data;
>> +
>> + if (c->pending_retry && c->pending_retry->id == id &&
>> + c->pending_retry->retry_pending) {
>> + chan = c;
>> + op = c->pending_retry;
>> + break;
>> + }
>> + }
>> +
>> + if (!chan || !op)
>> + return false;
>> +
>> + DBG(att, "(chan %p) Canceling retry for operation %p", chan, op);
>> +
>> + /* Clear pending retry state */
>> + op->retry_pending = false;
>> + chan->pending_retry = NULL;
>> +
>> + /* Call the callback with stored error PDU to notify upper layer */
>> + if (op->callback)
>> + op->callback(BT_ATT_OP_ERROR_RSP, op->error_pdu,
>> + op->error_pdu_len, op->user_data);
>> +
>> + destroy_att_send_op(op);
>> +
>> + /* Wake up writer in case there are other operations */
>> + wakeup_chan_writer(chan, NULL);
>> +
>> + return true;
>> +}
>> +
>> bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry)
>> {
>> struct att_send_op *op;
>> diff --git a/src/shared/att.h b/src/shared/att.h
>> index 53a3f7a2a..6dc9944bb 100644
>> --- a/src/shared/att.h
>> +++ b/src/shared/att.h
>> @@ -46,6 +46,15 @@ typedef void (*bt_att_disconnect_func_t)(int err, void *user_data);
>> typedef void (*bt_att_exchange_func_t)(uint16_t mtu, void *user_data);
>> typedef bool (*bt_att_counter_func_t)(uint32_t *sign_cnt, void *user_data);
>>
>> +/* Return values for bt_att_retry_func_t */
>> +#define BT_ATT_RETRY_NO 0 /* Don't retry */
>> +#define BT_ATT_RETRY_YES 1 /* Retry immediately */
>> +#define BT_ATT_RETRY_PENDING 2 /* Defer retry decision */
>> +
>> +typedef int (*bt_att_retry_func_t)(uint8_t opcode, uint8_t error_code,
>> + const void *pdu, uint16_t length,
>> + unsigned int att_id, void *user_data);
>> +
>> bool bt_att_set_debug(struct bt_att *att, uint8_t level,
>> bt_att_debug_func_t callback, void *user_data,
>> bt_att_destroy_func_t destroy);
>> @@ -58,6 +67,13 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
>> void *user_data,
>> bt_att_destroy_func_t destroy);
>>
>> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
>> + void *user_data,
>> + bt_att_destroy_func_t destroy);
>> +
>> +bool bt_att_retry_request(struct bt_att *att, unsigned int id);
>> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id);
>
> Hmm, why are you not reusing bt_att_resend and bt_att_cancel? We may
> need to adapt bt_att_resend to locate the id in the pending_retry and
> then force pushing to the queue head but other than that it looks
> pretty similar to me, also I don't think it really needs to be sent
> over the same channel, since over the air it really is another request
> it can be enqueued in the session queue, rather than the channel
> queue, just as done with bt_att_resend.
>
I did not reuse bt_att_resend() because it requires the original PDU and
respose callback as parameters, and these are not stored in the upper layer.
As discussed earlier, we moved these parts into att.c so that we can reuse
the relevant logic there.
bt_att_cancel() can be reused with a few adaptations. I will remove
bt_att_cancel_retry().
It does't need to be sent over the same channel, I just want to reuse an
existing op. I noticed that handle_error_rsp() does this.
>> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
>> const void *pdu, uint16_t length,
>> bt_att_response_func_t callback,
>> --
>> 2.34.1
>>
>
>
Best regards,
Mengshi Wu
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support
2026-01-26 10:44 ` Mengshi Wu
@ 2026-01-27 15:35 ` Luiz Augusto von Dentz
2026-02-02 2:46 ` Mengshi Wu
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2026-01-27 15:35 UTC (permalink / raw)
To: Mengshi Wu
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz
Hi,
On Mon, Jan 26, 2026 at 5:44 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
>
> Hi Luiz,
>
> Thanks for your comments.
>
> On 1/22/2026 2:15 AM, Luiz Augusto von Dentz wrote:
> > Hi Mengshi,
> >
> > On Wed, Jan 21, 2026 at 3:38 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
> >>
> >> Add a retry mechanism for ATT operations that allows upper layers to
> >> decide whether to retry failed requests. This includes:
> >>
> >> - Add retry callback registration (bt_att_set_retry_cb) to allow
> >> applications to handle retry decisions
> >> - Implement pending_retry state tracking in att_send_op to store
> >> operations awaiting retry approval
> >> - Add bt_att_retry_request() and bt_att_cancel_retry() functions to
> >> approve or reject retry attempts
> >> - Store error PDUs during retry_pending state for callback inspection
> >> - Modify handle_error_rsp() to return retry decision codes instead of
> >> boolean values
> >> - Add BT_ATT_RETRY_* constants for retry decision handling
> >> - Update GATT helpers to support retry callbacks for operations like
> >> discovery and read/write requests
> >>
> >> This enables more robust error handling by allowing the application
> >> layer to implement custom retry logic based on ATT error codes.
> >>
> >> Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
> >> ---
> >> src/shared/att.c | 182 +++++++++++++++++++++++++++++++++++++++++++++--
> >> src/shared/att.h | 16 +++++
> >> 2 files changed, 191 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/src/shared/att.c b/src/shared/att.c
> >> index 77ca4aa24..4ae97530a 100644
> >> --- a/src/shared/att.c
> >> +++ b/src/shared/att.c
> >> @@ -47,6 +47,7 @@ struct bt_att_chan {
> >>
> >> struct att_send_op *pending_req;
> >> struct att_send_op *pending_ind;
> >> + struct att_send_op *pending_retry;
> >> bool writer_active;
> >>
> >> bool in_req; /* There's a pending incoming request */
> >> @@ -78,6 +79,10 @@ struct bt_att {
> >> bt_att_destroy_func_t timeout_destroy;
> >> void *timeout_data;
> >>
> >> + bt_att_retry_func_t retry_callback;
> >> + bt_att_destroy_func_t retry_destroy;
> >> + void *retry_data;
> >
> > Why do we need these fields? The bt_att_chan already have this data as
> > part of pending_retry, so this sound duplicated to me, or is this this
> > just to register for retry callback?
>
> These fields serve different purposes. The callback in att_send_op
> (pending_retry in bt_att_chan) is triggered too late to handle a deferred
> retry. By the time this callback is executed, the retry opportunity has
> already been lost. Therefore, I added this new callback that checks whether
> a retry is pending beforehand.
Don't really understand, it seems to me that the retry callback here
is not used for retrying but instead to check if a request needs
retrying by upper layer (gatt-client).
> >
> >> +
> >> uint8_t debug_level;
> >> bt_att_debug_func_t debug_callback;
> >> bt_att_destroy_func_t debug_destroy;
> >> @@ -194,6 +199,9 @@ struct att_send_op {
> >> void *pdu;
> >> uint16_t len;
> >> bool retry;
> >> + bool retry_pending; /* Waiting for approval to retry */
> >> + uint8_t *error_pdu; /* Stored error PDU for retry_pending */
> >> + uint16_t error_pdu_len;
> >
> > These fields are definitely not needed, juist reuse pdu, len and retry
> > fields, you are already adding a dedicated field for them in
> > bt_att_chan.pending_retry.
>
> These fields store the received error‑response PDU, not the original ATT
> request PDU, which is already kept in the pdu/len fields. We need to keep
> the error information because, if recovery fails, the error will be
> propagated to the upper layer.
Well, afaik there is only one error that needs this sort of handling,
so this is suboptimal at best, I would assume if could just use the
chan.pending_db_sync to store the pending op and that already tell
exactly what it is for, rather than using a generic retry mechanism
which we don't know we will need in the future.
>
> >
> >> bt_att_response_func_t callback;
> >> bt_att_destroy_func_t destroy;
> >> void *user_data;
> >> @@ -210,6 +218,7 @@ static void destroy_att_send_op(void *data)
> >> op->destroy(op->user_data);
> >>
> >> free(op->pdu);
> >> + free(op->error_pdu);
> >> free(op);
> >> }
> >>
> >> @@ -644,6 +653,9 @@ static void bt_att_chan_free(void *data)
> >> if (chan->pending_ind)
> >> destroy_att_send_op(chan->pending_ind);
> >>
> >> + if (chan->pending_retry)
> >> + destroy_att_send_op(chan->pending_retry);
> >> +
> >> queue_destroy(chan->queue, destroy_att_send_op);
> >>
> >> io_destroy(chan->io);
> >> @@ -682,6 +694,11 @@ static bool disconnect_cb(struct io *io, void *user_data)
> >> chan->pending_ind = NULL;
> >> }
> >>
> >> + if (chan->pending_retry) {
> >> + disc_att_send_op(chan->pending_retry);
> >> + chan->pending_retry = NULL;
> >> + }
> >> +
> >> bt_att_chan_free(chan);
> >>
> >> /* Don't run disconnect callback if there are channels left */
> >> @@ -777,16 +794,17 @@ static bool change_security(struct bt_att_chan *chan, uint8_t ecode)
> >> return bt_att_chan_set_security(chan, security);
> >> }
> >>
> >> -static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> >> +static int handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> >> ssize_t pdu_len, uint8_t *opcode)
> >> {
> >> struct bt_att *att = chan->att;
> >> const struct bt_att_pdu_error_rsp *rsp;
> >> struct att_send_op *op = chan->pending_req;
> >> + int should_retry = BT_ATT_RETRY_NO;
> >>
> >> if (pdu_len != sizeof(*rsp)) {
> >> *opcode = 0;
> >> - return false;
> >> + return should_retry;
> >> }
> >>
> >> rsp = (void *) pdu;
> >> @@ -797,11 +815,43 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> >> * the security again.
> >> */
> >> if (op->retry)
> >> - return false;
> >> + return should_retry;
> >>
> >> /* Attempt to change security */
> >> - if (!change_security(chan, rsp->ecode))
> >> - return false;
> >> + if (change_security(chan, rsp->ecode)) {
> >> + should_retry = BT_ATT_RETRY_YES;
> >> + } else if (att->retry_callback) {
> >> + should_retry = att->retry_callback(op->opcode, rsp->ecode,
> >> + op->pdu + 1, op->len - 1,
> >> + op->id, att->retry_data);
> >> +
> >> + /* Check if callback wants to defer the retry decision */
> >> + if (should_retry == BT_ATT_RETRY_PENDING) {
> >> + op->retry_pending = true;
> >> +
> >> + /* Store error PDU for later use */
> >> + op->error_pdu = malloc(pdu_len);
> >> + if (op->error_pdu) {
> >> + memcpy(op->error_pdu, pdu, pdu_len);
> >> + op->error_pdu_len = pdu_len;
> >> + }
> >> +
> >> + /* Remove timeout since we're waiting for approval */
> >> + if (op->timeout_id) {
> >> + timeout_remove(op->timeout_id);
> >> + op->timeout_id = 0;
> >> + }
> >> +
> >> + /* Move from pending_req to pending_retry */
> >> + chan->pending_retry = op;
> >> +
> >> + DBG(att, "(chan %p) Retry pending for operation %p",
> >> + chan, op);
> >> + }
> >> + }
> >> +
> >> + if (should_retry != BT_ATT_RETRY_YES)
> >> + return should_retry;
> >>
> >> /* Remove timeout_id if outstanding */
> >> if (op->timeout_id) {
> >> @@ -815,7 +865,8 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
> >> op->retry = true;
> >>
> >> /* Push operation back to channel queue */
> >> - return queue_push_head(chan->queue, op);
> >> + return queue_push_head(chan->queue, op) ?
> >> + BT_ATT_RETRY_YES : BT_ATT_RETRY_NO;
> >> }
> >>
> >> static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
> >> @@ -845,9 +896,15 @@ static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
> >> */
> >> if (opcode == BT_ATT_OP_ERROR_RSP) {
> >> /* Return if error response cause a retry */
> >> - if (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
> >> + switch (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
> >> + case BT_ATT_RETRY_PENDING:
> >> + /* Operation moved to pending_retry, clear pending_req */
> >> + chan->pending_req = NULL;
> >> + case BT_ATT_RETRY_YES:
> >> wakeup_chan_writer(chan, NULL);
> >> return;
> >> + default:
> >> + break;
> >> }
> >> } else if (!(req_opcode = get_req_opcode(opcode)))
> >> goto fail;
> >> @@ -1142,6 +1199,9 @@ static void bt_att_free(struct bt_att *att)
> >> if (att->timeout_destroy)
> >> att->timeout_destroy(att->timeout_data);
> >>
> >> + if (att->retry_destroy)
> >> + att->retry_destroy(att->retry_data);
> >> +
> >> if (att->debug_destroy)
> >> att->debug_destroy(att->debug_data);
> >>
> >> @@ -1473,6 +1533,23 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
> >> return true;
> >> }
> >>
> >> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
> >> + void *user_data,
> >> + bt_att_destroy_func_t destroy)
> >> +{
> >> + if (!att)
> >> + return false;
> >> +
> >> + if (att->retry_destroy)
> >> + att->retry_destroy(att->retry_data);
> >> +
> >> + att->retry_callback = callback;
> >> + att->retry_destroy = destroy;
> >> + att->retry_data = user_data;
> >> +
> >> + return true;
> >> +}
> >> +
> >> unsigned int bt_att_register_disconnect(struct bt_att *att,
> >> bt_att_disconnect_func_t callback,
> >> void *user_data,
> >> @@ -2051,6 +2128,97 @@ bool bt_att_has_crypto(struct bt_att *att)
> >> return att->crypto ? true : false;
> >> }
> >>
> >> +bool bt_att_retry_request(struct bt_att *att, unsigned int id)
> >> +{
> >> + const struct queue_entry *entry;
> >> + struct bt_att_chan *chan = NULL;
> >> + struct att_send_op *op;
> >> +
> >> + if (!att || !id)
> >> + return false;
> >> +
> >> + /* Find the channel with the pending retry operation */
> >> + for (entry = queue_get_entries(att->chans); entry;
> >> + entry = entry->next) {
> >> + struct bt_att_chan *c = entry->data;
> >> +
> >> + if (c->pending_retry && c->pending_retry->id == id &&
> >> + c->pending_retry->retry_pending) {
> >> + chan = c;
> >> + op = c->pending_retry;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (!chan || !op)
> >> + return false;
> >> +
> >> + DBG(att, "(chan %p) Approving retry for operation %p", chan, op);
> >> +
> >> + /* Clear pending retry state and mark for retry */
> >> + op->retry_pending = false;
> >> + op->retry = true;
> >> + chan->pending_retry = NULL;
> >> +
> >> + /* Free stored error PDU as we're retrying */
> >> + free(op->error_pdu);
> >> + op->error_pdu = NULL;
> >> + op->error_pdu_len = 0;
> >> +
> >> + /* Push operation back to channel queue for retry */
> >> + if (!queue_push_head(chan->queue, op))
> >> + return false;
> >> +
> >> + /* Wake up writer to send the retry */
> >> + wakeup_chan_writer(chan, NULL);
> >> +
> >> + return true;
> >> +}
> >> +
> >> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id)
> >> +{
> >> + const struct queue_entry *entry;
> >> + struct bt_att_chan *chan = NULL;
> >> + struct att_send_op *op;
> >> +
> >> + if (!att || !id)
> >> + return false;
> >> +
> >> + /* Find the channel with the pending retry operation */
> >> + for (entry = queue_get_entries(att->chans); entry;
> >> + entry = entry->next) {
> >> + struct bt_att_chan *c = entry->data;
> >> +
> >> + if (c->pending_retry && c->pending_retry->id == id &&
> >> + c->pending_retry->retry_pending) {
> >> + chan = c;
> >> + op = c->pending_retry;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (!chan || !op)
> >> + return false;
> >> +
> >> + DBG(att, "(chan %p) Canceling retry for operation %p", chan, op);
> >> +
> >> + /* Clear pending retry state */
> >> + op->retry_pending = false;
> >> + chan->pending_retry = NULL;
> >> +
> >> + /* Call the callback with stored error PDU to notify upper layer */
> >> + if (op->callback)
> >> + op->callback(BT_ATT_OP_ERROR_RSP, op->error_pdu,
> >> + op->error_pdu_len, op->user_data);
> >> +
> >> + destroy_att_send_op(op);
> >> +
> >> + /* Wake up writer in case there are other operations */
> >> + wakeup_chan_writer(chan, NULL);
> >> +
> >> + return true;
> >> +}
> >> +
> >> bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry)
> >> {
> >> struct att_send_op *op;
> >> diff --git a/src/shared/att.h b/src/shared/att.h
> >> index 53a3f7a2a..6dc9944bb 100644
> >> --- a/src/shared/att.h
> >> +++ b/src/shared/att.h
> >> @@ -46,6 +46,15 @@ typedef void (*bt_att_disconnect_func_t)(int err, void *user_data);
> >> typedef void (*bt_att_exchange_func_t)(uint16_t mtu, void *user_data);
> >> typedef bool (*bt_att_counter_func_t)(uint32_t *sign_cnt, void *user_data);
> >>
> >> +/* Return values for bt_att_retry_func_t */
> >> +#define BT_ATT_RETRY_NO 0 /* Don't retry */
> >> +#define BT_ATT_RETRY_YES 1 /* Retry immediately */
> >> +#define BT_ATT_RETRY_PENDING 2 /* Defer retry decision */
> >> +
> >> +typedef int (*bt_att_retry_func_t)(uint8_t opcode, uint8_t error_code,
> >> + const void *pdu, uint16_t length,
> >> + unsigned int att_id, void *user_data);
> >> +
> >> bool bt_att_set_debug(struct bt_att *att, uint8_t level,
> >> bt_att_debug_func_t callback, void *user_data,
> >> bt_att_destroy_func_t destroy);
> >> @@ -58,6 +67,13 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
> >> void *user_data,
> >> bt_att_destroy_func_t destroy);
> >>
> >> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
> >> + void *user_data,
> >> + bt_att_destroy_func_t destroy);
> >> +
> >> +bool bt_att_retry_request(struct bt_att *att, unsigned int id);
> >> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id);
> >
> > Hmm, why are you not reusing bt_att_resend and bt_att_cancel? We may
> > need to adapt bt_att_resend to locate the id in the pending_retry and
> > then force pushing to the queue head but other than that it looks
> > pretty similar to me, also I don't think it really needs to be sent
> > over the same channel, since over the air it really is another request
> > it can be enqueued in the session queue, rather than the channel
> > queue, just as done with bt_att_resend.
> >
>
> I did not reuse bt_att_resend() because it requires the original PDU and
> respose callback as parameters, and these are not stored in the upper layer.
> As discussed earlier, we moved these parts into att.c so that we can reuse
> the relevant logic there.
Well you can still use it internally though, instead of duplicating
most of its logic in a new function, anyway why does it needs to be
called from upper layer? I though the retry_callback would be used to
communicate to the upper layer about an error and then depending on
the return of the callback we could resend the request directly in
att.c.
Maybe something like the following works better:
typedef bool (*bt_att_out_of_sync_func_t)(uint8_t opcode,
const void *pdu, uint16_t length,
unsigned int att_id, void *user_data);
So if the upper layer return true it means the request should be
resend, otherwise bt_att_response_func_t should be called.
> bt_att_cancel() can be reused with a few adaptations. I will remove
> bt_att_cancel_retry().
>
> It does't need to be sent over the same channel, I just want to reuse an
> existing op. I noticed that handle_error_rsp() does this.
>
> >> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> >> const void *pdu, uint16_t length,
> >> bt_att_response_func_t callback,
> >> --
> >> 2.34.1
> >>
> >
> >
>
> Best regards,
> Mengshi Wu
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support
2026-01-27 15:35 ` Luiz Augusto von Dentz
@ 2026-02-02 2:46 ` Mengshi Wu
0 siblings, 0 replies; 9+ messages in thread
From: Mengshi Wu @ 2026-02-02 2:46 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth, shuai.zhang, cheng.jiang, chezhou, wei.deng,
yiboz
Hi Luiz,
On 1/27/2026 11:35 PM, Luiz Augusto von Dentz wrote:
> Hi,
>
> On Mon, Jan 26, 2026 at 5:44 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
>>
>> Hi Luiz,
>>
>> Thanks for your comments.
>>
>> On 1/22/2026 2:15 AM, Luiz Augusto von Dentz wrote:
>>> Hi Mengshi,
>>>
>>> On Wed, Jan 21, 2026 at 3:38 AM Mengshi Wu <mengshi.wu@oss.qualcomm.com> wrote:
>>>>
>>>> Add a retry mechanism for ATT operations that allows upper layers to
>>>> decide whether to retry failed requests. This includes:
>>>>
>>>> - Add retry callback registration (bt_att_set_retry_cb) to allow
>>>> applications to handle retry decisions
>>>> - Implement pending_retry state tracking in att_send_op to store
>>>> operations awaiting retry approval
>>>> - Add bt_att_retry_request() and bt_att_cancel_retry() functions to
>>>> approve or reject retry attempts
>>>> - Store error PDUs during retry_pending state for callback inspection
>>>> - Modify handle_error_rsp() to return retry decision codes instead of
>>>> boolean values
>>>> - Add BT_ATT_RETRY_* constants for retry decision handling
>>>> - Update GATT helpers to support retry callbacks for operations like
>>>> discovery and read/write requests
>>>>
>>>> This enables more robust error handling by allowing the application
>>>> layer to implement custom retry logic based on ATT error codes.
>>>>
>>>> Signed-off-by: Mengshi Wu <mengshi.wu@oss.qualcomm.com>
>>>> ---
>>>> src/shared/att.c | 182 +++++++++++++++++++++++++++++++++++++++++++++--
>>>> src/shared/att.h | 16 +++++
>>>> 2 files changed, 191 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>> index 77ca4aa24..4ae97530a 100644
>>>> --- a/src/shared/att.c
>>>> +++ b/src/shared/att.c
>>>> @@ -47,6 +47,7 @@ struct bt_att_chan {
>>>>
>>>> struct att_send_op *pending_req;
>>>> struct att_send_op *pending_ind;
>>>> + struct att_send_op *pending_retry;
>>>> bool writer_active;
>>>>
>>>> bool in_req; /* There's a pending incoming request */
>>>> @@ -78,6 +79,10 @@ struct bt_att {
>>>> bt_att_destroy_func_t timeout_destroy;
>>>> void *timeout_data;
>>>>
>>>> + bt_att_retry_func_t retry_callback;
>>>> + bt_att_destroy_func_t retry_destroy;
>>>> + void *retry_data;
>>>
>>> Why do we need these fields? The bt_att_chan already have this data as
>>> part of pending_retry, so this sound duplicated to me, or is this this
>>> just to register for retry callback?
>>
>> These fields serve different purposes. The callback in att_send_op
>> (pending_retry in bt_att_chan) is triggered too late to handle a deferred
>> retry. By the time this callback is executed, the retry opportunity has
>> already been lost. Therefore, I added this new callback that checks whether
>> a retry is pending beforehand.
>
> Don't really understand, it seems to me that the retry callback here
> is not used for retrying but instead to check if a request needs
> retrying by upper layer (gatt-client).
>
retry_callback is used for validation, not for performing the retry itself.
I should rename it to avoid confusion. As we discussed earlier, we need additional
information—such as the affected handle range and the DB hash value—to determine
whether an ATT request can be recovered. This is why I introduced this callback:
it allows the GATT layer to verify the DB hash and affected handle range when
available.
>>>
>>>> +
>>>> uint8_t debug_level;
>>>> bt_att_debug_func_t debug_callback;
>>>> bt_att_destroy_func_t debug_destroy;
>>>> @@ -194,6 +199,9 @@ struct att_send_op {
>>>> void *pdu;
>>>> uint16_t len;
>>>> bool retry;
>>>> + bool retry_pending; /* Waiting for approval to retry */
>>>> + uint8_t *error_pdu; /* Stored error PDU for retry_pending */
>>>> + uint16_t error_pdu_len;
>>>
>>> These fields are definitely not needed, juist reuse pdu, len and retry
>>> fields, you are already adding a dedicated field for them in
>>> bt_att_chan.pending_retry.
>>
>> These fields store the received error‑response PDU, not the original ATT
>> request PDU, which is already kept in the pdu/len fields. We need to keep
>> the error information because, if recovery fails, the error will be
>> propagated to the upper layer.
>
> Well, afaik there is only one error that needs this sort of handling,
> so this is suboptimal at best, I would assume if could just use the
> chan.pending_db_sync to store the pending op and that already tell
> exactly what it is for, rather than using a generic retry mechanism
> which we don't know we will need in the future.
>
OK, this may not be a common retry mechanism. Should I pack them into a
new structure (such as chan.pending_db_sync)?
>>
>>>
>>>> bt_att_response_func_t callback;
>>>> bt_att_destroy_func_t destroy;
>>>> void *user_data;
>>>> @@ -210,6 +218,7 @@ static void destroy_att_send_op(void *data)
>>>> op->destroy(op->user_data);
>>>>
>>>> free(op->pdu);
>>>> + free(op->error_pdu);
>>>> free(op);
>>>> }
>>>>
>>>> @@ -644,6 +653,9 @@ static void bt_att_chan_free(void *data)
>>>> if (chan->pending_ind)
>>>> destroy_att_send_op(chan->pending_ind);
>>>>
>>>> + if (chan->pending_retry)
>>>> + destroy_att_send_op(chan->pending_retry);
>>>> +
>>>> queue_destroy(chan->queue, destroy_att_send_op);
>>>>
>>>> io_destroy(chan->io);
>>>> @@ -682,6 +694,11 @@ static bool disconnect_cb(struct io *io, void *user_data)
>>>> chan->pending_ind = NULL;
>>>> }
>>>>
>>>> + if (chan->pending_retry) {
>>>> + disc_att_send_op(chan->pending_retry);
>>>> + chan->pending_retry = NULL;
>>>> + }
>>>> +
>>>> bt_att_chan_free(chan);
>>>>
>>>> /* Don't run disconnect callback if there are channels left */
>>>> @@ -777,16 +794,17 @@ static bool change_security(struct bt_att_chan *chan, uint8_t ecode)
>>>> return bt_att_chan_set_security(chan, security);
>>>> }
>>>>
>>>> -static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>>>> +static int handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>>>> ssize_t pdu_len, uint8_t *opcode)
>>>> {
>>>> struct bt_att *att = chan->att;
>>>> const struct bt_att_pdu_error_rsp *rsp;
>>>> struct att_send_op *op = chan->pending_req;
>>>> + int should_retry = BT_ATT_RETRY_NO;
>>>>
>>>> if (pdu_len != sizeof(*rsp)) {
>>>> *opcode = 0;
>>>> - return false;
>>>> + return should_retry;
>>>> }
>>>>
>>>> rsp = (void *) pdu;
>>>> @@ -797,11 +815,43 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>>>> * the security again.
>>>> */
>>>> if (op->retry)
>>>> - return false;
>>>> + return should_retry;
>>>>
>>>> /* Attempt to change security */
>>>> - if (!change_security(chan, rsp->ecode))
>>>> - return false;
>>>> + if (change_security(chan, rsp->ecode)) {
>>>> + should_retry = BT_ATT_RETRY_YES;
>>>> + } else if (att->retry_callback) {
>>>> + should_retry = att->retry_callback(op->opcode, rsp->ecode,
>>>> + op->pdu + 1, op->len - 1,
>>>> + op->id, att->retry_data);
>>>> +
>>>> + /* Check if callback wants to defer the retry decision */
>>>> + if (should_retry == BT_ATT_RETRY_PENDING) {
>>>> + op->retry_pending = true;
>>>> +
>>>> + /* Store error PDU for later use */
>>>> + op->error_pdu = malloc(pdu_len);
>>>> + if (op->error_pdu) {
>>>> + memcpy(op->error_pdu, pdu, pdu_len);
>>>> + op->error_pdu_len = pdu_len;
>>>> + }
>>>> +
>>>> + /* Remove timeout since we're waiting for approval */
>>>> + if (op->timeout_id) {
>>>> + timeout_remove(op->timeout_id);
>>>> + op->timeout_id = 0;
>>>> + }
>>>> +
>>>> + /* Move from pending_req to pending_retry */
>>>> + chan->pending_retry = op;
>>>> +
>>>> + DBG(att, "(chan %p) Retry pending for operation %p",
>>>> + chan, op);
>>>> + }
>>>> + }
>>>> +
>>>> + if (should_retry != BT_ATT_RETRY_YES)
>>>> + return should_retry;
>>>>
>>>> /* Remove timeout_id if outstanding */
>>>> if (op->timeout_id) {
>>>> @@ -815,7 +865,8 @@ static bool handle_error_rsp(struct bt_att_chan *chan, uint8_t *pdu,
>>>> op->retry = true;
>>>>
>>>> /* Push operation back to channel queue */
>>>> - return queue_push_head(chan->queue, op);
>>>> + return queue_push_head(chan->queue, op) ?
>>>> + BT_ATT_RETRY_YES : BT_ATT_RETRY_NO;
>>>> }
>>>>
>>>> static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
>>>> @@ -845,9 +896,15 @@ static void handle_rsp(struct bt_att_chan *chan, uint8_t opcode, uint8_t *pdu,
>>>> */
>>>> if (opcode == BT_ATT_OP_ERROR_RSP) {
>>>> /* Return if error response cause a retry */
>>>> - if (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
>>>> + switch (handle_error_rsp(chan, pdu, pdu_len, &req_opcode)) {
>>>> + case BT_ATT_RETRY_PENDING:
>>>> + /* Operation moved to pending_retry, clear pending_req */
>>>> + chan->pending_req = NULL;
>>>> + case BT_ATT_RETRY_YES:
>>>> wakeup_chan_writer(chan, NULL);
>>>> return;
>>>> + default:
>>>> + break;
>>>> }
>>>> } else if (!(req_opcode = get_req_opcode(opcode)))
>>>> goto fail;
>>>> @@ -1142,6 +1199,9 @@ static void bt_att_free(struct bt_att *att)
>>>> if (att->timeout_destroy)
>>>> att->timeout_destroy(att->timeout_data);
>>>>
>>>> + if (att->retry_destroy)
>>>> + att->retry_destroy(att->retry_data);
>>>> +
>>>> if (att->debug_destroy)
>>>> att->debug_destroy(att->debug_data);
>>>>
>>>> @@ -1473,6 +1533,23 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
>>>> return true;
>>>> }
>>>>
>>>> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
>>>> + void *user_data,
>>>> + bt_att_destroy_func_t destroy)
>>>> +{
>>>> + if (!att)
>>>> + return false;
>>>> +
>>>> + if (att->retry_destroy)
>>>> + att->retry_destroy(att->retry_data);
>>>> +
>>>> + att->retry_callback = callback;
>>>> + att->retry_destroy = destroy;
>>>> + att->retry_data = user_data;
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> unsigned int bt_att_register_disconnect(struct bt_att *att,
>>>> bt_att_disconnect_func_t callback,
>>>> void *user_data,
>>>> @@ -2051,6 +2128,97 @@ bool bt_att_has_crypto(struct bt_att *att)
>>>> return att->crypto ? true : false;
>>>> }
>>>>
>>>> +bool bt_att_retry_request(struct bt_att *att, unsigned int id)
>>>> +{
>>>> + const struct queue_entry *entry;
>>>> + struct bt_att_chan *chan = NULL;
>>>> + struct att_send_op *op;
>>>> +
>>>> + if (!att || !id)
>>>> + return false;
>>>> +
>>>> + /* Find the channel with the pending retry operation */
>>>> + for (entry = queue_get_entries(att->chans); entry;
>>>> + entry = entry->next) {
>>>> + struct bt_att_chan *c = entry->data;
>>>> +
>>>> + if (c->pending_retry && c->pending_retry->id == id &&
>>>> + c->pending_retry->retry_pending) {
>>>> + chan = c;
>>>> + op = c->pending_retry;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + if (!chan || !op)
>>>> + return false;
>>>> +
>>>> + DBG(att, "(chan %p) Approving retry for operation %p", chan, op);
>>>> +
>>>> + /* Clear pending retry state and mark for retry */
>>>> + op->retry_pending = false;
>>>> + op->retry = true;
>>>> + chan->pending_retry = NULL;
>>>> +
>>>> + /* Free stored error PDU as we're retrying */
>>>> + free(op->error_pdu);
>>>> + op->error_pdu = NULL;
>>>> + op->error_pdu_len = 0;
>>>> +
>>>> + /* Push operation back to channel queue for retry */
>>>> + if (!queue_push_head(chan->queue, op))
>>>> + return false;
>>>> +
>>>> + /* Wake up writer to send the retry */
>>>> + wakeup_chan_writer(chan, NULL);
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id)
>>>> +{
>>>> + const struct queue_entry *entry;
>>>> + struct bt_att_chan *chan = NULL;
>>>> + struct att_send_op *op;
>>>> +
>>>> + if (!att || !id)
>>>> + return false;
>>>> +
>>>> + /* Find the channel with the pending retry operation */
>>>> + for (entry = queue_get_entries(att->chans); entry;
>>>> + entry = entry->next) {
>>>> + struct bt_att_chan *c = entry->data;
>>>> +
>>>> + if (c->pending_retry && c->pending_retry->id == id &&
>>>> + c->pending_retry->retry_pending) {
>>>> + chan = c;
>>>> + op = c->pending_retry;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + if (!chan || !op)
>>>> + return false;
>>>> +
>>>> + DBG(att, "(chan %p) Canceling retry for operation %p", chan, op);
>>>> +
>>>> + /* Clear pending retry state */
>>>> + op->retry_pending = false;
>>>> + chan->pending_retry = NULL;
>>>> +
>>>> + /* Call the callback with stored error PDU to notify upper layer */
>>>> + if (op->callback)
>>>> + op->callback(BT_ATT_OP_ERROR_RSP, op->error_pdu,
>>>> + op->error_pdu_len, op->user_data);
>>>> +
>>>> + destroy_att_send_op(op);
>>>> +
>>>> + /* Wake up writer in case there are other operations */
>>>> + wakeup_chan_writer(chan, NULL);
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry)
>>>> {
>>>> struct att_send_op *op;
>>>> diff --git a/src/shared/att.h b/src/shared/att.h
>>>> index 53a3f7a2a..6dc9944bb 100644
>>>> --- a/src/shared/att.h
>>>> +++ b/src/shared/att.h
>>>> @@ -46,6 +46,15 @@ typedef void (*bt_att_disconnect_func_t)(int err, void *user_data);
>>>> typedef void (*bt_att_exchange_func_t)(uint16_t mtu, void *user_data);
>>>> typedef bool (*bt_att_counter_func_t)(uint32_t *sign_cnt, void *user_data);
>>>>
>>>> +/* Return values for bt_att_retry_func_t */
>>>> +#define BT_ATT_RETRY_NO 0 /* Don't retry */
>>>> +#define BT_ATT_RETRY_YES 1 /* Retry immediately */
>>>> +#define BT_ATT_RETRY_PENDING 2 /* Defer retry decision */
>>>> +
>>>> +typedef int (*bt_att_retry_func_t)(uint8_t opcode, uint8_t error_code,
>>>> + const void *pdu, uint16_t length,
>>>> + unsigned int att_id, void *user_data);
>>>> +
>>>> bool bt_att_set_debug(struct bt_att *att, uint8_t level,
>>>> bt_att_debug_func_t callback, void *user_data,
>>>> bt_att_destroy_func_t destroy);
>>>> @@ -58,6 +67,13 @@ bool bt_att_set_timeout_cb(struct bt_att *att, bt_att_timeout_func_t callback,
>>>> void *user_data,
>>>> bt_att_destroy_func_t destroy);
>>>>
>>>> +bool bt_att_set_retry_cb(struct bt_att *att, bt_att_retry_func_t callback,
>>>> + void *user_data,
>>>> + bt_att_destroy_func_t destroy);
>>>> +
>>>> +bool bt_att_retry_request(struct bt_att *att, unsigned int id);
>>>> +bool bt_att_cancel_retry(struct bt_att *att, unsigned int id);
>>>
>>> Hmm, why are you not reusing bt_att_resend and bt_att_cancel? We may
>>> need to adapt bt_att_resend to locate the id in the pending_retry and
>>> then force pushing to the queue head but other than that it looks
>>> pretty similar to me, also I don't think it really needs to be sent
>>> over the same channel, since over the air it really is another request
>>> it can be enqueued in the session queue, rather than the channel
>>> queue, just as done with bt_att_resend.
>>>
>>
>> I did not reuse bt_att_resend() because it requires the original PDU and
>> respose callback as parameters, and these are not stored in the upper layer.
>> As discussed earlier, we moved these parts into att.c so that we can reuse
>> the relevant logic there.
>
> Well you can still use it internally though, instead of duplicating
> most of its logic in a new function, anyway why does it needs to be
> called from upper layer? I though the retry_callback would be used to
> communicate to the upper layer about an error and then depending on
> the return of the callback we could resend the request directly in
> att.c.
>
> Maybe something like the following works better:
>
> typedef bool (*bt_att_out_of_sync_func_t)(uint8_t opcode,
> const void *pdu, uint16_t length,
> unsigned int att_id, void *user_data);
>
> So if the upper layer return true it means the request should be
> resend, otherwise bt_att_response_func_t should be called.
>
The upper layer may not be able to decide on a retry immediately. The GATT
layer cannot determine whether the failed handle is affected until it checks
the DB hash value or receives a service‑changed indication.
>> bt_att_cancel() can be reused with a few adaptations. I will remove
>> bt_att_cancel_retry().
>>
>> It does't need to be sent over the same channel, I just want to reuse an
>> existing op. I noticed that handle_error_rsp() does this.
>>
>>>> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
>>>> const void *pdu, uint16_t length,
>>>> bt_att_response_func_t callback,
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>>
>> Best regards,
>> Mengshi Wu
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-02-02 2:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-21 8:38 [PATCH v3 0/2] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching Mengshi Wu
2026-01-21 8:38 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Mengshi Wu
2026-01-21 8:59 ` gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching bluez.test.bot
2026-01-21 18:15 ` [PATCH v3 1/2] shared/att: Implement ATT error retry mechanism with callback support Luiz Augusto von Dentz
2026-01-26 10:44 ` Mengshi Wu
2026-01-27 15:35 ` Luiz Augusto von Dentz
2026-02-02 2:46 ` Mengshi Wu
2026-01-21 8:38 ` [PATCH v3 2/2] gatt-client: Add DB_OUT_OF_SYNC error handling with retry mechanism Mengshi Wu
-- strict thread matches above, loose matches on Subject: below --
2026-01-05 10:38 [PATCH v2 1/1] gatt-client:Implement error handling for DB_OUT_OF_SYNC in GATT caching Mengshi Wu
2026-01-05 10:54 ` bluez.test.bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox