Message-ID: <53f37700-29ee-4ae9-bf68-e37c317e55cd@amlogic.com>
Date: Mon, 27 Oct 2025 11:24:42 +0800
From: Yang Li <yang.li@...ogic.com>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: Marcel Holtmann <marcel@...tmann.org>,
 Johan Hedberg <johan.hedberg@...il.com>, linux-bluetooth@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Bluetooth: iso: fix socket matching ambiguity between
 BIS and CIS

Hi Luiz,


> [ EXTERNAL EMAIL ]
>
> Hi Yang,
>
> On Fri, Oct 24, 2025 at 9:45 AM Luiz Augusto von Dentz
> <luiz.dentz@...il.com> wrote:
>> Hi Yang,
>>
>> On Thu, Oct 23, 2025 at 11:47 PM Yang Li <yang.li@...ogic.com> wrote:
>>> Hi Luiz,
>>> A gentle ping, thanks.
>>>
>>>> Hi Luiz,
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Yang,
>>>>>
>>>>> On Sun, Aug 3, 2025 at 9:07 PM Yang Li <yang.li@...ogic.com> wrote:
>>>>>> Hi Luiz,
>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>
>>>>>>> Hi Yang,
>>>>>>>
>>>>>>> On Thu, Jul 31, 2025 at 4:00 AM Yang Li via B4 Relay
>>>>>>> <devnull+yang.li.amlogic.com@...nel.org> wrote:
>>>>>>>> From: Yang Li <yang.li@...ogic.com>
>>>>>>>>
>>>>>>>> When both BIS and CIS links exist, their sockets are in
>>>>>>>> the BT_LISTEN state.
>>>>>>> We probably need to introduce tests to iso-test that setup both then
>>>>>>> to avoid reintroducing the problem.
>>>>>> Since the coexistence of BIS sink and CIS sink is determined by
>>>>>> application-level logic, it may be difficult to reproduce this scenario
>>>>>> using iso-test.
>>>>> Looks like you haven't look at what iso-tester tools tests do, that is
>>>>> not tight to bluetoothd, it directly operates at the socket layer so
>>>>> we can create any scenario we want.
>>>>
>>>> Based on the current structure of iso-tester, it is not possible to
>>>> implement test cases where CIS and BIS listen simultaneously. There
>>>> are several issues:
>>>>
>>>> 1.
>>>>
>>>>     In struct iso_client_data, although both CIS and BIS related
>>>>     elements are defined, they are mutually exclusive. CIS and BIS
>>>>     cannot be used at the same time. For example, .bcast must explicitly
>>>>     specify whether it is broadcast or unicast.
>>>>
>>>> 2.
>>>>
>>>>     The setup_listen_many function also identifies BIS or CIS through
>>>>     .bcast.
>>>>
>>>> Therefore, if we want to add test cases for the coexistence of BIS and
>>>> CIS, the current data structure needs to be modified to completely
>>>> separate the elements for BIS and CIS.
>>>>
>>>>
>>> I'm not sure if my understanding is fully correct, so I would appreciate
>>> any guidance or suggestions.
>>>
>>> Based on my testing, this patch does fix the issue on my side.
>>> Please let me know if there is anything I may have missed or if further
>>> changes are needed.
>> I hope you are paying attention to the mailing list since I did add a
>> lot of new code that introduces support for PAST, including new test
>> cases for iso-tester, so I don't think asking for a test case for
>> having both BIS/CIS together is too much really. Works for me doesn't
>> really cut it since we do want to make sure we don't reintroduce the
>> problem later, but Im fine merging this now if it doesn't introduce
>> any problem existing tests in iso-tester.
> You will need to resend it since it is no longer available in patchwork.


I completely understand the necessity of adding a test case for the 
coexistence of BIS and CIS. However, the current issue is that the data 
structure of iso-tester doesn't support listening to both BIS and CIS at 
the same time. I will keep an eye on the updates of iso-tester and add 
the test case at the appropriate time.

I will update this patch.

Thanks

