Re: [RFC 3/5] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann <marcel@holtmann.org>]

Hi Peter,

>>>>> The line discipline is always notified of line rate changes
>>>>> via the set_termios() method, so if you add that to the hci_ldisc,
>>>>> you'll be able to keep the BT device in sync with your
>>>>> vendor-specific commands.
>>>> If I'm not wrong, line discipline is notified after the tty termios change.
>>>> So, it's too late to send the  HCI change speed command.
>>>> Moreover, user space is not aware of the device behavior, the driver should
>>>> be the only one to manage the device and its serial link.
>>>> Here, user space is just a bootstrap which hands over to the driver.
>>> 
>>> You realize that you're telling me that userspace is unaware of
>>> this in a thread that begins with a proposed userspace ioctl change to
>>> the line discipline? An ioctl change that adds a new ioctl to set the
>>> speed in a line discipline? For which there are already lots of ioctls
>>> to set the line speed?
>> 
>> actually the problem is a little bit different here.
> 
> Thanks for taking the time to outline the operation of hciattach. It's
> been a while since I was involved in the BT userspace code so I hadn't
> realized how much hciattach had morphed.
> 
>> Every Bluetooth UART chip has its default baudrate. Mostly that is 115k, but some actually have others.
>> 
>> The general idea was that userspace deals with opening the TTY, send the HCI commands to change the rate, then change the termios setting, attach the ldisc and then hand it to the kernel.
>> 
>> This is however not how at least two major Bluetooth controllers work when you have to do firmware loading. When they boot into the firmware, then baudrate falls back to default and you have to do that all over again.
>> 
>> So what this ioctl will just tell the kernel about is the desired baudrate that it wants to be set whenever possible. The whenever possible is important here since depending on the manufacturer that might be a total different times. And with firmware download procedures being fully vendor specific this will vary.
>> 
>>> Setting aside the problem with the firmware load, I see no reason why
>>> this can't be done within the existing line discipline framework.
>> 
>> So far everything has been done in hciattach tool. And it is so ugly and does not even work properly for any complex UART protocols like BCSP or 3-Wire. There is a patch for a modified 3-Wire setup from one vendor that made my eyes bleed. So what you really want is having the kernel take control over HCI from the beginning. Otherwise this goes out of control and handling exceptions like hardware error event becomes impossible.
>> 
>> In summary what we have to do these days is this:
>> 
>> - Send Reset
>> - Send a few basic HCI commands
>> - Send baud rate change command
>> - Change actual baud rate on the TTY
>> - Download firmware
>> - Send Reset to boot the firmware
>> - Change actual baud rate back to default
>> - Send baud rate change command
>> - Change actual baud rate
>> - Send Reset
>> - Standard init sequence
>> 
>> Now tell me how the line discipline framework helps us here?
> 
> Well,
> 1. in hciattach I would have set the N_HCI line discipline immediately
>   after opening the uart device.
> 2. in the N_HCI line discipline, I would have implemented write() so
>   userspace command write()s would still write through to the uart
>   device.
> 3. either,
>   a. I would have added a new line discipline method for pre-termios changes, or
>   b. I would have caught set termios ioctls in the N_HCI line discipline ioctl.
> 
>   and handled sending target line speed change commands there.
>   Userspace would simply change the line speed.
> 
> Userspace firmware loads are straightforward.
> 
> However, it seems that the new intel approach (tied in with the protocol)
> is trickier to accommodate, but I haven't spent a lot of time trying to follow
> that setup.
> 
> I'm not sure I understand why the firmware load is tied to the protocol;
> can the intel part load a different firmware that implements a different protocol?

the firmware loading procedure for USB and UART is actually the same. That applies to Broadcom and Intel controllers for sure and most likely also for other vendors. One goal is to share the firmware loading code between USB and UART based SKUs of these controllers. Where are not there yet, but that is where this is heading. So while we can hack around certain things in userspace, we really do not want to do that since it just means duplicated code.

Implementing the write() is a bad idea in my opinion. That is like injecting HCI commands behind your back. All sorts of messy things can happen if you allow that. And that is why this is actually not even implemented at the moment. Remember that HCI protocol comes with flow control for their commands and events. Injecting something without taking care of the command flow control means you are doing evil stuff. The firmware loading is using HCI commands and thus it should go through the high-level Bluetooth core that understands HCI commands.

Our idea is essentially that we set the N_HCI line discipline and then set the vendor protocol. That is it. From that point on it is the kernels problem to handle the bringup of the Bluetooth device. Including firmware download, address configuration and so on. Before setting N_HCI we would correctly configure the default baudrate and all other needed settings. However that is as much as we do from userspace.

For this to work, we need to be able to tell the TTY to change its baudrate. And this has to happen at least 3 times now. Higher baudrate before downloading the firmware, back to default baudrate after that, switch to higher baudrate and only then we are at normal operation.

So how does intercepting the termios ioctl will help us in this regard. The kernel needs to control the baudrate change and not userspace.

>> Keep in mind you have to tell the controller via HCI commands over the old baud rate to change its rate. And then change it on the TTY to match what the controller does on its side.
> 
> I'm conscious of the limitations of using tty/serial in the current manner.
> 
> A fun project for one of these vendors would be to split the serial core
> into a separate tty driver and an abstraction layer that would allow other
> kernel subsystems to enslave the port.

They are proposing TTY slave devices and maybe that will help eventually, but finding agreement on that seems hard as well. And that is also mostly for handling the extra GPIO configuration for power up/down the Bluetooth chip behind the TTY and not the TTY itself.

I would love if we could just expose these TTYs as bi-directional pipes on a "serial bus" and just have a driver attach to it instead of going through a line discipline.

Regards

Marcel