linux-kernel - Re: [PATCH 2/2] Bluetooth: BT_HCIUART now depends on SERIAL_DEV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <0E9ED850-8F65-498C-A4EB-D3413E25CFF7@holtmann.org>
Date:   Thu, 12 Oct 2017 10:40:29 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        "Gustavo F. Padovan" <gustavo@...ovan.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Sebastian Reichel <sre@...nel.org>,
        Tobias Regnery <tobias.regnery@...il.com>,
        "bluez mailin list (linux-bluetooth@...r.kernel.org)" 
        <linux-bluetooth@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Bluetooth: BT_HCIUART now depends on SERIAL_DEV_BUS

Hi Hans,

>>>> It is no longer possible to build BT_HCIUART into the kernel
>>>> when SERIAL_DEV_BUS is a loadable module, even if none of the
>>>> SERIAL_DEV_BUS based implementations are selected:
>>>> drivers/bluetooth/hci_ldisc.o: In function `hci_uart_set_flow_control':
>>>> hci_ldisc.c:(.text+0xb40): undefined reference to `serdev_device_set_flow_control'
>>>> hci_ldisc.c:(.text+0xb5c): undefined reference to `serdev_device_set_tiocm'
>>>> This adds a dependency to avoid the broken configuration.
>>>> Fixes: 7841d554809b ("Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev")
>>>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>>> 
>>> Another one I have on my TODO after the buildbot errors. In this case
>>> I do not believe this is the proper fix though.
>>> 
>>> As pointed out in another thread discussing the series introducing
>>> this problem, hci_ldisc.c really should not depend on serdev,
>>> so the proper fix would be to have hci_bcm.c directly call
>>> the serdev flowcontrol and rts functions when the hci is
>>> backed by a serdev device, like hci_bcm.c is already doing
>>> when setting the baudrate, see host_set_baudrate in hci_bcm.c,
>>> so a similar host_set_flow_control should be added after which
>>> the changes to hci_ldisc.c can be reverted.
>>> 
>>> If I understood Marcel correctly he prefers a single patch
>>> fixing this which also removes the changes from hci_ldisc.c,
>>> rather then a separate revert.
>> actually two patches is fine, but I want them as a patch series so they are applied in a row. I think best is first to revert the btusb.c changes and then apply the new ACPI PNP id.
> 
> This is actually about a different series, but I assume the same
> applies for both series.
> 
> My remark here was not about the btusb 0000:0000 USB ids handling +
> ACPI PNP ids, but about fixing hci_ldisc.c now depending on
> serdev.
> 
> So since you now have accepted Arnd patch making hci_ldisc.c now
> depending on serdev more or less official (which I'm fine with),
> can I assume that this is going to be the final (for now / for 4.15)
> state of the deps situation surrounding hci_ldisc.c ?
> 
> The reason I'm asking this is because my plan was to undo the
> changes introducing the hci_ldisc.c dependency on serdev and
> instead adding a host_set_flow_control helper to hci_bcm.c,
> so have hci_bcm.c directly call the serdev flowcontrol funcs
> instead of having it depend on hci_ldisc.c for this in the
> same way as it is already directly calling
> serdev_device_set_baudrate.
> 
> If we are going to let hci_ldisc.c deal with tty vs serdev
> backed devices for flowcontrol, it makes sense to do the
> same for baudrate and make it save to call hci_uart_set_baudrate
> on a serdev backed hci_uart and drop the host_set_baudrate
> helper from hci_bcm.c. I can write and test a patch for this ...
> 
> Either way let me know how you want to proceed. I will let
> this rest until I hear back from you.

what we have currently in bluetooth-next is something that I am planning to push into net-next and with that 4.15. I have not heard any complaints so far and it has been baking for a while now. So yes, lets remove the USB 0000:0000 device support and move them over to UART serdev devices.

For flow control and baud rate handling, I am open for proposals to simplify this or streamline it into something cleaner. I think the goal should be to move towards enabling more devices for serdev operation. As I mentioned the other day, there are ACPI described Intel and Realtek Bluetooth controllers as well. So there is some work to be done here.

We need to keep basic btattach functionality working since in some cases that is required for development boards that come via USB-UART bridges. But I think he main interface should be serdev now.

I think what we should figure out is if serdev based drivers need any hci_ldisc.c infrastructure at all or if they be better separated out into individual drivers. Not sure we have to make that call for 4.15 kernel.

At one point I was dreaming about having everything serdev based and hci_ldisc.c is just used to create the serdev devices and manage its lifetime. Rob tried this in one of his branch, but the TTY layer is still getting a bit in your way. So maybe a debugfs interface or something similar might be an alternate option instead of trying to hack a line discipline to setup serdev.

Regards

Marcel