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(-)
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) */
--
2.40.1
Hi Vlad,
On Mon, Apr 15, 2024 at 10:45 AM Vlad Pruteanu <[email protected]> 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
>
--
Luiz Augusto von Dentz
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
Hello Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <[email protected]>
> Sent: Monday, April 15, 2024 6:07 PM
> To: Vlad Pruteanu <[email protected]>
> Cc: [email protected]; Claudia Cristina Draghicescu
> <[email protected]>; Mihai-Octavian Urzica <mihai-
> [email protected]>; Silviu Florian Barbulescu
> <[email protected]>; Iulia Tanasescu <[email protected]>;
> Andrei Istodorescu <[email protected]>
> 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
> <[email protected]> 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
Hi Vlad,
On Tue, Apr 16, 2024 at 6:22 AM Vlad Pruteanu <[email protected]> wrote:
>
> Hello Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <[email protected]>
> > Sent: Monday, April 15, 2024 6:07 PM
> > To: Vlad Pruteanu <[email protected]>
> > Cc: [email protected]; Claudia Cristina Draghicescu
> > <[email protected]>; Mihai-Octavian Urzica <mihai-
> > [email protected]>; Silviu Florian Barbulescu
> > <[email protected]>; Iulia Tanasescu <[email protected]>;
> > Andrei Istodorescu <[email protected]>
> > 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
> > <[email protected]> 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
--
Luiz Augusto von Dentz