>
>>>>>> Do you have any suggestions on how to simulate or test this case more
>>>>>> effectively?
>>>>>>
>>>>>>>> dump sock:
>>>>>>>>      sk 000000001977ef51 state 6
>>>>>>>>      src 10:a5:62:31:05:cf dst 00:00:00:00:00:00
>>>>>>>>      sk 0000000031d28700 state 7
>>>>>>>>      src 10:a5:62:31:05:cf dst00:00:00:00:00:00
>>>>>>>>      sk 00000000613af00e state 4   # listen sock of bis
>>>>>>>>      src 10:a5:62:31:05:cf dst 54:00:00:d4:99:30
>>>>>>>>      sk 000000001710468c state 9
>>>>>>>>      src 10:a5:62:31:05:cf dst 54:00:00:d4:99:30
>>>>>>>>      sk 000000005d97dfde state 4   #listen sock of cis
>>>>>>>>      src 10:a5:62:31:05:cf dst 00:00:00:00:00:00
>>>>>>>>
>>>>>>>> To locate the CIS socket correctly, check both the BT_LISTEN
>>>>>>>> state and whether dst addr is BDADDR_ANY.
>>>>>>>>
>>>>>>>> Link: https://github.com/bluez/bluez/issues/1224
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Li <yang.li@...ogic.com>
>>>>>>>> ---
>>>>>>>>     net/bluetooth/iso.c | 9 +++++++--
>>>>>>>>     1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>>>>>>>> index eaffd25570e3..9a4dea03af8c 100644
>>>>>>>> --- a/net/bluetooth/iso.c
>>>>>>>> +++ b/net/bluetooth/iso.c
>>>>>>>> @@ -1919,6 +1919,11 @@ static bool iso_match_pa_sync_flag(struct
>>>>>>>> sock *sk, void *data)
>>>>>>>>            return test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static bool iso_match_dst(struct sock *sk, void *data)
>>>>>>>> +{
>>>>>>>> +       return !bacmp(&iso_pi(sk)->dst, (bdaddr_t *)data);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>     static void iso_conn_ready(struct iso_conn *conn)
>>>>>>>>     {
>>>>>>>>            struct sock *parent = NULL;
>>>>>>>> @@ -1981,7 +1986,7 @@ static void iso_conn_ready(struct iso_conn
>>>>>>>> *conn)
>>>>>>>>
>>>>>>>>                    if (!parent)
>>>>>>>>                            parent = iso_get_sock(&hcon->src,
>>>>>>>> BDADDR_ANY,
>>>>>>>> -                                             BT_LISTEN, NULL, NULL);
>>>>>>>> +                                             BT_LISTEN,
>>>>>>>> iso_match_dst, BDADDR_ANY);
>>>>>>>>
>>>>>>>>                    if (!parent)
>>>>>>>>                            return;
>>>>>>>> @@ -2220,7 +2225,7 @@ int iso_connect_ind(struct hci_dev *hdev,
>>>>>>>> bdaddr_t *bdaddr, __u8 *flags)
>>>>>>>>                    }
>>>>>>>>            } else {
>>>>>>>>                    sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY,
>>>>>>>> -                                 BT_LISTEN, NULL, NULL);
>>>>>>>> +                                 BT_LISTEN, iso_match_dst,
>>>>>>>> BDADDR_ANY);
>>>>>>> Perhaps we should add helper function that wrap the iso_get_sock (e.g.
>>>>>>> iso_get_sock_cis and iso_get_sock_bis) to make it clearer what is the
>>>>>>> expected socket type, also if the hcon has been set perhaps that
>>>>>>> should be matched as well with CIS_LINK/BIS_LINK, or perhaps we
>>>>>>> introduce a socket type to differentiate since the use of the address
>>>>>>> can make the logic a little confusing when the socket types are mixed
>>>>>>> together.
>>>>>>>
>>>>>>> Now looking at the source code perhaps we can have a separate list for
>>>>>>> cis and bis sockets instead of global iso_sk_list (e.g. cis_sk_list
>>>>>>> and bis_sk_list), that way we don't need a type and there is no risk
>>>>>>> of confusing the sockets since they would never be in the same list.
>>>>>> Alright, I will give it a try.
>>>>>>
>>>>>>>>            }
>>>>>>>>
>>>>>>>>     done:
>>>>>>>>
>>>>>>>> ---
>>>>>>>> base-commit: 9c533991fe15be60ad9f9a7629c25dbc5b09788d
>>>>>>>> change-id: 20250731-bis_cis_coexist-717a442d5c42
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> --
>>>>>>>> Yang Li <yang.li@...ogic.com>
>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>> Luiz Augusto von Dentz
>>>>>
>>>>> --
>>>>> Luiz Augusto von Dentz
>>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> --
> Luiz Augusto von Dentz

Powered by blists - more mailing lists