All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 13/13] Bluetooth: btbcm: Fix sleep mode struct ordering
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (7 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 08/13] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 05/13] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable() Lukas Wunner
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

According to the documentation for Laird SD40 radio modules (which use
the BCM4329 chipset), the order of the Enable_BREAK_To_Host and
Pulsed_HOST_WAKE parameters in the sleep mode struct is reversed
vis-à-vis our struct declaration.  See page 46 of this PDF:

http://cdn.lairdtech.com/home/brandworld/files/Application%20Note%20-%2040%20Series%20Bluetooth.pdf

The documentation is dated Oct 2015, so fairly recent, making it appear
more likely that the documentation is correct and our code is wrong.
Amend our code to be in congruence with the documentation.

Cc: Sue White <sue.white@lairdtech.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/btbcm.h   | 2 +-
 drivers/bluetooth/hci_bcm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index d9e6b41658e5..cfe6ad4cc621 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -44,8 +44,8 @@ struct bcm_set_sleep_mode {
 	__u8 tristate_control;
 	__u8 usb_auto_sleep;
 	__u8 usb_resume_timeout;
-	__u8 pulsed_host_wake;
 	__u8 break_to_host;
+	__u8 pulsed_host_wake;
 } __packed;
 
 struct bcm_set_pcm_int_params {
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 461364a00b20..64800cd2796c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -300,8 +300,8 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
 	/* Irrelevant USB flags */
 	.usb_auto_sleep = 0,
 	.usb_resume_timeout = 0,
+	.break_to_host = 0,
 	.pulsed_host_wake = 0,
-	.break_to_host = 0
 };
 
 static int bcm_setup_sleep(struct hci_uart *hu)
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 12/13] Bluetooth: hci_bcm: Sleep instead of spinning
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (4 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 07/13] Bluetooth: hci_bcm: Document struct bcm_device Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 11/13] Bluetooth: hci_bcm: Silence IRQ printk Lukas Wunner
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

The driver calls mdelay(15) in the ->suspend, ->resume, ->runtime_suspend
and ->runtime_resume hook, however spinning for such a long period of
time is discouraged as per Documentation/timers/timers-howto.txt.

The use of mdelay() seems unnecessary, it is allowed to sleep in the
system sleep and runtime PM hooks (with the exception of ->suspend_noirq
and ->resume_noirq) and the driver itself also does not rely on a
non-sleeping ->runtime_resume as the only place where a synchronous
resume is performed, in bcm_dequeue(), is called from a work item in
hci_ldisc.c and hci_serdev.c.

So replace the mdelay(15) with msleep(15).

Note that the delay is inserted after asserting or deasserting the
device wake pin, but in bcm_gpio_set_power() that pin is asserted or
deasserted *without* observing a delay.  It is thus unclear if the delay
is necessary at all.  It is likewise unclear why it is exactly 15 ms,
the commit introducing it, 118612fb9165 ("Bluetooth: hci_bcm: Add
suspend/resume PM functions"), does not provide a rationale.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Suggested-and-reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0929a264bffa..461364a00b20 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -656,7 +656,7 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-	mdelay(15);
+	msleep(15);
 
 	return 0;
 }
@@ -675,7 +675,7 @@ static int bcm_resume_device(struct device *dev)
 	}
 
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
-	mdelay(15);
+	msleep(15);
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 11/13] Bluetooth: hci_bcm: Silence IRQ printk
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (5 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 12/13] Bluetooth: hci_bcm: Sleep instead of spinning Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 08/13] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs Lukas Wunner
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

The host wake IRQ is optional, but if none is found, "BCM irq: -22" is
logged which may irritate users.  This is really a debug message, so use
dev_dbg() instead of dev_info().  If users are interested in the IRQ,
they can always consult /proc/interrupts.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 3e818a429fc5..0929a264bffa 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -943,7 +943,7 @@ static int bcm_get_resources(struct bcm_device *dev)
 		dev->irq = gpiod_to_irq(gpio);
 	}
 
