[1/1] Bluetooth: hci_event: Fix setting of unicast qos interval

Commit Message

Vlad Pruteanu April 15, 2024, 2:44 p.m. UTC

qos->ucast interval reffers to the SDU interval, and should not
be set to the interval value reported by the LE CIS Established
event since the latter reffers to the ISO interval. These two
interval are not the same thing:

BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G

Isochronous interval:
The time between two consecutive BIS or CIS events (designated
ISO_Interval in the Link Layer)

SDU interval:
The nominal time between two consecutive SDUs that are sent or
received by the upper layer.
---
 net/bluetooth/hci_event.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Luiz Augusto von Dentz April 15, 2024, 3:06 p.m. UTC | #1

Hi Vlad,

On Mon, Apr 15, 2024 at 10:45 AM Vlad Pruteanu <[e-mail]> wrote:
>
> qos->ucast interval reffers to the SDU interval, and should not
> be set to the interval value reported by the LE CIS Established
> event since the latter reffers to the ISO interval. These two
> interval are not the same thing:
>
> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G
>
> Isochronous interval:
> The time between two consecutive BIS or CIS events (designated
> ISO_Interval in the Link Layer)
>
> SDU interval:
> The nominal time between two consecutive SDUs that are sent or
> received by the upper layer.

I assume they are not the same because the ISO interval can have more
than one subevents, but otherwise if BN=1 then it shall be aligned, so
we are probably missing the BN component here.

> ---
>  net/bluetooth/hci_event.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 868ffccff773..83cf0e8a56cf 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6824,10 +6824,6 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
>
>         pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
>
> -       /* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */
> -       qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250;

This most likely needs to be le16_to_cpu(ev->interval) * 1250 *
ev->bn, anyway it probably makes sense to indicate what the BN is
causing this problem.

