diff mbox series

Bluetooth: hci_bcm: Use speed set by btattach as oper_speed

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

Checks

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

Commit Message

Frédéric Danis Aug. 14, 2024, 1 p.m. UTC
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(+)

Comments

bluez.test.bot@gmail.com Aug. 14, 2024, 1:39 p.m. UTC | #1
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
Luiz Augusto von Dentz Aug. 19, 2024, 2:49 p.m. UTC | #2
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
>
>
Frédéric Danis Aug. 20, 2024, 2:04 p.m. UTC | #3
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 mbox series

Patch

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);