-	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
+	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
 	return 0;
 }
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (11 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 09/13] Bluetooth: hci_bcm: Handle errors properly Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 18:02 ` [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Marcel Holtmann
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

Enable Bluetooth on the following Macs which provide custom ACPI methods
to toggle the GPIOs for device wake and shutdown instead of accessing
the pins directly:

    MacBook8,1     2015  12"
    MacBook9,1     2016  12"
    MacBook10,1    2017  12"
    MacBookPro13,1 2016  13"
    MacBookPro13,2 2016  13" with Touch Bar
    MacBookPro13,3 2016  15" with Touch Bar
    MacBookPro14,1 2017  13"
    MacBookPro14,2 2017  13" with Touch Bar
    MacBookPro14,3 2017  15" with Touch Bar

On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
on the SSD) under the control of PCH GPIO 36.  Because serdev cannot
deal with multiple slaves yet, it is currently necessary to patch the
DSDT and remove the SSDC device.

The custom ACPI methods are called:

    BTLP (Low Power) takes one argument, toggles device wake GPIO
    BTPU (Power Up) tells SMC to drive shutdown GPIO high
    BTPD (Power Down) tells SMC to drive shutdown GPIO low
    BTRS (Reset) calls BTPD followed by BTPU
    BTRB unknown, not present on all MacBooks

Search for the BTLP, BTPU and BTPD methods on ->probe and cache them in
struct bcm_device if the machine is a Mac.

Additionally, set the init_speed based on a custom device property
provided by Apple in lieu of _CRS resources.  The Broadcom UART's speed
is fixed on Apple Macs:  Any attempt to change it results in Bluetooth
status code 0x0c and bcm_set_baudrate() thus always returns -EBUSY.
By setting only the init_speed and leaving oper_speed at zero, we can
achieve that the host UART's speed is adjusted but the Broadcom UART's
speed is left as is.

The host wake pin goes into the SMC which handles it independently
of the OS, so there's no IRQ for it.

Thanks to Ronald Tschalär who did extensive debugging and testing of
this patch and contributed fixes.

ACPI snippet containing the custom methods and device properties
(taken from a MacBook8,1):

    Method (BTLP, 1, Serialized)
    {
        If (LEqual (Arg0, 0x00))
        {
            Store (0x01, GD54) /* set PCH GPIO 54 direction to input */
        }

        If (LEqual (Arg0, 0x01))
        {
            Store (0x00, GD54) /* set PCH GPIO 54 direction to output */
            Store (0x00, GP54) /* set PCH GPIO 54 value to low */
        }
    }

    Method (BTPU, 0, Serialized)
    {
        Store (0x01, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTPD, 0, Serialized)
    {
        Store (0x00, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTRS, 0, Serialized)
    {
        BTPD ()
        BTPU ()
    }

    Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
    {
        If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
        {
            Store (Package (0x08)
                {
                    "baud",
                    Buffer (0x08)
                    { 0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "parity",
                    Buffer (0x08)
                    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "dataBits",
                    Buffer (0x08)
                    { 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "stopBits",
                    Buffer (0x08)
                    { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }
                }, Local0)
            DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
            Return (Local0)
        }
        Return (0x00)
    }

Link: https://github.com/Dunedan/mbp-2016-linux/issues/29
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
Reported-by: Leif Liddy <leif.liddy@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Max Shavrick <mxms@me.com>                     [MacBook8,1]
Tested-by: Leif Liddy <leif.liddy@gmail.com>              [MacBook9,1]
Tested-by: Daniel Roschka <danielroschka@phoenitydawn.de> [MacBookPro13,2]
Tested-by: Ronald Tschalär <ronald@innovation.ch>         [MacBookPro13,3]
Tested-by: Peter Y. Chuang <peteryuchuang@gmail.com>      [MacBookPro14,1]
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v3:
  Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)
Changes since v2:
  Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
  s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)

 drivers/bluetooth/hci_bcm.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 03a365148184..3e818a429fc5 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -29,6 +29,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/property.h>
+#include <linux/platform_data/x86/apple.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
@@ -64,7 +65,12 @@
  * @shutdown: BT_REG_ON pin,
  *	power up or power down Bluetooth device internal regulators
  * @set_device_wakeup: callback to toggle BT_WAKE pin
+ *	either by accessing @device_wakeup or by calling @btlp
  * @set_shutdown: callback to toggle BT_REG_ON pin
+ *	either by accessing @shutdown or by calling @btpu/@btpd
+ * @btlp: Apple ACPI method to toggle BT_WAKE pin ("Bluetooth Low Power")
+ * @btpu: Apple ACPI method to drive BT_REG_ON pin high ("Bluetooth Power Up")
+ * @btpd: Apple ACPI method to drive BT_REG_ON pin low ("Bluetooth Power Down")
  * @clk: clock used by Bluetooth device
  * @clk_enabled: whether @clk is prepared and enabled
  * @init_speed: default baudrate of Bluetooth device;
@@ -90,6 +96,9 @@ struct bcm_device {
 	struct gpio_desc	*shutdown;
 	int			(*set_device_wakeup)(struct bcm_device *, bool);
 	int			(*set_shutdown)(struct bcm_device *, bool);
+#ifdef CONFIG_ACPI
+	acpi_handle		btlp, btpu, btpd;
+#endif
 
 	struct clk		*clk;
 	bool			clk_enabled;
@@ -844,6 +853,49 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 
 	return 0;
 }
+
+static int bcm_apple_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+	if (ACPI_FAILURE(acpi_execute_simple_method(dev->btlp, NULL, !awake)))
+		return -EIO;
+
+	return 0;
+}
+
+static int bcm_apple_set_shutdown(struct bcm_device *dev, bool powered)
+{
+	if (ACPI_FAILURE(acpi_evaluate_object(powered ? dev->btpu : dev->btpd,
+					      NULL, NULL, NULL)))
+		return -EIO;
+
+	return 0;
+}
+
+static int bcm_apple_get_resources(struct bcm_device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev->dev);
+	const union acpi_object *obj;
+
+	if (!adev ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTLP", &dev->btlp)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPU", &dev->btpu)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPD", &dev->btpd)))
+		return -ENODEV;
+
+	if (!acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER, &obj) &&
+	    obj->buffer.length == 8)
+		dev->init_speed = *(u64 *)obj->buffer.pointer;
+
+	dev->set_device_wakeup = bcm_apple_set_device_wakeup;
+	dev->set_shutdown = bcm_apple_set_shutdown;
+
+	return 0;
+}
+#else
+static inline int bcm_apple_get_resources(struct bcm_device *dev)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_ACPI */
 
 static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
@@ -862,6 +914,9 @@ static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
 
+	if (x86_apple_machine && !bcm_apple_get_resources(dev))
+		return 0;
+
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
 	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 09/13] Bluetooth: hci_bcm: Handle errors properly
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (10 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 06/13] Bluetooth: hci_bcm: Invalidate IRQ on request failure Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling Lukas Wunner
  2018-01-10 18:02 ` [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Marcel Holtmann
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

A significant portion of this driver lacks error handling.  As a first
step, add error paths to bcm_gpio_set_power(), bcm_open(), bcm_close(),
bcm_suspend_device(), bcm_resume_device(), bcm_resume(), bcm_probe() and
bcm_serdev_probe().  (I've also scrutinized bcm_suspend() but think it's
fine as is.)

Those are all the functions accessing the device wake and shutdown GPIO.
On Apple Macs the pins are accessed through ACPI methods, which may fail
for various reasons, hence proper error handling is necessary.  Non-Macs
access the pins directly, which may fail as well but the GPIO core does
not yet pass back errors to consumers.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v3:
  Fix corner case where an unbalanced pm_runtime_disable() could occur.
Changes since v2:
  Drop redundant assignment. (Andy)

 drivers/bluetooth/hci_bcm.c | 91 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8741e302e6fd..03a365148184 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -197,11 +197,21 @@ static bool bcm_device_exists(struct bcm_device *device)
 
 static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 {
-	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
-		clk_prepare_enable(dev->clk);
+	int err;
 
-	dev->set_shutdown(dev, powered);
-	dev->set_device_wakeup(dev, powered);
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
+		err = clk_prepare_enable(dev->clk);
+		if (err)
+			return err;
+	}
+
+	err = dev->set_shutdown(dev, powered);
+	if (err)
+		goto err_clk_disable;
+
+	err = dev->set_device_wakeup(dev, powered);
+	if (err)
+		goto err_revert_shutdown;
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -209,6 +219,13 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	dev->clk_enabled = powered;
 
 	return 0;
+
+err_revert_shutdown:
+	dev->set_shutdown(dev, !powered);
+err_clk_disable:
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+		clk_disable_unprepare(dev->clk);
+	return err;
 }
 
 #ifdef CONFIG_PM
