BT / serial regression introduced during the recent 4.9-rc1 merge?

I've compiled the latest bluetooth-next branch and I'm facing what looks like a regression to me (still on a Lenovo ThinkPad 8 tablet): btattach doesn't properly initialize the Broadcom BCM2E55 chipset anymore.

I'm getting various timeout messages in dmesg:

[   13.188057] Bluetooth: hci0 command 0xfc45 tx timeout [   16.093068] serial8250_interrupt: 4158 callbacks suppressed [   16.093072] serial8250: too much work for irq39
[   16.094287] serial8250: too much work for irq39
...
[   16.103868] serial8250: too much work for irq39
[   21.100041] serial8250_interrupt: 4167 callbacks suppressed [   21.100044] serial8250: too much work for irq39
...
[   21.222065] Bluetooth: hci0: BCM: failed to write clock (-110) [   23.238528] Bluetooth: hci0 command 0x0c03 tx timeout [   26.104253] serial8250_interrupt: 4165 callbacks suppressed [   26.104257] serial8250: too much work for irq39

which I never had before and Bluetooth never actually starts.

I've gone back in time and it doesn't work either with a kernel built at e6445f52d9c8b0e6557a45fa7d0e8e088d430a8c "Merge tag 'usb-4.9-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb".

It still worked at commit a2f195a73eba807006fb0cb882ecb552c06eea00 "bluetooth: bcm203x: don't print error when allocating urb fails" though, which was the last previous commit modifying files in drivers/bluetooth in the bluetooth-next branch.

I've compiled the latest bluetooth-next branch and I'm facing what looks like a regression to me (still on a Lenovo ThinkPad 8 tablet): btattach doesn't properly initialize the Broadcom BCM2E55 chipset anymore.

I'm getting various timeout messages in dmesg:

[   13.188057] Bluetooth: hci0 command 0xfc45 tx timeout [   16.093068] serial8250_interrupt: 4158 callbacks suppressed [   16.093072] serial8250: too much work for irq39
[   16.094287] serial8250: too much work for irq39
...
[   16.103868] serial8250: too much work for irq39
[   21.100041] serial8250_interrupt: 4167 callbacks suppressed [   21.100044] serial8250: too much work for irq39
...
[   21.222065] Bluetooth: hci0: BCM: failed to write clock (-110) [   23.238528] Bluetooth: hci0 command 0x0c03 tx timeout [   26.104253] serial8250_interrupt: 4165 callbacks suppressed [   26.104257] serial8250: too much work for irq39

which I never had before and Bluetooth never actually starts.

Bluetooth doesn't init with a kernel built with the latest commit from yesterday 526c86021e5102b8a4b5555b4196f7a19f44e2c4.

I've gone back in time and it doesn't work either with a kernel built at e6445f52d9c8b0e6557a45fa7d0e8e088d430a8c "Merge tag 'usb-4.9-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb".

It still worked at commit a2f195a73eba807006fb0cb882ecb552c06eea00 "bluetooth: bcm203x: don't print error when allocating urb fails" though, which was the last previous commit modifying files in drivers/bluetooth in the bluetooth-next branch.

I've attached the output of btmon when it used to work and one not working (prefixed with .e6445f5) if that may be useful.

I'll continue my investigation in my spare time, trying to bissect to find a while. I'll focus on patches touching the serial 8250 driver to start with, as there are only a few of them, but feel free to point me into a different direction if you have another idea or suggestion.

Hi Heikki, Andy, Greg, Rafael,

I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly initializing, with many timeout messages in dmesg like these:
"serial8250_interrupt: WXYZ callbacks suppressed" "serial8250: too much work for irq39"
cf. my earlier report on linux-bluetooth at the very end for reference.

After a long git bisect, I've found the commit that's triggering the issue:

commit 20a875e2e86e73d13ec256781a7d55a7885868ec

Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Date:   Tue Aug 23 11:33:28 2016 +0300

serial: 8250_dw: Add quirk for APM X-Gene SoC

The APM X-Gene SoC UART is the only board that still needs the hard-coded values, so handle it separately in dw8250_quirks(). The other ACPI platforms are able to provide the values with device properties.

Signed-off-by Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

Reviewed-by Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Acked-by Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

Signed-off-by Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

drivers/tty/serial/8250/8250_dw.c +10 -5

