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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