@@ -331,6 +348,7 @@ static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
 	struct list_head *p;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -345,7 +363,10 @@ static int bcm_open(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 
 	if (hu->serdev) {
-		serdev_device_open(hu->serdev);
+		err = serdev_device_open(hu->serdev);
+		if (err)
+			goto err_free;
+
 		bcm->dev = serdev_device_get_drvdata(hu->serdev);
 		goto out;
 	}
@@ -373,17 +394,30 @@ static int bcm_open(struct hci_uart *hu)
 	if (bcm->dev) {
 		hu->init_speed = bcm->dev->init_speed;
 		hu->oper_speed = bcm->dev->oper_speed;
-		bcm_gpio_set_power(bcm->dev, true);
+		err = bcm_gpio_set_power(bcm->dev, true);
+		if (err)
+			goto err_unset_hu;
 	}
 
 	mutex_unlock(&bcm_device_lock);
 	return 0;
+
+err_unset_hu:
+#ifdef CONFIG_PM
+	bcm->dev->hu = NULL;
+#endif
+err_free:
+	mutex_unlock(&bcm_device_lock);
+	hu->priv = NULL;
+	kfree(bcm);
+	return err;
 }
 
 static int bcm_close(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
 	struct bcm_device *bdev = NULL;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -407,8 +441,11 @@ static int bcm_close(struct hci_uart *hu)
 			pm_runtime_disable(bdev->dev);
 		}
 
-		bcm_gpio_set_power(bdev, false);
-		pm_runtime_set_suspended(bdev->dev);
+		err = bcm_gpio_set_power(bdev, false);
+		if (err)
+			bt_dev_err(hu->hdev, "Failed to power down");
+		else
+			pm_runtime_set_suspended(bdev->dev);
 	}
 	mutex_unlock(&bcm_device_lock);
 
