[PATCH 1/2] Bluetooth: hci_bcm: fix build error without CONFIG_PM

[PATCH 1/2] Bluetooth: hci_bcm: fix build error without CONFIG_PM

[Arnd Bergmann]

This was introduced by the rework adding PM support:

drivers/bluetooth/hci_bcm.c: In function 'bcm_device_exists':
drivers/bluetooth/hci_bcm.c:156:22: error: 'struct bcm_device' has no member named 'hu'
  if (device && device->hu && device->hu->serdev)
                      ^~

The pointer is not available otherwise, so I'm enclosing
all references in an #ifdef here.

Fixes: 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/bluetooth/hci_bcm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ab1455e63b92..089bd0473451 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -152,9 +152,11 @@ static bool bcm_device_exists(struct bcm_device *device)
 {
 	struct list_head *p;
 
+#ifdef CONFIG_PM
 	/* Devices using serdev always exist */
 	if (device && device->hu && device->hu->serdev)
 		return true;
+#endif
 
 	list_for_each(p, &bcm_device_list) {
 		struct bcm_device *dev = list_entry(p, struct bcm_device, list);
@@ -965,7 +967,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	bcmdev->dev = &serdev->dev;
+#ifdef CONFIG_PM
 	bcmdev->hu = &bcmdev->serdev_hu;
+#endif
 	bcmdev->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, bcmdev);
 
-- 
2.9.0

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

[Arnd Bergmann]

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>
---
 drivers/bluetooth/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index fae5a74dc737..4427d54b7331 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -65,6 +65,7 @@ config BT_HCIBTSDIO
 
 config BT_HCIUART
 	tristate "HCI UART driver"
+	depends on SERIAL_DEV_BUS || !SERIAL_DEV_BUS
 	depends on TTY
 	help
 	  Bluetooth HCI UART driver.
@@ -79,7 +80,6 @@ config BT_HCIUART
 config BT_HCIUART_SERDEV
 	bool
 	depends on SERIAL_DEV_BUS && BT_HCIUART
-	depends on SERIAL_DEV_BUS=y || SERIAL_DEV_BUS=BT_HCIUART
 	default y
 
 config BT_HCIUART_H4
-- 
2.9.0

Re: [PATCH 1/2] Bluetooth: hci_bcm: fix build error without CONFIG_PM

[Hans de Goede]

Hi,

On 11-10-17 15:46, Arnd Bergmann wrote:
> This was introduced by the rework adding PM support:
> 
> drivers/bluetooth/hci_bcm.c: In function 'bcm_device_exists':
> drivers/bluetooth/hci_bcm.c:156:22: error: 'struct bcm_device' has no member named 'hu'
>    if (device && device->hu && device->hu->serdev)
>                        ^~
> 
> The pointer is not available otherwise, so I'm enclosing
> all references in an #ifdef here.
> 
> Fixes: 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I had this on my TODO after the buildbot errors, thank you for fixing
this.

The fix looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>   drivers/bluetooth/hci_bcm.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ab1455e63b92..089bd0473451 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -152,9 +152,11 @@ static bool bcm_device_exists(struct bcm_device *device)
>   {
>   	struct list_head *p;
>   
> +#ifdef CONFIG_PM
>   	/* Devices using serdev always exist */
>   	if (device && device->hu && device->hu->serdev)
>   		return true;
> +#endif
>   
>   	list_for_each(p, &bcm_device_list) {
>   		struct bcm_device *dev = list_entry(p, struct bcm_device, list);
> @@ -965,7 +967,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
>   		return -ENOMEM;
>   
>   	bcmdev->dev = &serdev->dev;
> +#ifdef CONFIG_PM
>   	bcmdev->hu = &bcmdev->serdev_hu;
> +#endif
>   	bcmdev->serdev_hu.serdev = serdev;
>   	serdev_device_set_drvdata(serdev, bcmdev);
>   
> 

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

[Hans de Goede]

Hi,

On 11-10-17 15:47, Arnd Bergmann wrote:
> 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.

As said I've this on my todo, but feel free to beat me to it,
I likely will not get around to this before Friday anyways.

Regards,

Hans



> ---
>   drivers/bluetooth/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index fae5a74dc737..4427d54b7331 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -65,6 +65,7 @@ config BT_HCIBTSDIO
>   
>   config BT_HCIUART
>   	tristate "HCI UART driver"
> +	depends on SERIAL_DEV_BUS || !SERIAL_DEV_BUS
>   	depends on TTY
>   	help
>   	  Bluetooth HCI UART driver.
> @@ -79,7 +80,6 @@ config BT_HCIUART
>   config BT_HCIUART_SERDEV
>   	bool
>   	depends on SERIAL_DEV_BUS && BT_HCIUART
> -	depends on SERIAL_DEV_BUS=y || SERIAL_DEV_BUS=BT_HCIUART
>   	default y
>   
>   config BT_HCIUART_H4
> 

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

[Marcel Holtmann]

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.

Regards

Marcel

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

[Marcel Holtmann]

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.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: hci_bcm: fix build error without CONFIG_PM

[Marcel Holtmann]

Hi Arnd,

> This was introduced by the rework adding PM support:
> 
> drivers/bluetooth/hci_bcm.c: In function 'bcm_device_exists':
> drivers/bluetooth/hci_bcm.c:156:22: error: 'struct bcm_device' has no member named 'hu'
>  if (device && device->hu && device->hu->serdev)
>                      ^~
> 
> The pointer is not available otherwise, so I'm enclosing
> all references in an #ifdef here.
> 
> Fixes: 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/bluetooth/hci_bcm.c | 4 ++++
> 1 file changed, 4 insertions(+)

both patches have been applied to bluetooth-next tree.

Regards

Marcel

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

[Hans de Goede]

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

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

[Hans de Goede]

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

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

[Marcel Holtmann]

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.

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

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

[Johan Hovold]

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.

Johan

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

[Johan Hovold]

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.

Johan

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

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