index e199696..5c0c123 100644
@@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct
 dw8250_data *data)
                        p->serial_out = dw8250_serial_out32be;
                }
        } else if (has_acpi_companion(p->dev)) {
-               p->iotype = UPIO_MEM32;
-               p->regshift = 2;
-               p->serial_in = dw8250_serial_in32;
+               const struct acpi_device_id *id;
+
+               id = acpi_match_device(p->dev->driver->acpi_match_table,
+                                      p->dev);
+               if (id && !strcmp(id->id, "APMC0D08")) {
+                       p->iotype = UPIO_MEM32;
+                       p->regshift = 2;
+                       p->serial_in = dw8250_serial_in32;
+                       data->uart_16550_compatible = true;
+               }
                p->set_termios = dw8250_set_termios;
-               /* So far none of there implement the Busy Functionality */
-               data->uart_16550_compatible = true;
        }
 

        /* Platforms with iDMA */

This is fully reproducible on my end: removing this specific patch gets rid of the issue; with the patch applied, Bluetooth simply doesn't work anymore.

As opposed to the commit description, it seems that the Lenovo ThinkPad 8 still needs the original hard-coded values currently (even if it could be possible to provide them in another way in the future, if I interpret the commit message the right way).

Could this patch be reverted for the moment to remove the regression, until a proper fix is found?

Quoted 8 lines · Jérôme de Bretagne

Hi Heikki, Andy, Greg, Rafael,

I've detected a regression on my Lenovo ThinkPad 8 tablet introduced during 4.9-rc1, preventing the built-in Bluetooth Broadcom chipset from properly initializing, with many timeout messages in dmesg like these:
"serial8250_interrupt: WXYZ callbacks suppressed" "serial8250: too much work for irq39"
cf. my earlier report on linux-bluetooth at the very end for reference.

Quoted 57 lines

After a long git bisect, I've found the commit that's triggering the issue:

commit 20a875e2e86e73d13ec256781a7d55a7885868ec

Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Date:   Tue Aug 23 11:33:28 2016 +0300

serial: 8250_dw: Add quirk for APM X-Gene SoC

The APM X-Gene SoC UART is the only board that still needs the hard-coded values, so handle it separately in dw8250_quirks(). The other ACPI platforms are able to provide the values with device properties.

Signed-off-by Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

Reviewed-by Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Acked-by Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

Signed-off-by Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

drivers/tty/serial/8250/8250_dw.c +10 -5

index e199696..5c0c123 100644
@@ -298,12 +298,17 @@ static void dw8250_quirks(struct uart_port *p, struct
 dw8250_data *data)
                        p->serial_out = dw8250_serial_out32be;
                }
        } else if (has_acpi_companion(p->dev)) {
-               p->iotype = UPIO_MEM32;
-               p->regshift = 2;
-               p->serial_in = dw8250_serial_in32;
+               const struct acpi_device_id *id;
+
+               id = acpi_match_device(p->dev->driver->acpi_match_table,
+                                      p->dev);
+               if (id && !strcmp(id->id, "APMC0D08")) {
+                       p->iotype = UPIO_MEM32;
+                       p->regshift = 2;
+                       p->serial_in = dw8250_serial_in32;
+                       data->uart_16550_compatible = true;
+               }
                p->set_termios = dw8250_set_termios;
-               /* So far none of there implement the Busy Functionality */
-               data->uart_16550_compatible = true;
        }
 

        /* Platforms with iDMA */

This is fully reproducible on my end: removing this specific patch gets rid of the issue; with the patch applied, Bluetooth simply doesn't work anymore.

As opposed to the commit description, it seems that the Lenovo ThinkPad 8 still needs the original hard-coded values currently (even if it could be possible to provide them in another way in the future, if I interpret the commit message the right way).

Quoted 2 lines

Could this patch be reverted for the moment to remove the regression, until a proper fix is found?

Heikki, I don't think anything depends on this commit, is that correct?

Quoted 1 line

Heikki, I don't think anything depends on this commit, is that correct?

Unfortunately we can't fix the values for every UART that has ACPI companion like we did before anymore.

There is at least one new platform, that the driver supports, which does not have the Designware UART in this "16550 compatible" mode, and I guess it's possible that there are others as well. And I guess the other values can also be what ever on those platforms.