@@ -588,6 +625,7 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 static int bcm_suspend_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
@@ -599,7 +637,15 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	bdev->set_device_wakeup(bdev, false);
+	err = bdev->set_device_wakeup(bdev, false);
+	if (err) {
+		if (bdev->is_suspended && bdev->hu) {
+			bdev->is_suspended = false;
+			hci_uart_set_flow_control(bdev->hu, false);
+		}
+		return -EBUSY;
+	}
+
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
 	mdelay(15);
 
@@ -609,10 +655,16 @@ static int bcm_suspend_device(struct device *dev)
 static int bcm_resume_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
-	bdev->set_device_wakeup(bdev, true);
+	err = bdev->set_device_wakeup(bdev, true);
+	if (err) {
+		dev_err(dev, "Failed to power up\n");
+		return err;
+	}
+
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
 	mdelay(15);
 
@@ -666,6 +718,7 @@ static int bcm_suspend(struct device *dev)
 static int bcm_resume(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err = 0;
 
 	bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
 
@@ -685,14 +738,16 @@ static int bcm_resume(struct device *dev)
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
 
-	bcm_resume_device(dev);
+	err = bcm_resume_device(dev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	if (!err) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
 
 	return 0;
 }
@@ -923,7 +978,9 @@ static int bcm_probe(struct platform_device *pdev)
 	list_add_tail(&dev->list, &bcm_device_list);
 	mutex_unlock(&bcm_device_lock);
 
-	bcm_gpio_set_power(dev, false);
+	ret = bcm_gpio_set_power(dev, false);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to power down\n");
 
 	return 0;
 }
@@ -1025,7 +1082,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 	if (err)
 		return err;
 
-	bcm_gpio_set_power(bcmdev, false);
+	err = bcm_gpio_set_power(bcmdev, false);
+	if (err)
+		dev_err(&serdev->dev, "Failed to power down\n");
 
 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 08/13] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (6 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 11/13] Bluetooth: hci_bcm: Silence IRQ printk Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 13/13] Bluetooth: btbcm: Fix sleep mode struct ordering Lukas Wunner
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

MacBooks provides custom ACPI methods to toggle the GPIOs for device
wake and shutdown instead of accessing the pins directly.  Prepare for
their support by adding callbacks to toggle the GPIOs, which on non-Macs
do nothing more but call gpiod_set_value().

No functional change intended.

Suggested-and-reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v2:
  Replaces patch "Add helper to toggle device wake GPIO" in v2.
  This makes the subsequent addition of Mac support less intrusive. (Andy)

 drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 3c1282a9fcd4..8741e302e6fd 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -63,6 +63,8 @@
  *	deassert = Bluetooth device may sleep when sleep criteria are met
  * @shutdown: BT_REG_ON pin,
  *	power up or power down Bluetooth device internal regulators
+ * @set_device_wakeup: callback to toggle BT_WAKE pin
+ * @set_shutdown: callback to toggle BT_REG_ON pin
  * @clk: clock used by Bluetooth device
  * @clk_enabled: whether @clk is prepared and enabled
  * @init_speed: default baudrate of Bluetooth device;
@@ -86,6 +88,8 @@ struct bcm_device {
 	const char		*name;
 	struct gpio_desc	*device_wakeup;
 	struct gpio_desc	*shutdown;
+	int			(*set_device_wakeup)(struct bcm_device *, bool);
+	int			(*set_shutdown)(struct bcm_device *, bool);
 
 	struct clk		*clk;
 	bool			clk_enabled;
@@ -196,8 +200,8 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
 		clk_prepare_enable(dev->clk);
 
-	gpiod_set_value(dev->shutdown, powered);
-	gpiod_set_value(dev->device_wakeup, powered);
+	dev->set_shutdown(dev, powered);
+	dev->set_device_wakeup(dev, powered);
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -595,7 +599,7 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	gpiod_set_value(bdev->device_wakeup, false);
+	bdev->set_device_wakeup(bdev, false);
 	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
 	mdelay(15);
 
@@ -608,7 +612,7 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	gpiod_set_value(bdev->device_wakeup, true);
+	bdev->set_device_wakeup(bdev, true);
 	bt_dev_dbg(bdev, "resume, delaying 15 ms");
 	mdelay(15);
 
@@ -787,6 +791,18 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 }
 #endif /* CONFIG_ACPI */
 
+static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+	gpiod_set_value(dev->device_wakeup, awake);
+	return 0;
+}
+
+static int bcm_gpio_set_shutdown(struct bcm_device *dev, bool powered)
+{
+	gpiod_set_value(dev->shutdown, powered);
+	return 0;
+}
+
 static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