> -       qos->ucast.out.interval = qos->ucast.in.interval;
>
>         switch (conn->role) {
>         case HCI_ROLE_SLAVE:
>                 /* Convert Transport Latency (us) to Latency (msec) */
> --
> 2.40.1
>

[e-mail] April 15, 2024, 3:33 p.m. UTC | #2

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=844684

---Test result---

Test Summary:
CheckPatch                    FAIL      0.90 seconds
GitLint                       PASS      0.31 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      29.88 seconds
CheckAllWarning               PASS      32.46 seconds
CheckSparse                   WARNING   38.05 seconds
CheckSmatch                   FAIL      36.47 seconds
BuildKernel32                 PASS      28.89 seconds
TestRunnerSetup               PASS      519.60 seconds
TestRunner_l2cap-tester       PASS      18.36 seconds
TestRunner_iso-tester         PASS      30.67 seconds
TestRunner_bnep-tester        PASS      4.69 seconds
TestRunner_mgmt-tester        PASS      113.50 seconds
TestRunner_rfcomm-tester      PASS      7.36 seconds
TestRunner_sco-tester         PASS      14.93 seconds
TestRunner_ioctl-tester       PASS      7.60 seconds
TestRunner_mesh-tester        PASS      5.72 seconds
TestRunner_smp-tester         PASS      6.76 seconds
TestRunner_userchan-tester    PASS      4.86 seconds
IncrementalBuild              PASS      27.83 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[1/1] Bluetooth: hci_event: Fix setting of unicast qos interval
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 0 checks, 10 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13630191.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2


---
Regards,
Linux Bluetooth

Vlad Pruteanu April 16, 2024, 10:22 a.m. UTC | #3

Hello Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <[e-mail]>
> Sent: Monday, April 15, 2024 6:07 PM
> To: Vlad Pruteanu <[e-mail]>
> Cc: [e-mail]; Claudia Cristina Draghicescu
> <[e-mail]>; Mihai-Octavian Urzica <mihai-
> [e-mail]>; Silviu Florian Barbulescu
> <[e-mail]>; Iulia Tanasescu <[e-mail]>;
> Andrei Istodorescu <[e-mail]>
> Subject: [EXT] Re: [PATCH 1/1] Bluetooth: hci_event: Fix setting of unicast qos
> interval
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Hi Vlad,
> 
> On Mon, Apr 15, 2024 at 10:45 AM Vlad Pruteanu
> <[e-mail]> wrote:
> >
> > qos->ucast interval reffers to the SDU interval, and should not
> > be set to the interval value reported by the LE CIS Established
> > event since the latter reffers to the ISO interval. These two
> > interval are not the same thing:
> >
> > BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G
> >
> > Isochronous interval:
> > The time between two consecutive BIS or CIS events (designated
> > ISO_Interval in the Link Layer)
> >
> > SDU interval:
> > The nominal time between two consecutive SDUs that are sent or
> > received by the upper layer.
> 
> I assume they are not the same because the ISO interval can have more
> than one subevents, but otherwise if BN=1 then it shall be aligned, so
> we are probably missing the BN component here.
> 
I don't think that there's any need for setting the SDU Interval of the qos
here since it has already been set by the host prior to issuing the LE Set
CIG Parameters command, so the controller will have to respect that
value. Since the it has been set by the host, to be used by the controller,
to me, it seems a little bit redundant to derive the SDU Interval
once again based on parameters received on this event. I think that
continuing to use the initial value set by the Host should suffice.

> > ---
> >  net/bluetooth/hci_event.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 868ffccff773..83cf0e8a56cf 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6824,10 +6824,6 @@ static void hci_le_cis_estabilished_evt(struct
> hci_dev *hdev, void *data,
> >
> >         pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
> >
> > -       /* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */
> > -       qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250;
> 
> This most likely needs to be le16_to_cpu(ev->interval) * 1250 *
> ev->bn, anyway it probably makes sense to indicate what the BN is
> causing this problem.
> 
> > -       qos->ucast.out.interval = qos->ucast.in.interval;
> >
> >         switch (conn->role) {
> >         case HCI_ROLE_SLAVE:
> >                 /* Convert Transport Latency (us) to Latency (msec) */
> > --
> > 2.40.1
> >
> 
> 
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz April 16, 2024, 2:18 p.m. UTC | #4

Hi Vlad,

On Tue, Apr 16, 2024 at 6:22 AM Vlad Pruteanu <[e-mail]> wrote:
>
> Hello Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <[e-mail]>
> > Sent: Monday, April 15, 2024 6:07 PM
> > To: Vlad Pruteanu <[e-mail]>
> > Cc: [e-mail]; Claudia Cristina Draghicescu
> > <[e-mail]>; Mihai-Octavian Urzica <mihai-
> > [e-mail]>; Silviu Florian Barbulescu
> > <[e-mail]>; Iulia Tanasescu <[e-mail]>;
> > Andrei Istodorescu <[e-mail]>
> > Subject: [EXT] Re: [PATCH 1/1] Bluetooth: hci_event: Fix setting of unicast qos
> > interval
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> >
> >
> > Hi Vlad,
> >
> > On Mon, Apr 15, 2024 at 10:45 AM Vlad Pruteanu
> > <[e-mail]> wrote:
> > >
> > > qos->ucast interval reffers to the SDU interval, and should not
> > > be set to the interval value reported by the LE CIS Established
> > > event since the latter reffers to the ISO interval. These two
> > > interval are not the same thing:
> > >
> > > BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G
> > >
> > > Isochronous interval:
> > > The time between two consecutive BIS or CIS events (designated
> > > ISO_Interval in the Link Layer)
> > >
> > > SDU interval:
> > > The nominal time between two consecutive SDUs that are sent or
> > > received by the upper layer.
> >
> > I assume they are not the same because the ISO interval can have more
> > than one subevents, but otherwise if BN=1 then it shall be aligned, so
> > we are probably missing the BN component here.
> >
> I don't think that there's any need for setting the SDU Interval of the qos
> here since it has already been set by the host prior to issuing the LE Set
> CIG Parameters command, so the controller will have to respect that
> value. Since the it has been set by the host, to be used by the controller,
> to me, it seems a little bit redundant to derive the SDU Interval
> once again based on parameters received on this event. I think that
> continuing to use the initial value set by the Host should suffice.

Yeah but how about the receiver case? Or you expected that we set the
QoS settings as a server as well? We need to confirm that this works
in both directions or actually I don't think this would work with the
likes of iso-test/isotester because there is no BAP layer seating
above it to configure the SDU interval it really needs to come from
the ISO socket itself.

> > > ---
> > >  net/bluetooth/hci_event.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 868ffccff773..83cf0e8a56cf 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -6824,10 +6824,6 @@ static void hci_le_cis_estabilished_evt(struct
> > hci_dev *hdev, void *data,
> > >
> > >         pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
> > >
> > > -       /* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */
> > > -       qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250;
> >
> > This most likely needs to be le16_to_cpu(ev->interval) * 1250 *
> > ev->bn, anyway it probably makes sense to indicate what the BN is
> > causing this problem.
> >
> > > -       qos->ucast.out.interval = qos->ucast.in.interval;
> > >
> > >         switch (conn->role) {
> > >         case HCI_ROLE_SLAVE:
> > >                 /* Convert Transport Latency (us) to Latency (msec) */
> > > --
> > > 2.40.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Patch

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 868ffccff773..83cf0e8a56cf 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6824,10 +6824,6 @@  static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
 
 	pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
 
-	/* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */
-	qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250;
-	qos->ucast.out.interval = qos->ucast.in.interval;
-
 	switch (conn->role) {
 	case HCI_ROLE_SLAVE:
 		/* Convert Transport Latency (us) to Latency (msec) */