[PATCH v2 1/3] Bluetooth: ISO: Don't initiate CIS connections if there are no buffers

[PATCH v2 1/3] Bluetooth: ISO: Don't initiate CIS connections if there are no buffers

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If the controller has no buffers left return -ENOBUFF to indicate that
iso_cnt might be out of sync.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/iso.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 5ce823ca3aaf..dff3fc4917d6 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -458,6 +458,13 @@ static int iso_connect_cis(struct sock *sk)
 		goto unlock;
 	}
 
+	/* Check if there are available buffers for output/TX. */
+	if (iso_pi(sk)->qos.ucast.out.sdu && !hci_conn_num(hdev, CIS_LINK) &&
+	    (hdev->iso_pkts && !hdev->iso_cnt)) {
+		err = -ENOBUFS;
+		goto unlock;
+	}
+
 	/* Just bind if DEFER_SETUP has been set */
 	if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
 		hcon = hci_bind_cis(hdev, &iso_pi(sk)->dst,
-- 
2.50.1

[PATCH v2 2/3] Bluetooth: HCI: Fix using LE/ACL buffers for ISO packets

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

ISO packets shall not use LE/ACL buffer pool, that feature seem to be
exclusive to LE-ACL only.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c  | 26 +++++++++++---------------
 net/bluetooth/hci_core.c  |  3 +--
 net/bluetooth/hci_event.c | 16 +++-------------
 3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 7a879290dd28..e6cea51b3eab 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1152,28 +1152,24 @@ void hci_conn_del(struct hci_conn *conn)
 	disable_delayed_work_sync(&conn->auto_accept_work);
 	disable_delayed_work_sync(&conn->idle_work);
 
-	if (conn->type == ACL_LINK) {
-		/* Unacked frames */
+	/* Handle unnacked frames */
+	switch (conn->type) {
+	case ACL_LINK:
 		hdev->acl_cnt += conn->sent;
-	} else if (conn->type == LE_LINK) {
+		break;
+	case LE_LINK:
 		cancel_delayed_work(&conn->le_conn_timeout);
 
 		if (hdev->le_pkts)
 			hdev->le_cnt += conn->sent;
 		else
 			hdev->acl_cnt += conn->sent;
-	} else {
-		/* Unacked ISO frames */
-		if (conn->type == CIS_LINK ||
-		    conn->type == BIS_LINK ||
-		    conn->type == PA_LINK) {
-			if (hdev->iso_pkts)
-				hdev->iso_cnt += conn->sent;
-			else if (hdev->le_pkts)
-				hdev->le_cnt += conn->sent;
-			else
-				hdev->acl_cnt += conn->sent;
-		}
+		break;
+	case CIS_LINK:
+	case BIS_LINK:
+	case PA_LINK:
+		hdev->iso_cnt += conn->sent;
+		break;
 	}
 
 	skb_queue_purge(&conn->data_q);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 55e0722fd066..eb09f80a3243 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3759,8 +3759,7 @@ static void hci_sched_iso(struct hci_dev *hdev, __u8 type)
 	if (!hci_conn_num(hdev, type))
 		return;
 
-	cnt = hdev->iso_pkts ? &hdev->iso_cnt :
-		hdev->le_pkts ? &hdev->le_cnt : &hdev->acl_cnt;
+	cnt = &hdev->iso_cnt;
 	while (*cnt && (conn = hci_low_sent(hdev, type, &quote))) {
 		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
 			BT_DBG("skb %p len %d", skb, skb->len);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index fe7cdd67ad2a..1686680a38c8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4433,19 +4433,9 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
 		case CIS_LINK:
 		case BIS_LINK:
 		case PA_LINK:
-			if (hdev->iso_pkts) {
-				hdev->iso_cnt += count;
-				if (hdev->iso_cnt > hdev->iso_pkts)
-					hdev->iso_cnt = hdev->iso_pkts;
-			} else if (hdev->le_pkts) {
-				hdev->le_cnt += count;
-				if (hdev->le_cnt > hdev->le_pkts)
-					hdev->le_cnt = hdev->le_pkts;
-			} else {
-				hdev->acl_cnt += count;
-				if (hdev->acl_cnt > hdev->acl_pkts)
-					hdev->acl_cnt = hdev->acl_pkts;
-			}
+			hdev->iso_cnt += count;
+			if (hdev->iso_cnt > hdev->iso_pkts)
+				hdev->iso_cnt = hdev->iso_pkts;
 			break;
 
 		default:
-- 
2.50.1

[PATCH v2 3/3] Bluetooth: hci_conn: Make unacked packet handling more robust

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This attempts to make unacked packet handling more robust by detecting
if there are no connections left then restore all buffers of the
respective pool.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e6cea51b3eab..4bd2e4cd477f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1152,22 +1152,44 @@ void hci_conn_del(struct hci_conn *conn)
 	disable_delayed_work_sync(&conn->auto_accept_work);
 	disable_delayed_work_sync(&conn->idle_work);
 
-	/* Handle unnacked frames */
+	/* Handle unnacked frames:
+	 *
+	 * - In case there are no connection restore all buffers to the pool
+	 * - Otherwise restore just the buffers considered in transit for the
+	 *   hci_conn
+	 */
 	switch (conn->type) {
 	case ACL_LINK:
-		hdev->acl_cnt += conn->sent;
+		if (!hci_conn_num(hdev, ACL_LINK))
+			hdev->acl_cnt = hdev->acl_pkts;
+		else
+			hdev->acl_cnt += conn->sent;
 		break;
 	case LE_LINK:
 		cancel_delayed_work(&conn->le_conn_timeout);
 
-		if (hdev->le_pkts)
-			hdev->le_cnt += conn->sent;
-		else
-			hdev->acl_cnt += conn->sent;
+		if (hdev->le_pkts) {
+			if (!hci_conn_num(hdev, LE_LINK))
+				hdev->le_cnt = hdev->le_pkts;
+			else
+				hdev->le_cnt += conn->sent;
+		} else {
+			if (!hci_conn_num(hdev, LE_LINK) &&
+			    !hci_conn_num(hdev, ACL_LINK))
+				hdev->acl_cnt = hdev->acl_pkts;
+			else
+				hdev->acl_cnt += conn->sent;
+		}
 		break;
 	case CIS_LINK:
 	case BIS_LINK:
 	case PA_LINK:
+		if (!hci_conn_num(hdev, CIS_LINK) &&
+		    !hci_conn_num(hdev, BIS_LINK) &&
+		    !hci_conn_num(hdev, PA_LINK))
+			hdev->iso_cnt = hdev->iso_pkts;
+		else
+			hdev->iso_cnt += conn->sent;
 		hdev->iso_cnt += conn->sent;
 		break;
 	}
-- 
2.50.1

Re: [PATCH v2 3/3] Bluetooth: hci_conn: Make unacked packet handling more robust

[Paul Menzel]

Dear Luiz,


Thank you for your patch.

Am 12.08.25 um 21:53 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This attempts to make unacked packet handling more robust by detecting
> if there are no connections left then restore all buffers of the
> respective pool.

Did you find this in code review, or was a bug reported?

> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>   net/bluetooth/hci_conn.c | 34 ++++++++++++++++++++++++++++------
>   1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index e6cea51b3eab..4bd2e4cd477f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1152,22 +1152,44 @@ void hci_conn_del(struct hci_conn *conn)
>   	disable_delayed_work_sync(&conn->auto_accept_work);
>   	disable_delayed_work_sync(&conn->idle_work);
>   
> -	/* Handle unnacked frames */
> +	/* Handle unnacked frames:
> +	 *
> +	 * - In case there are no connection restore all buffers to the pool
> +	 * - Otherwise restore just the buffers considered in transit for the
> +	 *   hci_conn
> +	 */
>   	switch (conn->type) {
>   	case ACL_LINK:
> -		hdev->acl_cnt += conn->sent;
> +		if (!hci_conn_num(hdev, ACL_LINK))
> +			hdev->acl_cnt = hdev->acl_pkts;
> +		else
> +			hdev->acl_cnt += conn->sent;
>   		break;
>   	case LE_LINK:
>   		cancel_delayed_work(&conn->le_conn_timeout);
>   
> -		if (hdev->le_pkts)
> -			hdev->le_cnt += conn->sent;
> -		else
> -			hdev->acl_cnt += conn->sent;
> +		if (hdev->le_pkts) {
> +			if (!hci_conn_num(hdev, LE_LINK))
> +				hdev->le_cnt = hdev->le_pkts;
> +			else
> +				hdev->le_cnt += conn->sent;
> +		} else {
> +			if (!hci_conn_num(hdev, LE_LINK) &&
> +			    !hci_conn_num(hdev, ACL_LINK))
> +				hdev->acl_cnt = hdev->acl_pkts;
> +			else
> +				hdev->acl_cnt += conn->sent;
> +		}
>   		break;
>   	case CIS_LINK:
>   	case BIS_LINK:
>   	case PA_LINK:
> +		if (!hci_conn_num(hdev, CIS_LINK) &&
> +		    !hci_conn_num(hdev, BIS_LINK) &&
> +		    !hci_conn_num(hdev, PA_LINK))
> +			hdev->iso_cnt = hdev->iso_pkts;
> +		else
> +			hdev->iso_cnt += conn->sent;
>   		hdev->iso_cnt += conn->sent;
>   		break;
>   	}


Kind regards,

Paul

Re: [PATCH v2 3/3] Bluetooth: hci_conn: Make unacked packet handling more robust

[Luiz Augusto von Dentz]

Hi Paul,

On Tue, Aug 12, 2025 at 4:03 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Luiz,
>
>
> Thank you for your patch.
>
> Am 12.08.25 um 21:53 schrieb Luiz Augusto von Dentz:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This attempts to make unacked packet handling more robust by detecting
> > if there are no connections left then restore all buffers of the
> > respective pool.
>
> Did you find this in code review, or was a bug reported?

Internal testing has run into a problem where some packets are never
TX for some reason, we are still accessing if this is going to help
but it is not an actual fix and more of a way to improve the handling
of unacked packets.

Now I'm curious, why this sort of question seem to be recurring, we do
encourage as much details as possible, but I don't think we ever said
that it is required to disclose what sort of test or bug report it
came from, especially if they come from internal testing which cannot
be referenced anyway, so was there anything unclear with the commit
message?

> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >   net/bluetooth/hci_conn.c | 34 ++++++++++++++++++++++++++++------
> >   1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index e6cea51b3eab..4bd2e4cd477f 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1152,22 +1152,44 @@ void hci_conn_del(struct hci_conn *conn)
> >       disable_delayed_work_sync(&conn->auto_accept_work);
> >       disable_delayed_work_sync(&conn->idle_work);
> >
> > -     /* Handle unnacked frames */
> > +     /* Handle unnacked frames:
> > +      *
> > +      * - In case there are no connection restore all buffers to the pool
> > +      * - Otherwise restore just the buffers considered in transit for the
> > +      *   hci_conn
> > +      */
> >       switch (conn->type) {
> >       case ACL_LINK:
> > -             hdev->acl_cnt += conn->sent;
> > +             if (!hci_conn_num(hdev, ACL_LINK))
> > +                     hdev->acl_cnt = hdev->acl_pkts;
> > +             else
> > +                     hdev->acl_cnt += conn->sent;
> >               break;
> >       case LE_LINK:
> >               cancel_delayed_work(&conn->le_conn_timeout);
> >
> > -             if (hdev->le_pkts)
> > -                     hdev->le_cnt += conn->sent;
> > -             else
> > -                     hdev->acl_cnt += conn->sent;
> > +             if (hdev->le_pkts) {
> > +                     if (!hci_conn_num(hdev, LE_LINK))
> > +                             hdev->le_cnt = hdev->le_pkts;
> > +                     else
> > +                             hdev->le_cnt += conn->sent;
> > +             } else {
> > +                     if (!hci_conn_num(hdev, LE_LINK) &&
> > +                         !hci_conn_num(hdev, ACL_LINK))
> > +                             hdev->acl_cnt = hdev->acl_pkts;
> > +                     else
> > +                             hdev->acl_cnt += conn->sent;
> > +             }
> >               break;
> >       case CIS_LINK:
> >       case BIS_LINK:
> >       case PA_LINK:
> > +             if (!hci_conn_num(hdev, CIS_LINK) &&
> > +                 !hci_conn_num(hdev, BIS_LINK) &&
> > +                 !hci_conn_num(hdev, PA_LINK))
> > +                     hdev->iso_cnt = hdev->iso_pkts;
> > +             else
> > +                     hdev->iso_cnt += conn->sent;
> >               hdev->iso_cnt += conn->sent;
> >               break;
> >       }
>
>
> Kind regards,
>
> Paul



-- 
Luiz Augusto von Dentz

RE: [v2,1/3] Bluetooth: ISO: Don't initiate CIS connections if there are no buffers

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2992 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=990709

---Test result---

Test Summary:
CheckPatch                    PENDING   0.35 seconds
GitLint                       PENDING   0.27 seconds
SubjectPrefix                 PASS      0.34 seconds
BuildKernel                   PASS      23.92 seconds
CheckAllWarning               PASS      26.53 seconds
CheckSparse                   WARNING   29.98 seconds
BuildKernel32                 PASS      23.96 seconds
TestRunnerSetup               PASS      476.08 seconds
TestRunner_l2cap-tester       PASS      24.64 seconds
TestRunner_iso-tester         PASS      40.47 seconds
TestRunner_bnep-tester        PASS      5.92 seconds
TestRunner_mgmt-tester        FAIL      129.29 seconds
TestRunner_rfcomm-tester      PASS      9.30 seconds
TestRunner_sco-tester         PASS      14.50 seconds
TestRunner_ioctl-tester       PASS      9.93 seconds
TestRunner_mesh-tester        FAIL      11.50 seconds
TestRunner_smp-tester         PASS      8.60 seconds
TestRunner_userchan-tester    PASS      6.19 seconds
IncrementalBuild              PENDING   0.46 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_core.c:85:9: warning: context imbalance in '__hci_dev_get' - different lock contexts for basic blocknet/bluetooth/hci_core.c: note: in included file (through include/linux/notifier.h, include/linux/memory_hotplug.h, include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, include/linux/radix-tree.h, ...):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4

Failed Test Cases
Add Ext Advertising - Success 22 (LE -> off, Remove) Failed       0.126 seconds
Set Device ID - Power off and Power on               Failed       0.110 seconds
Set Device ID - SSP off and Power on                 Failed       0.112 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.107 seconds
Mesh - Send cancel - 2                               Timed out    1.998 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v2 3/3] Bluetooth: hci_conn: Make unacked packet handling more robust

[Paul Menzel]

Dear Luiz,


Am 12.08.25 um 22:29 schrieb Luiz Augusto von Dentz:

> On Tue, Aug 12, 2025 at 4:03 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:

>> Am 12.08.25 um 21:53 schrieb Luiz Augusto von Dentz:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This attempts to make unacked packet handling more robust by detecting
>>> if there are no connections left then restore all buffers of the
>>> respective pool.
>>
>> Did you find this in code review, or was a bug reported?
> 
> Internal testing has run into a problem where some packets are never
> TX for some reason, we are still accessing if this is going to help
> but it is not an actual fix and more of a way to improve the handling
> of unacked packets.
> 
> Now I'm curious, why this sort of question seem to be recurring, we do
> encourage as much details as possible, but I don't think we ever said
> that it is required to disclose what sort of test or bug report it
> came from, especially if they come from internal testing which cannot
> be referenced anyway, so was there anything unclear with the commit
> message?

Kind of:

1.  “attempt” sounds to me like, that you tried to fix something, but 
are not actually sure.

2.  In the end, the devices need to work, so knowing how to verify a fix 
– myself for example – is always helpful in my opinion, so I often ask 
for reproducers and tests.

>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>>    net/bluetooth/hci_conn.c | 34 ++++++++++++++++++++++++++++------
>>>    1 file changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index e6cea51b3eab..4bd2e4cd477f 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -1152,22 +1152,44 @@ void hci_conn_del(struct hci_conn *conn)
>>>        disable_delayed_work_sync(&conn->auto_accept_work);
>>>        disable_delayed_work_sync(&conn->idle_work);
>>>
>>> -     /* Handle unnacked frames */
>>> +     /* Handle unnacked frames:
>>> +      *
>>> +      * - In case there are no connection restore all buffers to the pool
>>> +      * - Otherwise restore just the buffers considered in transit for the
>>> +      *   hci_conn
>>> +      */
>>>        switch (conn->type) {
>>>        case ACL_LINK:
>>> -             hdev->acl_cnt += conn->sent;
>>> +             if (!hci_conn_num(hdev, ACL_LINK))
>>> +                     hdev->acl_cnt = hdev->acl_pkts;
>>> +             else
>>> +                     hdev->acl_cnt += conn->sent;
>>>                break;
>>>        case LE_LINK:
>>>                cancel_delayed_work(&conn->le_conn_timeout);
>>>
>>> -             if (hdev->le_pkts)
>>> -                     hdev->le_cnt += conn->sent;
>>> -             else
>>> -                     hdev->acl_cnt += conn->sent;
>>> +             if (hdev->le_pkts) {
>>> +                     if (!hci_conn_num(hdev, LE_LINK))
>>> +                             hdev->le_cnt = hdev->le_pkts;
>>> +                     else
>>> +                             hdev->le_cnt += conn->sent;
>>> +             } else {
>>> +                     if (!hci_conn_num(hdev, LE_LINK) &&
>>> +                         !hci_conn_num(hdev, ACL_LINK))
>>> +                             hdev->acl_cnt = hdev->acl_pkts;
>>> +                     else
>>> +                             hdev->acl_cnt += conn->sent;
>>> +             }
>>>                break;
>>>        case CIS_LINK:
>>>        case BIS_LINK:
>>>        case PA_LINK:
>>> +             if (!hci_conn_num(hdev, CIS_LINK) &&
>>> +                 !hci_conn_num(hdev, BIS_LINK) &&
>>> +                 !hci_conn_num(hdev, PA_LINK))
>>> +                     hdev->iso_cnt = hdev->iso_pkts;
>>> +             else
>>> +                     hdev->iso_cnt += conn->sent;
>>>                hdev->iso_cnt += conn->sent;
>>>                break;
>>>        }

Kind regards,

Paul