@@ -802,6 +818,9 @@ static int bcm_get_resources(struct bcm_device *dev)
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
+	dev->set_device_wakeup = bcm_gpio_set_device_wakeup;
+	dev->set_shutdown = bcm_gpio_set_shutdown;
+
 	/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
 	if (dev->irq <= 0) {
 		struct gpio_desc *gpio;
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 07/13] Bluetooth: hci_bcm: Document struct bcm_device
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (3 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 04/13] Bluetooth: hci_bcm: Fix race on close Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 12/13] Bluetooth: hci_bcm: Sleep instead of spinning Lukas Wunner
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v3:
  s/enable flow control/disable flow control/ to account for inverse
  semantics of hci_uart_set_flow_control().

 drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index c893c597fead..3c1282a9fcd4 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -52,7 +52,30 @@
 
 #define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
 
-/* device driver resources */
+/**
+ * struct bcm_device - device driver resources
+ * @serdev_hu: HCI UART controller struct
+ * @list: bcm_device_list node
+ * @dev: physical UART slave
+ * @name: device name logged by bt_dev_*() functions
+ * @device_wakeup: BT_WAKE pin,
+ *	assert = Bluetooth device must wake up or remain awake,
+ *	deassert = Bluetooth device may sleep when sleep criteria are met
+ * @shutdown: BT_REG_ON pin,
+ *	power up or power down Bluetooth device internal regulators
+ * @clk: clock used by Bluetooth device
+ * @clk_enabled: whether @clk is prepared and enabled
+ * @init_speed: default baudrate of Bluetooth device;
+ *	the host UART is initially set to this baudrate so that
+ *	it can configure the Bluetooth device for @oper_speed
+ * @oper_speed: preferred baudrate of Bluetooth device;
+ *	set to 0 if @init_speed is already the preferred baudrate
+ * @irq: interrupt triggered by HOST_WAKE_BT pin
+ * @irq_active_low: whether @irq is active low
+ * @hu: pointer to HCI UART controller struct,
+ *	used to disable flow control during runtime suspend and system sleep
+ * @is_suspended: whether flow control is currently disabled
+ */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
 	struct hci_uart		serdev_hu;
@@ -74,7 +97,7 @@ struct bcm_device {
 
 #ifdef CONFIG_PM
 	struct hci_uart		*hu;
-	bool			is_suspended; /* suspend/resume flag */
+	bool			is_suspended;
 #endif
 };
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 06/13] Bluetooth: hci_bcm: Invalidate IRQ on request failure
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (9 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 05/13] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable() Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 09/13] Bluetooth: hci_bcm: Handle errors properly Lukas Wunner
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

If devm_request_irq() fails, the driver bails out of bcm_request_irq()
but continues to ->setup the device (because the IRQ is optional).

The driver subsequently calls devm_free_irq(), enable_irq_wake() and
disable_irq_wake() on the IRQ even though requesting it failed.

Avoid by invalidating the IRQ on request failure.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 140c7e5bf812..c893c597fead 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -216,8 +216,10 @@ static int bcm_request_irq(struct bcm_data *bcm)
 			       bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
 						      IRQF_TRIGGER_RISING,
 			       "host_wake", bdev);
-	if (err)
+	if (err) {
+		bdev->irq = err;
 		goto unlock;
+	}
 
 	device_init_wakeup(bdev->dev, true);
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 05/13] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (8 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 13/13] Bluetooth: btbcm: Fix sleep mode struct ordering Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 06/13] Bluetooth: hci_bcm: Invalidate IRQ on request failure Lukas Wunner
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

On ->setup, pm_runtime_enable() is only called if a valid IRQ was found,
but on ->close(), pm_runtime_disable() is called unconditionally.
Disablement of runtime PM is recorded in a counter, so every
pm_runtime_disable() needs to be balanced.  Fix it.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reported-and-reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 6144a3f9c37a..140c7e5bf812 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -375,10 +375,10 @@ static int bcm_close(struct hci_uart *hu)
 		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
+			pm_runtime_disable(bdev->dev);
 		}
 
 		bcm_gpio_set_power(bdev, false);
-		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 	}
 	mutex_unlock(&bcm_device_lock);
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 04/13] Bluetooth: hci_bcm: Fix race on close
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (2 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 03/13] Bluetooth: hci_bcm: Clean up unnecessary #ifdef Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 07/13] Bluetooth: hci_bcm: Document struct bcm_device Lukas Wunner
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

Upon ->close, the driver powers the Bluetooth controller down, deasserts
the device wake pin, updates the runtime PM status to "suspended" and
finally frees the IRQ.

Because the IRQ is freed last, a runtime resume can take place after
the controller was powered down.  The impact is not grave, the worst
thing that can happen is that the device wake pin is reasserted (should
have no effect while the regulator is off) and that setting the runtime
PM status to "suspended" does not reflect reality.

