Bluetooth: hci_bcm: Use speed set by btattach as oper_speed

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(+)

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.