Bluetooth: CSIP: Fix ntf issue to clients on reboot

[niym-ot]

On reboot, client list to notify is not updated properly. Fix is to check and add the reconnected clients on security changed cb.

Subscription check is added before notify to clients. BT Enable check is added in the register function before adding bonded devices to client list.

Also typo is corrected in add_bonded_addr_to_client_list in the second loop.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

subsys/bluetooth/audio/csip_set_member.c:843

• The loop increment originally used 'i' instead of 'j', which could lead to incorrect iteration behavior. The fix correctly updates the loop to use 'j++' starting at index 0.

for (size_t j = 1U; j < ARRAY_SIZE(svc_inst->clients); i++) {

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

subsys/bluetooth/audio/csip_set_member.c:848

• [nitpick] Consider rephrasing the comment to improve clarity, for example: '/* Check if device is registered; if not, add it */'.

/* Check if device is registered, it not, add it */

[Thalley]

@niym-ot for the future, please, as per https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers, allow reviewers to resolve non-trivial comments :)

[Copilot]

Pull Request Overview

This PR addresses a bug in the CSIP client notification mechanism on reboot by ensuring that reconnected clients are properly added and notified. Key changes include:

• Inclusion of settings-related functionality with a new commit handler to restore the client list from bonded devices.
• Introduction of a global mutex (svc_inst_lock) to protect concurrent operations, particularly in notify and unregister functions.
• Correction of a typo in the iteration variable in add_bonded_addr_to_client_list.

[Thalley]

May be outside the scope of this PR, but it looks like that if CONFIG_BT_BONDABLE=n, then there's some parts of our code that shouldn't be run (should be guarded by CONFIG_BT_BONDABLE), but I think we can do that in a follow-up PR, but something like this probably should since it's only dealing with bonds at the moment

[Thalley]

Ideally we would add some sort of test for the mutex behavior. I'm not convinced that the current implement properly guards the service instance members, but on the other hand it's better than what it was, and not the main purpose of this PR.

I suggest to fix the comment about memset, and then follow up with another PR to ensure correct guarding of all svc_inst access, as I think there are more cases where we may memset memory while other functions in other threads assume that it is valid.

[Thalley]

The memset needs to be done while we hold the mutex, otherwise we can risk memsetting something that has just been initialized in bt_csip_set_member_register.

We shall, however, never memset the mutex. After memsetting the mutex then it will be broken, as the mutex becomes invalid and is not initialized again.

[niym-ot]

may be many approaches

1. store the mutex locally in function , memset the instance and save the mutex back to instance struct and unlock
2. init the mutex everytime you register (no need to save it )
3. keep the mutex out of the instance structure

which one you suggest ?

[Thalley]

4. Do not memset the entire struct. If we keep the mutex as the start or end of the struct, we can apply offsetof when memsetting to avoid memsetting that like we do in https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/audio/bap_iso.c#L36-L54
5. Alternatively we can reset instances more manually like https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/audio/bap_broadcast_assistant.c#L961-L1015

I think it makes the most sense to keep the mutex in the struct. I think option 2 can work nicely.

So I'd go for option 2, 4 or 5, where 2 is arguably semantically the best (and also the simplest)

[niym-ot]

went with option 4

[Thalley]

I think we are fixing the issue that the PR sets out to.

Arguably all the mutex stuff should not have been part of this, as it requires a much larger change to function properly, and is only partially implemented by this PR.

I suggest to try to get this merged to fix the primary issue, and then we can work towards fixing the mutex stuff in a follow-up PR.

[Thalley]

Do we actually need to take the mutex in the register function?

The caller doesn't have a reference to the instance at this point, and there cannot be an activities on the instance at this point, so I don't see any way for multiple threads to access the instance at this point.

[niym-ot]

two threads can call register function and instance_cnt gets updated . can lead to undefined behaviour

[niym-ot]

Follow up PR created
#89087

[Thalley]

I noticed a bug in the way we use instance_cnt, so I've also created #89089 to fix that