Still, it's wrong, so free the IRQ first.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index c551ef4c350f..6144a3f9c37a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -372,14 +372,14 @@ static int bcm_close(struct hci_uart *hu)
 	}
 
 	if (bdev) {
-		bcm_gpio_set_power(bdev, false);
-		pm_runtime_disable(bdev->dev);
-		pm_runtime_set_suspended(bdev->dev);
-
 		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
+
+		bcm_gpio_set_power(bdev, false);
+		pm_runtime_disable(bdev->dev);
+		pm_runtime_set_suspended(bdev->dev);
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 03/13] Bluetooth: hci_bcm: Clean up unnecessary #ifdef
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 02/13] Bluetooth: hci_bcm: Validate IRQ before using it Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 04/13] Bluetooth: hci_bcm: Fix race on close Lukas Wunner
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

pm_runtime_disable() and pm_runtime_set_suspended() are replaced with
empty inlines if CONFIG_PM is disabled, so there's no need to #ifdef
them.

device_init_wakeup() is likewise replaced with an inline, though it's
not empty, but it and devm_free_irq() can be made conditional on
IS_ENABLED(CONFIG_PM), which is preferable to #ifdef as per section 20
of Documentation/process/coding-style.rst.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index d9f71ed26667..c551ef4c350f 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -373,15 +373,13 @@ static int bcm_close(struct hci_uart *hu)
 
 	if (bdev) {
 		bcm_gpio_set_power(bdev, false);
-#ifdef CONFIG_PM
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (bdev->irq > 0) {
+		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
-#endif
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 02/13] Bluetooth: hci_bcm: Validate IRQ before using it
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 03/13] Bluetooth: hci_bcm: Clean up unnecessary #ifdef Lukas Wunner
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

From: Ronald Tschalär <ronald@innovation.ch>

The ->close, ->suspend and ->resume hooks assume presence of a valid IRQ
if the device is wakeup capable.  However it's entirely possible that
wakeup was enabled by some other entity besides this driver and in this
case the user will get a WARN splat if no valid IRQ was found.  Avoid by
checking if the IRQ is valid, i.e. > 0.

