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

[Hans de Goede]

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@xxxxxxxx>
> > > > >
> > > > > 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