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

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

Hi,

On 13-10-17 12:09, Johan Hovold wrote:
> On Thu, Oct 12, 2017 at 10:40:29AM +0200, 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.
>>
>> 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.
> 
> So, again, this doesn't seem to have anything to do with that btusb
> patch, but rather a new dependency that was added by the following
> commit by Hans from last week:
> 
> 	7841d554809b ("Bluetooth: hci_uart_set_flow_control: Fix NULL
> 	               deref when using serdev")
> 
>> 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.
> 
> Yes, and that's how things were implemented when hci_serdev.c was added.
> To some extent code was copied and duplicated from hci_ldisc.c in order
> not to depend on those helpers, but now with the above mentioned commit
> we have a serdev driver calling into the old line-discipline code, which
> then only calls into the hci-serdev implementation again.
> 
> I mentioned this when I reviewed the patch in question, but at that
> point it had already been applied.
> 
> It seems to me like effectively reverting the above mentioned commit in
> favour of small helper in hci_bcm is the right thing to do short term
> (i.e. handle it as we handle set_baudrate).
> 
> Perhaps later someone can revisit the decision to keep two separate
> implementations (hci_serdev.c and hci_ldisc.c) and see what can be done
> about code reuse in order to simplify for hci-drivers wanting to support
> both interfaces.

As indicated I would be happy to provide (and test on my hw) patches
for this. I'm fine with leaving things as is too (although I agree that
is somewhat inconsistent).

Anyways I will go and prepare (and test) a new version of the btusb
0000:0000 revert + new ACPI pnp series as Marcel has asked for.

Regards,

Hans