Case in point:  On recent MacBook Pros, the Bluetooth device lacks an
IRQ (because host wakeup is handled by the SMC, independently of the
operating system), but it does possess a _PRW method (which specifies
the SMC's GPE as wake event).  The ACPI core therefore automatically
marks the physical Bluetooth device wakeup capable upon binding it to
its ACPI companion:

device_set_wakeup_capable+0x96/0xb0
acpi_bind_one+0x28a/0x310
acpi_platform_notify+0x20/0xa0
device_add+0x215/0x690
serdev_device_add+0x57/0xf0
acpi_serdev_add_device+0xc9/0x110
acpi_ns_walk_namespace+0x131/0x280
acpi_walk_namespace+0xf5/0x13d
serdev_controller_add+0x6f/0x110
serdev_tty_port_register+0x98/0xf0
tty_port_register_device_attr_serdev+0x3a/0x70
uart_add_one_port+0x268/0x500
serial8250_register_8250_port+0x32e/0x490
dw8250_probe+0x46c/0x720
platform_drv_probe+0x35/0x90
driver_probe_device+0x300/0x450
bus_for_each_drv+0x67/0xb0
__device_attach+0xde/0x160
bus_probe_device+0x9c/0xb0
device_add+0x448/0x690
platform_device_add+0x10e/0x260
mfd_add_device+0x392/0x4c0
mfd_add_devices+0xb1/0x110
intel_lpss_probe+0x2a9/0x610 [intel_lpss]
intel_lpss_pci_probe+0x7a/0xa8 [intel_lpss_pci]

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
[lukas: fix up ->suspend and ->resume as well, add commit message]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7d059b35c63a..d9f71ed26667 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -377,7 +377,7 @@ static int bcm_close(struct hci_uart *hu)
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (device_can_wakeup(bdev->dev)) {
+		if (bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
@@ -623,7 +623,7 @@ static int bcm_suspend(struct device *dev)
 	if (pm_runtime_active(dev))
 		bcm_suspend_device(dev);
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		error = enable_irq_wake(bdev->irq);
 		if (!error)
 			bt_dev_dbg(bdev, "BCM irq: enabled");
@@ -653,7 +653,7 @@ static int bcm_resume(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		disable_irq_wake(bdev->irq);
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
@ 2018-01-10 15:32 ` Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 02/13] Bluetooth: hci_bcm: Validate IRQ before using it Lukas Wunner
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
amended this driver to request a shutdown and device wake GPIO on probe,
but mandated that only one of them need to be present:

	/* Make sure at-least one of the GPIO is defined and that
	 * a name is specified for this instance
	 */
	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
		dev_err(&pdev->dev, "invalid platform data\n");
		return -EINVAL;
	}

However the same commit added a call to bcm_gpio_set_power() to the
->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
the resulting NULL pointer deref was never reported, suggesting there's
no machine where either GPIO is missing.

Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
serdev driver") removed the check whether at least one of the GPIOs is
present without specifying a reason.

Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
API") refactored the driver to use devm_gpiod_get_optional() instead of
devm_gpiod_get(), one is now tempted to believe that the driver doesn't
require *any* of the two GPIOs.

Which is wrong, the driver still requires both GPIOs to avoid a NULL
pointer deref.  To this end, establish the status quo ante and request
the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
of them is missing.

Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
unconditionally, bcm_suspend_device() and bcm_resume_device() do check
for its presence before accessing it.  Those checks are superfluous,
so remove them.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v2:
  Keep netdev indentation style. (Marcel)

 drivers/bluetooth/hci_bcm.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1fc604a0d870..7d059b35c63a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -572,11 +572,9 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, false);
-		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-		mdelay(15);
-	}
+	gpiod_set_value(bdev->device_wakeup, false);
+	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+	mdelay(15);
 
 	return 0;
 }
@@ -587,11 +585,9 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, true);
-		bt_dev_dbg(bdev, "resume, delaying 15 ms");
-		mdelay(15);
-	}
+	gpiod_set_value(bdev->device_wakeup, true);
+	bt_dev_dbg(bdev, "resume, delaying 15 ms");
+	mdelay(15);
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
@@ -774,14 +770,12 @@ static int bcm_get_resources(struct bcm_device *dev)
 
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
-	dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
-						     "device-wakeup",
-						     GPIOD_OUT_LOW);
+	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
+					    GPIOD_OUT_LOW);
 	if (IS_ERR(dev->device_wakeup))
 		return PTR_ERR(dev->device_wakeup);
 
-	dev->shutdown = devm_gpiod_get_optional(dev->dev, "shutdown",
-						GPIOD_OUT_LOW);
+	dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro)
@ 2018-01-10 15:32 Lukas Wunner
  2018-01-10 15:32 ` [PATCH v4 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO Lukas Wunner
                   ` (13 more replies)
  0 siblings, 14 replies; 15+ messages in thread
From: Lukas Wunner @ 2018-01-10 15:32 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg
  Cc: Mika Westerberg, Andy Shevchenko, Frederic Danis, Loic Poulain,
	Hans de Goede, Max Shavrick, Leif Liddy, Daniel Roschka,
	Ronald Tschalaer, Peter Y. Chuang, linux-bluetooth

Enable UART-attached Bluetooth on 2015+ MacBook (Pro), v4.

I forgot to include a changelog in each individual patch in v3,
I'm making up for it now by listing the changes since both, v3 and v2.

Changes since v3:

- In patch [07/13] ("Document struct bcm_device"):
  s/enable flow control/disable flow control/ to account for inverse
  semantics of hci_uart_set_flow_control().

- In patch [05/13] and [09/13] ("Handle errors properly"):
  Fix corner case where an unbalanced pm_runtime_disable() could occur.

- In patch [10/13] ("Support Apple GPIO handling"):
  Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)

Changes since v2:

- In patch [01/13] ("Mandate presence of shutdown and device wake GPIO"):
  Keep netdev indentation style. (Marcel)

- New patch [04/13] to fix an IRQ race on ->close.

- New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)

- New patch [06/13] to invalidate the IRQ if requesting it failed.

- New patch [08/13] to add callbacks to toggle GPIOs.
  This makes the subsequent addition of Mac support less intrusive.
  Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)

- In patch [09/13] ("Handle errors properly"):
  Drop redundant assignment. (Andy)

- In patch [10/13] ("Support Apple GPIO handling"):
  Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
  s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)

- Move patch "Silence IRQ printk" to end of series as it's merely
  a cleanup and no longer necessary for Mac support.

- New patch [12/13] to use msleep() instead of mdelay()
  after toggling device wake pin. (Andy)

- New patch [13/13] to fix sleep mode struct ordering.

- Drop incorrect patch "Enable runtime PM despite absence of IRQ". (Hans)

Link to v3:
https://www.spinics.net/lists/linux-bluetooth/msg73705.html

Link to v2:
https://www.spinics.net/lists/linux-bluetooth/msg73628.html

Thanks,

Lukas


Lukas Wunner (12):
  Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
  Bluetooth: hci_bcm: Clean up unnecessary #ifdef
  Bluetooth: hci_bcm: Fix race on close
  Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
  Bluetooth: hci_bcm: Invalidate IRQ on request failure
  Bluetooth: hci_bcm: Document struct bcm_device
  Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
  Bluetooth: hci_bcm: Handle errors properly
  Bluetooth: hci_bcm: Support Apple GPIO handling
  Bluetooth: hci_bcm: Silence IRQ printk
  Bluetooth: hci_bcm: Sleep instead of spinning
  Bluetooth: btbcm: Fix sleep mode struct ordering

Ronald Tschalär (1):
  Bluetooth: hci_bcm: Validate IRQ before using it

 drivers/bluetooth/btbcm.h   |   2 +-
 drivers/bluetooth/hci_bcm.c | 228 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 190 insertions(+), 40 deletions(-)

-- 
2.15.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro)
  2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
                   ` (12 preceding siblings ...)
  2018-01-10 15:32 ` [PATCH v4 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling Lukas Wunner
@ 2018-01-10 18:02 ` Marcel Holtmann
  13 siblings, 0 replies; 15+ messages in thread
From: Marcel Holtmann @ 2018-01-10 18:02 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Johan Hedberg, Mika Westerberg, Andy Shevchenko, Frederic Danis,
	Loic Poulain, Hans de Goede, Max Shavrick, Leif Liddy,
	Daniel Roschka, Ronald Tschalaer, Peter Y. Chuang,
	linux-bluetooth

Hi Lukas,

> Enable UART-attached Bluetooth on 2015+ MacBook (Pro), v4.
> 
> I forgot to include a changelog in each individual patch in v3,
> I'm making up for it now by listing the changes since both, v3 and v2.
> 
> Changes since v3:
> 
> - In patch [07/13] ("Document struct bcm_device"):
>  s/enable flow control/disable flow control/ to account for inverse
>  semantics of hci_uart_set_flow_control().
> 
> - In patch [05/13] and [09/13] ("Handle errors properly"):
>  Fix corner case where an unbalanced pm_runtime_disable() could occur.
> 
> - In patch [10/13] ("Support Apple GPIO handling"):
>  Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)
> 
> Changes since v2:
> 
> - In patch [01/13] ("Mandate presence of shutdown and device wake GPIO"):
>  Keep netdev indentation style. (Marcel)
> 
> - New patch [04/13] to fix an IRQ race on ->close.
> 
> - New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)
> 
> - New patch [06/13] to invalidate the IRQ if requesting it failed.
> 
> - New patch [08/13] to add callbacks to toggle GPIOs.
>  This makes the subsequent addition of Mac support less intrusive.
>  Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)
> 
> - In patch [09/13] ("Handle errors properly"):
>  Drop redundant assignment. (Andy)
> 
> - In patch [10/13] ("Support Apple GPIO handling"):
>  Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
>  s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
> 
> - Move patch "Silence IRQ printk" to end of series as it's merely
>  a cleanup and no longer necessary for Mac support.
> 
> - New patch [12/13] to use msleep() instead of mdelay()
>  after toggling device wake pin. (Andy)
> 
> - New patch [13/13] to fix sleep mode struct ordering.
> 
> - Drop incorrect patch "Enable runtime PM despite absence of IRQ". (Hans)
> 
> Link to v3:
> https://www.spinics.net/lists/linux-bluetooth/msg73705.html
> 
> Link to v2:
> https://www.spinics.net/lists/linux-bluetooth/msg73628.html
> 
> Thanks,
> 
> Lukas
> 
> 
> Lukas Wunner (12):
>  Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
>  Bluetooth: hci_bcm: Clean up unnecessary #ifdef
>  Bluetooth: hci_bcm: Fix race on close
>  Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
>  Bluetooth: hci_bcm: Invalidate IRQ on request failure
>  Bluetooth: hci_bcm: Document struct bcm_device
>  Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
>  Bluetooth: hci_bcm: Handle errors properly
>  Bluetooth: hci_bcm: Support Apple GPIO handling
>  Bluetooth: hci_bcm: Silence IRQ printk
>  Bluetooth: hci_bcm: Sleep instead of spinning
>  Bluetooth: btbcm: Fix sleep mode struct ordering
> 
> Ronald Tschalär (1):
>  Bluetooth: hci_bcm: Validate IRQ before using it
> 
> drivers/bluetooth/btbcm.h   |   2 +-
> drivers/bluetooth/hci_bcm.c | 228 ++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 190 insertions(+), 40 deletions(-)

all 13 patches have been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-01-10 18:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10 15:32 [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 01/13] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 02/13] Bluetooth: hci_bcm: Validate IRQ before using it Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 03/13] Bluetooth: hci_bcm: Clean up unnecessary #ifdef Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 04/13] Bluetooth: hci_bcm: Fix race on close Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 07/13] Bluetooth: hci_bcm: Document struct bcm_device Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 12/13] Bluetooth: hci_bcm: Sleep instead of spinning Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 11/13] Bluetooth: hci_bcm: Silence IRQ printk Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 08/13] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 13/13] Bluetooth: btbcm: Fix sleep mode struct ordering Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 05/13] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable() Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 06/13] Bluetooth: hci_bcm: Invalidate IRQ on request failure Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 09/13] Bluetooth: hci_bcm: Handle errors properly Lukas Wunner
2018-01-10 15:32 ` [PATCH v4 10/13] Bluetooth: hci_bcm: Support Apple GPIO handling Lukas Wunner
2018-01-10 18:02 ` [PATCH v4 00/13] Bluetooth on 2015+ MacBook (Pro) Marcel Holtmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.