Jérôme, can you test if using the quirk on Baytrails works:

(quoted diff) +0 -0

@@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct
 dw8250_data *data)

                id = acpi_match_device(p->dev->driver->acpi_match_table, p->dev);
-               if (id && !strcmp(id->id, "APMC0D08")) {
+               if (id && (!strcmp(id->id, "APMC0D08") ||
+                   !strcmp(id->id, "80860F0A"))) { p->iotype = UPIO_MEM32;
                        p->regshift = 2;
                        p->serial_in = dw8250_serial_in32;

Please note that this really is not a fix, not even a temporary one for this issue. There are a lot of Baytrails on the market. I just want to make sure there really is a problem delivering those values as device properties with your board.

Hi again,

Quoted 15 lines

Quoted 1 line

Heikki, I don't think anything depends on this commit, is that correct?

Unfortunately we can't fix the values for every UART that has ACPI companion like we did before anymore.

There is at least one new platform, that the driver supports, which does not have the Designware UART in this "16550 compatible" mode, and I guess it's possible that there are others as well. And I guess the other values can also be what ever on those platforms.

Jérôme, can you test if using the quirk on Baytrails works:

(quoted diff) +0 -0

@@ -302,7 +302,8 @@ static void dw8250_quirks(struct uart_port *p, struct
 dw8250_data *data)

Quoted 8 lines

                id = acpi_match_device(p->dev->driver->acpi_match_table, p->dev);
-               if (id && !strcmp(id->id, "APMC0D08")) {
+               if (id && (!strcmp(id->id, "APMC0D08") ||
+                   !strcmp(id->id, "80860F0A"))) { p->iotype = UPIO_MEM32;
                        p->regshift = 2;
                        p->serial_in = dw8250_serial_in32;

Compilation finished with that small modification applied: Bluetooth works indeed in that case and I don't have the error messages in dmesg anymore.

Quoted 1 line

Quoted 4 lines

Please note that this really is not a fix, not even a temporary one for this issue. There are a lot of Baytrails on the market. I just want to make sure there really is a problem delivering those values as device properties with your board.

I guess this confirms there is indeed a problem delivering those values as devices properties on that specific Baytrail device at least.

We have a couple of drivers, acpi_apd.c and acpi_lpss.c, that need to pass extra build-in properties to the devices they create. Previously the drivers added those properties to the struct device which is member of the struct acpi_device, but that does not work. Those properties need to be assigned to the struct device of the platform device instead in order for them to become available to the drivers.

To fix this, this patch changes acpi_create_platform_device function to take struct property_entry pointer as parameter.

Fixes 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")

Signed-off-by Heikki Krogerus <[email protected]>

We have a couple of drivers, acpi_apd.c and acpi_lpss.c, that need to pass extra build-in properties to the devices they create. Previously the drivers added those properties to the struct device which is member of the struct acpi_device, but that does not work. Those properties need to be assigned to the struct device of the platform device instead in order for them to become available to the drivers.

To fix this, this patch changes acpi_create_platform_device function to take struct property_entry pointer as parameter.

Fixes 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")

Signed-off-by Heikki Krogerus <[email protected]>

---

Hi,

Found the problem, and this is my proposal for a fix.

Jérôme, please test it when you have time.

Le jeudi 03 novembre 2016 =C3=A0 16:21 +0200, Heikki Krogerus a =C3=A9cri=

Quoted 20 lines

We have a couple of drivers, acpi_apd.c and acpi_lpss.c, that need to pass extra build-in properties to the devices they create. Previously the drivers added those properties to the struct device which is member of the struct acpi_device, but that does not work. Those properties need to be assigned to the struct device of the platform device instead in order for them to become available to the drivers.

To fix this, this patch changes acpi_create_platform_device function to take struct property_entry pointer as parameter.

Fixes 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")

Signed-off-by Heikki Krogerus <[email protected]>

---

Hi,

Found the problem, and this is my proposal for a fix.

Quoted 2 lines

J=C3=A9r=C3=B4me, please test it when you have time.

Hi Heikki,

I can confirm that this patch fixes the issue I've reported on the Lenovo ThinkPad 8 tablet. You were fast to find the root cause, impressive, real=

appreciated.

Feel free to add:

Tested-by J=C3=A9r=C3=B4me de Bretagne <[email protected]>

Let's hope it can still make it during the remaining 4.9 rc timing.

We have a couple of drivers, acpi_apd.c and acpi_lpss.c, that need to pass extra build-in properties to the devices they create. Previously the drivers added those properties to the struct device which is member of the struct acpi_device, but that does not work. Those properties need to be assigned to the struct device of the platform device instead in order for them to become available to the drivers.

To fix this, this patch changes acpi_create_platform_device function to take struct property_entry pointer as parameter.

Fixes 20a875e2e86e ("serial: 8250_dw: Add quirk for APM X-Gene SoC")

Signed-off-by Heikki Krogerus <[email protected]>

---
 drivers/acpi/acpi_apd.c             | 10 ++-------- drivers/acpi/acpi_lpss.c            | 10 ++-------- drivers/acpi/acpi_platform.c        |  5 ++++- drivers/acpi/dptf/int340x_thermal.c |  4 ++-- drivers/acpi/scan.c                 |  2 +- drivers/platform/x86/intel-hid.c    |  2 +- drivers/platform/x86/intel-vbtn.c   |  2 +- include/linux/acpi.h                |  3 ++-

 8 files changed, 15 insertions(+), 23 deletions(-)

Hi,

Found the problem, and this is my proposal for a fix.

Jérôme, please test it when you have time.

Thanks,

drivers/acpi/acpi_apd.c +0 -0

index 5ec7f3a..26696b6 100644
@@ -127,7 +127,7 @@ static int acpi_apd_create_device(struct
 acpi_device *adev,
  	int ret;

  if (!dev_desc) {

drivers/acpi/acpi_apd.c +1 -1

-		pdev = acpi_create_platform_device(adev);
+		pdev = acpi_create_platform_device(adev, NULL);

  return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1; }

drivers/acpi/acpi_apd.c +0 -0

@@ -144,14 +144,8 @@ static int acpi_apd_create_device(struct
 acpi_device *adev,
  			goto err_out;
  	}

drivers/acpi/acpi_apd.c +0 -2

-	if (dev_desc->properties) {
-		ret = device_add_properties(&adev->dev, dev_desc-

Quoted 1 line

properties);

drivers/acpi/acpi_apd.c +0 -4

-		if (ret)
-			goto err_out;
-	}
-

  adev->driver_data = pdata;

drivers/acpi/acpi_apd.c +1 -1

-	pdev = acpi_create_platform_device(adev);
+	pdev = acpi_create_platform_device(adev, dev_desc-

Quoted 1 line

properties);

  if (!IS_ERR_OR_NULL(pdev))
  return 1;

drivers/acpi/acpi_lpss.c +0 -0

index 5520102..373657f 100644
@@ -395,7 +395,7 @@ static int acpi_lpss_create_device(struct
 acpi_device *adev,

  dev_desc = (const struct lpss_device_desc *)id->driver_data; if (!dev_desc) {

drivers/acpi/acpi_lpss.c +1 -1

-		pdev = acpi_create_platform_device(adev);
+		pdev = acpi_create_platform_device(adev, NULL);

  return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1; }
  pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);

drivers/acpi/acpi_lpss.c +0 -0

@@ -451,14 +451,8 @@ static int acpi_lpss_create_device(struct
 acpi_device *adev,
  		goto err_out;
  	}

drivers/acpi/acpi_lpss.c +0 -2

-	if (dev_desc->properties) {
-		ret = device_add_properties(&adev->dev, dev_desc-

Quoted 1 line

properties);

drivers/acpi/acpi_lpss.c +0 -4

-		if (ret)
-			goto err_out;
-	}
-

  adev->driver_data = pdata;

drivers/acpi/acpi_lpss.c +1 -1

-	pdev = acpi_create_platform_device(adev);
+	pdev = acpi_create_platform_device(adev, dev_desc-

Quoted 1 line

properties);

  if (!IS_ERR_OR_NULL(pdev)) {
  return 1;
  }

drivers/acpi/acpi_platform.c +0 -0

index b200ae1..b4c1a6a 100644
@@ -50,6 +50,7 @@ static void acpi_platform_fill_resource(struct
 acpi_device *adev,
  /**
   * acpi_create_platform_device - Create platform device for ACPI

device node
  * @adev: ACPI device node to create a platform device for.
+ * @properties: Optional collection of build-in properties. *
  * Check if the given @adev can be represented as a platform device and, if
  * that's the case, create and register a platform device, populate its common

drivers/acpi/acpi_platform.c +2 -1

@@ -57,7 +58,8 @@ static void acpi_platform_fill_resource(struct
 acpi_device *adev,
   *
   * Name of the platform device will be the same as @adev's.
   */
-struct platform_device *acpi_create_platform_device(struct
 acpi_device *adev)
+struct platform_device *acpi_create_platform_device(struct
 acpi_device *adev,
+					struct property_entry

*properties)
 {
  struct platform_device *pdev = NULL;
  struct platform_device_info pdevinfo;

drivers/acpi/acpi_platform.c +1 -0

@@ -106,6 +108,7 @@ struct platform_device
 *acpi_create_platform_device(struct acpi_device *adev)
  	pdevinfo.res = resources;
  	pdevinfo.num_res = count;
  	pdevinfo.fwnode = acpi_fwnode_handle(adev);
+	pdevinfo.properties = properties;

  if (acpi_dma_supported(adev)) pdevinfo.dma_mask = DMA_BIT_MASK(32);

drivers/acpi/dptf/int340x_thermal.c +0 -0

index 33505c6..8636409 100644
@@ -34,11 +34,11 @@ static int int340x_thermal_handler_attach(struct
 acpi_device *adev,

  const struct acpi_device_id *id)
 {
  if (IS_ENABLED(CONFIG_INT340X_THERMAL))

drivers/acpi/dptf/int340x_thermal.c +1 -1

-		acpi_create_platform_device(adev);
+		acpi_create_platform_device(adev, NULL);

  /* Intel SoC DTS thermal driver needs INT3401 to set IRQ descriptor */
  else if (IS_ENABLED(CONFIG_INTEL_SOC_DTS_THERMAL) && id->driver_data == INT3401_DEVICE)

drivers/acpi/dptf/int340x_thermal.c +1 -1

-		acpi_create_platform_device(adev);
+		acpi_create_platform_device(adev, NULL);

  return 1;
 }

drivers/acpi/scan.c +1 -1

index 035ac64..3d1856f 100644
@@ -1734,7 +1734,7 @@ static void acpi_default_enumeration(struct
 acpi_device *device)
  			       &is_spi_i2c_slave);
  	acpi_dev_free_resource_list(&resource_list);
  	if (!is_spi_i2c_slave) {
-		acpi_create_platform_device(device);
+		acpi_create_platform_device(device, NULL);
  		acpi_device_set_enumerated(device);
  	} else {

  blocking_notifier_call_chain(&acpi_reconfig_chain,

drivers/platform/x86/intel-hid.c +0 -0

index ed58742..12dbb50 100644
@@ -264,7 +264,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
 *context, void **rv)
  		return AE_OK;

  if (acpi_match_device_ids(dev, ids) == 0)

drivers/platform/x86/intel-hid.c +1 -1

-		if (acpi_create_platform_device(dev))
+		if (acpi_create_platform_device(dev, NULL))

  dev_info(&dev->dev, "intel-hid: created platform device\n");

drivers/platform/x86/intel-vbtn.c +0 -0

index 146d02f..7808076 100644
@@ -164,7 +164,7 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
 *context, void **rv)
  		return AE_OK;

  if (acpi_match_device_ids(dev, ids) == 0)

drivers/platform/x86/intel-vbtn.c +1 -1

-		if (acpi_create_platform_device(dev))
+		if (acpi_create_platform_device(dev, NULL))

  dev_info(&dev->dev, "intel-vbtn: created platform device\n");

include/linux/acpi.h +0 -0

index 689a8b9..61a3d90 100644
@@ -555,7 +555,8 @@ int acpi_device_uevent_modalias(struct device *,
 struct kobj_uevent_env *);
  int acpi_device_modalias(struct device *, char *, int);
  void acpi_walk_dep_device_list(acpi_handle handle);

-struct platform_device *acpi_create_platform_device(struct acpi_device *);
+struct platform_device *acpi_create_platform_device(struct acpi_device *,

include/linux/acpi.h +1 -0

+						    struct

property_entry *);
 #define ACPI_PTR(_ptr) (_ptr)

 static inline void acpi_device_set_enumerated(struct acpi_device *adev)