Re: [PATCH 2/2] Bluetooth: BT_HCIUART now depends on SERIAL_DEV_BUS

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11-10-17 20:10, Marcel Holtmann wrote:
> 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@arndb.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.

Regards,

Hans