| Message ID | 20240814130018.263129-1-frederic.danis@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: hci_bcm: Use speed set by btattach as oper_speed | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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=879637 ---Test result--- Test Summary: CheckPatch PASS 0.65 seconds GitLint PASS 0.32 seconds SubjectPrefix PASS 0.12 seconds BuildKernel PASS 29.35 seconds CheckAllWarning PASS 32.31 seconds CheckSparse PASS 38.30 seconds CheckSmatch PASS 102.76 seconds BuildKernel32 PASS 28.76 seconds TestRunnerSetup PASS 529.47 seconds TestRunner_l2cap-tester PASS 20.16 seconds TestRunner_iso-tester PASS 31.34 seconds TestRunner_bnep-tester PASS 4.88 seconds TestRunner_mgmt-tester FAIL 111.84 seconds TestRunner_rfcomm-tester PASS 7.50 seconds TestRunner_sco-tester PASS 15.04 seconds TestRunner_ioctl-tester PASS 8.06 seconds TestRunner_mesh-tester PASS 5.93 seconds TestRunner_smp-tester PASS 7.02 seconds TestRunner_userchan-tester PASS 5.11 seconds IncrementalBuild PASS 27.83 seconds Details ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2 Failed Test Cases LL Privacy - Add Device 6 (RL is full) Failed 0.198 seconds --- Regards, Linux Bluetooth
Hi Frédéric, On Wed, Aug 14, 2024 at 9:07 AM Frédéric Danis <frederic.danis@collabora.com> wrote: > > Starting a BCM UART controller not defined as a platform device or > a serdev with "btattach -B /dev/ttyS1 -P bcm -S 3000000" works fine > but the serial port remains at the init_speed, i.e. 115200. > > The oper_speed is only set if a device is declared in ACPI, device > tree or as a platform device. > > When no registered device has been found this commit will use the > current tty speed set by btattach as the oper_speed. > > Signed-off-by: Frédéric Danis <frederic.danis@collabora.com> > --- > > This has been tested with 5.4 kernel only. > Afaict there's no change in this driver which should be impacted by > this commit in latest kernel. > > drivers/bluetooth/hci_bcm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c > index 89d4c2224546..57cacf63ae12 100644 > --- a/drivers/bluetooth/hci_bcm.c > +++ b/drivers/bluetooth/hci_bcm.c > @@ -508,6 +508,11 @@ static int bcm_open(struct hci_uart *hu) > > if (err) > goto err_unset_hu; > + } else { > + /* This is not a serdev or platform device, it should have been started by > + * btattach, in this case use the tty speed set by btattach as oper_speed > + */ > + hu->oper_speed = hu->tty->termios.c_ospeed; > } While Im fine with the general idea of derive the speed from the tty what Im not sure where it should be done since doing this on the driver may duplicate the logic if we later found there is a better place to do it. Looks at hci_uart.c it seems like we could actually register a set_termios: * @set_termios: [TTY] ``void ()(struct tty_struct *tty, const struct ktermios *old)`` * * This function notifies the line discpline that a change has been made * to the termios structure. * * Optional. So I assume when the user changes the speed the above callback gets called and we could then reflect the changes to the oper_speed, or perhaps the real problem was that hci_uart_set_baudrate was not called? > mutex_unlock(&bcm_device_lock); > -- > 2.34.1 > >
Hi Luiz, On 19/08/2024 16:49, Luiz Augusto von Dentz wrote: > Hi Frédéric, > > On Wed, Aug 14, 2024 at 9:07 AM Frédéric Danis > <frederic.danis@collabora.com> wrote: >> Starting a BCM UART controller not defined as a platform device or >> a serdev with "btattach -B /dev/ttyS1 -P bcm -S 3000000" works fine >> but the serial port remains at the init_speed, i.e. 115200. >> >> The oper_speed is only set if a device is declared in ACPI, device >> tree or as a platform device. >> >> When no registered device has been found this commit will use the >> current tty speed set by btattach as the oper_speed. >> >> Signed-off-by: Frédéric Danis <frederic.danis@collabora.com> >> --- >> >> This has been tested with 5.4 kernel only. >> Afaict there's no change in this driver which should be impacted by >> this commit in latest kernel. >> >> drivers/bluetooth/hci_bcm.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c >> index 89d4c2224546..57cacf63ae12 100644 >> --- a/drivers/bluetooth/hci_bcm.c >> +++ b/drivers/bluetooth/hci_bcm.c >> @@ -508,6 +508,11 @@ static int bcm_open(struct hci_uart *hu) >> >> if (err) >> goto err_unset_hu; >> + } else { >> + /* This is not a serdev or platform device, it should have been started by >> + * btattach, in this case use the tty speed set by btattach as oper_speed >> + */ >> + hu->oper_speed = hu->tty->termios.c_ospeed; >> } > While Im fine with the general idea of derive the speed from the tty > what Im not sure where it should be done since doing this on the > driver may duplicate the logic if we later found there is a better > place to do it. > > Looks at hci_uart.c it seems like we could actually register a set_termios: > > * @set_termios: [TTY] ``void ()(struct tty_struct *tty, const struct > ktermios *old)`` > * > * This function notifies the line discpline that a change has been made > * to the termios structure. > * > * Optional. > > So I assume when the user changes the speed the above callback gets > called and we could then reflect the changes to the oper_speed, or > perhaps the real problem was that hci_uart_set_baudrate was not > called? The hci_uart_set_baudrate is called correctly as when I forced the oper_speed the serial port speed changes accordingly and I saw the hci_uart_set_baudrate debug trace. Unfortunately, the serial port speed is fixed by btattach before setting the line discipline, so that the set_termios is not called in this case. But above all, set_termios is called each time the driver change the speed, i.e. when it sets the initial speed. I will send a new patch to copy the serial port speed to the oper_speed in hci_uart_tty_open() of drivers/bluetooth/hci_ldisc.c.
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c index 89d4c2224546..57cacf63ae12 100644 --- a/drivers/bluetooth/hci_bcm.c +++ b/drivers/bluetooth/hci_bcm.c @@ -508,6 +508,11 @@ static int bcm_open(struct hci_uart *hu) if (err) goto err_unset_hu; + } else { + /* This is not a serdev or platform device, it should have been started by + * btattach, in this case use the tty speed set by btattach as oper_speed + */ + hu->oper_speed = hu->tty->termios.c_ospeed; } mutex_unlock(&bcm_device_lock);
Starting a BCM UART controller not defined as a platform device or a serdev with "btattach -B /dev/ttyS1 -P bcm -S 3000000" works fine but the serial port remains at the init_speed, i.e. 115200. The oper_speed is only set if a device is declared in ACPI, device tree or as a platform device. When no registered device has been found this commit will use the current tty speed set by btattach as the oper_speed. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com> --- This has been tested with 5.4 kernel only. Afaict there's no change in this driver which should be impacted by this commit in latest kernel. drivers/bluetooth/hci_bcm.c | 5 +++++ 1 file changed, 5 insertions(+)