[PATCH v3 0/3] Bluetooth: hci_bcm: Add wake-up and PM runtime support

[PATCH v3 0/3] Bluetooth: hci_bcm: Add wake-up and PM runtime support

[Frederic Danis]

Add wake-up capabilities by retrieveing interruption used by BCM device in ACPI
table.
Add PM runtime support.

v2->v3:
- Use DMI_EXACT_MATCH instead of DMI_MATCH
- Fix IRQ polarity for T100TA in driver_data of dmi_system_id struct
- Use dmi_first_match() instead of dmi_check_system()

v1->v2:
- Split 1st patch between general wake-up capability and T100TA IRQ fix
- Replace multiple "if ... else if" by switch in bcm_resource()
- Move code to limit number of #ifdef
- Use DMI info to restrict IRQ to T100TA
- Split 2nd patch to prepare PM runtime support in separated patch
- Tested with and without CONFIG_PM_SLEEP and CONFIG_PM.

Frederic Danis (3):
  Bluetooth: hci_bcm: Fix IRQ polarity for T100
  Bluetooth: hci_bcm: Prepare PM runtime support
  Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

 drivers/bluetooth/hci_bcm.c | 203 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 171 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Frederic Danis]

ACPI table for BCM2E39 of T100TA is not correct.
Set correct irq_polarity for this device.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index f306541..6551251 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -32,6 +32,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/tty.h>
 #include <linux/interrupt.h>
+#include <linux/dmi.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -513,6 +514,22 @@ static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
 };
 
 #ifdef CONFIG_ACPI
+static u8 acpi_active_low = ACPI_ACTIVE_LOW;
+
+/* IRQ polarity of some chipsets are not defined correctly in ACPI table. */
+static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
+	{
+		.ident = "Asus T100TA",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
+					"ASUSTeK COMPUTER INC."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100TA"),
+		},
+		.driver_data = &acpi_active_low,
+	},
+	{ }
+};
+
 static int bcm_resource(struct acpi_resource *ares, void *data)
 {
 	struct bcm_device *dev = data;
@@ -552,6 +569,7 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	const struct acpi_device_id *id;
 	struct acpi_device *adev;
 	LIST_HEAD(resources);
+	const struct dmi_system_id *dmi_id;
 	int ret;
 
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
@@ -608,6 +626,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 
 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
 
+	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
+	if (dmi_id) {
+		bt_dev_dbg(dev, "Fix irq polarity");
+		dev->irq_polarity = *(u8*)dmi_id->driver_data;
+	}
+
 	return 0;
 }
 #else
-- 
1.9.1

Re: [PATCH v3 1/3] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Marcel Holtmann]

Hi Fred,

> ACPI table for BCM2E39 of T100TA is not correct.
> Set correct irq_polarity for this device.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index f306541..6551251 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -32,6 +32,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/tty.h>
> #include <linux/interrupt.h>
> +#include <linux/dmi.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -513,6 +514,22 @@ static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
> };
> 
> #ifdef CONFIG_ACPI
> +static u8 acpi_active_low = ACPI_ACTIVE_LOW;
> +
> +/* IRQ polarity of some chipsets are not defined correctly in ACPI table. */
> +static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
> +	{
> +		.ident = "Asus T100TA",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
> +					"ASUSTeK COMPUTER INC."),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100TA"),
> +		},
> +		.driver_data = &acpi_active_low,
> +	},
> +	{ }
> +};
> +
> static int bcm_resource(struct acpi_resource *ares, void *data)
> {
> 	struct bcm_device *dev = data;
> @@ -552,6 +569,7 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> 	const struct acpi_device_id *id;
> 	struct acpi_device *adev;
> 	LIST_HEAD(resources);
> +	const struct dmi_system_id *dmi_id;
> 	int ret;
> 
> 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
> @@ -608,6 +626,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> 
> 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
> 
> +	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
> +	if (dmi_id) {
> +		bt_dev_dbg(dev, "Fix irq polarity");

I would actually make this bt_dev_warn, but we do not have that helper at the moment. Might be worth while adding it. These obvious bugs in firmware need to be pointed out and not quietly swallowed.

Also lets be a bit more verbose with these things

		bt_dev_warn(dev, "%s: Overwriting IRQ polarity to active low", dmi->ident)

> +		dev->irq_polarity = *(u8*)dmi_id->driver_data;
> +	}
> +
> 	return 0;

Regards

Marcel

Re: [PATCH v3 1/3] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Frederic Danis]

Hello Marcel,

On 09/09/2015 18:13, Marcel Holtmann wrote:
> Hi Fred,
>
>> ACPI table for BCM2E39 of T100TA is not correct.
>> Set correct irq_polarity for this device.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index f306541..6551251 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -32,6 +32,7 @@
>> #include <linux/gpio/consumer.h>
>> #include <linux/tty.h>
>> #include <linux/interrupt.h>
>> +#include <linux/dmi.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -513,6 +514,22 @@ static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
>> };
>>
>> #ifdef CONFIG_ACPI
>> +static u8 acpi_active_low = ACPI_ACTIVE_LOW;
>> +
>> +/* IRQ polarity of some chipsets are not defined correctly in ACPI table. */
>> +static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
>> +	{
>> +		.ident = "Asus T100TA",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR,
>> +					"ASUSTeK COMPUTER INC."),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100TA"),
>> +		},
>> +		.driver_data = &acpi_active_low,
>> +	},
>> +	{ }
>> +};
>> +
>> static int bcm_resource(struct acpi_resource *ares, void *data)
>> {
>> 	struct bcm_device *dev = data;
>> @@ -552,6 +569,7 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>> 	const struct acpi_device_id *id;
>> 	struct acpi_device *adev;
>> 	LIST_HEAD(resources);
>> +	const struct dmi_system_id *dmi_id;
>> 	int ret;
>>
>> 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
>> @@ -608,6 +626,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>
>> 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
>>
>> +	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
>> +	if (dmi_id) {
>> +		bt_dev_dbg(dev, "Fix irq polarity");
>
> I would actually make this bt_dev_warn, but we do not have that helper at the moment. Might be worth while adding it. These obvious bugs in firmware need to be pointed out and not quietly swallowed.
>
> Also lets be a bit more verbose with these things
>
> 		bt_dev_warn(dev, "%s: Overwriting IRQ polarity to active low", dmi->ident)

OK, I will do this change and add BT_WARN and bt_dev_warn logging macros.

Regards

Fred

[PATCH v3 2/3] Bluetooth: hci_bcm: Prepare PM runtime support

[Frederic Danis]

Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
will be used during PM runtime callbacks.

Add __bcm_suspend() and __bcm_resume() which performs link management for
PM callbacks.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 6551251..29ed069 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -56,7 +56,7 @@ struct bcm_device {
 	int			irq;
 	u8			irq_polarity;
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 	struct hci_uart		*hu;
 	bool			is_suspended; /* suspend/resume flag */
 #endif
@@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 static irqreturn_t bcm_host_wake(int irq, void *data)
 {
 	struct bcm_device *bdev = data;
@@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
 			bcm->dev = dev;
 			hu->init_speed = dev->init_speed;
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 			dev->hu = hu;
 #endif
 			bcm_gpio_set_power(bcm->dev, true);
@@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 	if (bcm_device_exists(bdev)) {
 		bcm_gpio_set_power(bdev, false);
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 		if (device_can_wakeup(&bdev->pdev->dev)) {
 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
 			device_init_wakeup(&bdev->pdev->dev, false);
@@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&bcm->txq);
 }
 
+#ifdef CONFIG_PM
+static void __bcm_suspend(struct bcm_device *bdev)
+{
+	if (!bdev->is_suspended && bdev->hu) {
+		hci_uart_set_flow_control(bdev->hu, true);
+
+		/* Once this returns, driver suspends BT via GPIO */
+		bdev->is_suspended = true;
+	}
+
+	/* Suspend the device */
+	if (bdev->device_wakeup) {
+		gpiod_set_value(bdev->device_wakeup, false);
+		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+		mdelay(15);
+	}
+}
+
+static void __bcm_resume(struct bcm_device *bdev)
+{
+	if (bdev->device_wakeup) {
+		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) {
+		bdev->is_suspended = false;
+
+		hci_uart_set_flow_control(bdev->hu, false);
+	}
+}
+#endif
+
 #ifdef CONFIG_PM_SLEEP
 /* Platform suspend callback */
 static int bcm_suspend(struct device *dev)
@@ -439,19 +474,7 @@ static int bcm_suspend(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (!bdev->is_suspended) {
-		hci_uart_set_flow_control(bdev->hu, true);
-
-		/* Once this callback returns, driver suspends BT via GPIO */
-		bdev->is_suspended = true;
-	}
-
-	/* Suspend the device */
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, false);
-		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-		mdelay(15);
-	}
+	__bcm_suspend(bdev);
 
 	if (device_may_wakeup(&bdev->pdev->dev)) {
 		error = enable_irq_wake(bdev->irq);
@@ -482,18 +505,7 @@ static int bcm_resume(struct device *dev)
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
 
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, true);
-		bt_dev_dbg(bdev, "resume, delaying 15 ms");
-		mdelay(15);
-	}
-
-	/* When this callback executes, the device has woken up already */
-	if (bdev->is_suspended) {
-		bdev->is_suspended = false;
-
-		hci_uart_set_flow_control(bdev->hu, false);
-	}
+	__bcm_resume(bdev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
-- 
1.9.1

Re: [PATCH v3 2/3] Bluetooth: hci_bcm: Prepare PM runtime support

[Marcel Holtmann]

Hi Fred,

> Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
> will be used during PM runtime callbacks.
> 
> Add __bcm_suspend() and __bcm_resume() which performs link management for
> PM callbacks.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
> 1 file changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 6551251..29ed069 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -56,7 +56,7 @@ struct bcm_device {
> 	int			irq;
> 	u8			irq_polarity;
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 	struct hci_uart		*hu;
> 	bool			is_suspended; /* suspend/resume flag */
> #endif
> @@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
> 	return 0;
> }
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> static irqreturn_t bcm_host_wake(int irq, void *data)
> {
> 	struct bcm_device *bdev = data;
> @@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
> 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
> 			bcm->dev = dev;
> 			hu->init_speed = dev->init_speed;
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 			dev->hu = hu;
> #endif
> 			bcm_gpio_set_power(bcm->dev, true);
> @@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
> 	mutex_lock(&bcm_device_lock);
> 	if (bcm_device_exists(bdev)) {
> 		bcm_gpio_set_power(bdev, false);
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 		if (device_can_wakeup(&bdev->pdev->dev)) {
> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
> 			device_init_wakeup(&bdev->pdev->dev, false);
> @@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> 	return skb_dequeue(&bcm->txq);
> }
> 
> +#ifdef CONFIG_PM
> +static void __bcm_suspend(struct bcm_device *bdev)
> +{
> +	if (!bdev->is_suspended && bdev->hu) {
> +		hci_uart_set_flow_control(bdev->hu, true);
> +
> +		/* Once this returns, driver suspends BT via GPIO */
> +		bdev->is_suspended = true;
> +	}
> +
> +	/* Suspend the device */
> +	if (bdev->device_wakeup) {
> +		gpiod_set_value(bdev->device_wakeup, false);
> +		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> +		mdelay(15);
> +	}
> +}
> +
> +static void __bcm_resume(struct bcm_device *bdev)
> +{
> +	if (bdev->device_wakeup) {
> +		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) {
> +		bdev->is_suspended = false;
> +
> +		hci_uart_set_flow_control(bdev->hu, false);
> +	}
> +}

I wonder if naming these bcm_suspend_device and bcm_resume_device are not better names actually. Since even the comments in these section say that is what you are doing.

And as a general note, I think for devices with complicated power management, it is a good idea to add more comments in various place to explain why things are done a certain way. Nobody is going to remember this in 6 month when we have to look at this driver again for adding support for another model.

> +#endif
> +
> #ifdef CONFIG_PM_SLEEP
> /* Platform suspend callback */
> static int bcm_suspend(struct device *dev)
> @@ -439,19 +474,7 @@ static int bcm_suspend(struct device *dev)
> 	if (!bdev->hu)
> 		goto unlock;
> 
> -	if (!bdev->is_suspended) {
> -		hci_uart_set_flow_control(bdev->hu, true);
> -
> -		/* Once this callback returns, driver suspends BT via GPIO */
> -		bdev->is_suspended = true;
> -	}
> -
> -	/* Suspend the device */
> -	if (bdev->device_wakeup) {
> -		gpiod_set_value(bdev->device_wakeup, false);
> -		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> -		mdelay(15);
> -	}
> +	__bcm_suspend(bdev);
> 
> 	if (device_may_wakeup(&bdev->pdev->dev)) {
> 		error = enable_irq_wake(bdev->irq);
> @@ -482,18 +505,7 @@ static int bcm_resume(struct device *dev)
> 		bt_dev_dbg(bdev, "BCM irq: disabled");
> 	}
> 
> -	if (bdev->device_wakeup) {
> -		gpiod_set_value(bdev->device_wakeup, true);
> -		bt_dev_dbg(bdev, "resume, delaying 15 ms");
> -		mdelay(15);
> -	}
> -
> -	/* When this callback executes, the device has woken up already */
> -	if (bdev->is_suspended) {
> -		bdev->is_suspended = false;
> -
> -		hci_uart_set_flow_control(bdev->hu, false);
> -	}
> +	__bcm_resume(bdev);
> 

Regards

Marcel

Re: [PATCH v3 2/3] Bluetooth: hci_bcm: Prepare PM runtime support

[Frederic Danis]

Hello Marcel,

On 09/09/2015 18:18, Marcel Holtmann wrote:
> Hi Fred,
>
>> Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
>> will be used during PM runtime callbacks.
>>
>> Add __bcm_suspend() and __bcm_resume() which performs link management for
>> PM callbacks.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
>> 1 file changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 6551251..29ed069 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -56,7 +56,7 @@ struct bcm_device {
>> 	int			irq;
>> 	u8			irq_polarity;
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 	struct hci_uart		*hu;
>> 	bool			is_suspended; /* suspend/resume flag */
>> #endif
>> @@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
>> 	return 0;
>> }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> static irqreturn_t bcm_host_wake(int irq, void *data)
>> {
>> 	struct bcm_device *bdev = data;
>> @@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
>> 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
>> 			bcm->dev = dev;
>> 			hu->init_speed = dev->init_speed;
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 			dev->hu = hu;
>> #endif
>> 			bcm_gpio_set_power(bcm->dev, true);
>> @@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
>> 	mutex_lock(&bcm_device_lock);
>> 	if (bcm_device_exists(bdev)) {
>> 		bcm_gpio_set_power(bdev, false);
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 		if (device_can_wakeup(&bdev->pdev->dev)) {
>> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
>> 			device_init_wakeup(&bdev->pdev->dev, false);
>> @@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>>
>> +#ifdef CONFIG_PM
>> +static void __bcm_suspend(struct bcm_device *bdev)
>> +{
>> +	if (!bdev->is_suspended && bdev->hu) {
>> +		hci_uart_set_flow_control(bdev->hu, true);
>> +
>> +		/* Once this returns, driver suspends BT via GPIO */
>> +		bdev->is_suspended = true;
>> +	}
>> +
>> +	/* Suspend the device */
>> +	if (bdev->device_wakeup) {
>> +		gpiod_set_value(bdev->device_wakeup, false);
>> +		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
>> +		mdelay(15);
>> +	}
>> +}
>> +
>> +static void __bcm_resume(struct bcm_device *bdev)
>> +{
>> +	if (bdev->device_wakeup) {
>> +		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) {
>> +		bdev->is_suspended = false;
>> +
>> +		hci_uart_set_flow_control(bdev->hu, false);
>> +	}
>> +}
>
> I wonder if naming these bcm_suspend_device and bcm_resume_device are not better names actually. Since even the comments in these section say that is what you are doing.
>
> And as a general note, I think for devices with complicated power management, it is a good idea to add more comments in various place to explain why things are done a certain way. Nobody is going to remember this in 6 month when we have to look at this driver again for adding support for another model.

I will do this

Regards

Fred

[PATCH v3 3/3] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Frederic Danis]

Adds autosuspend runtime functionality to BCM UART driver.
Autosuspend is enabled at end of bcm_setup.

Add a work queue to be able to perform pm_runtime commands during bcm_recv
which is called by hci_uart_tty_receive() under spinlock.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 29ed069..6aad699 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -33,6 +33,7 @@
 #include <linux/tty.h>
 #include <linux/interrupt.h>
 #include <linux/dmi.h>
+#include <linux/pm_runtime.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -40,6 +41,8 @@
 #include "btbcm.h"
 #include "hci_uart.h"
 
+#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
+
 struct bcm_device {
 	struct list_head	list;
 
@@ -67,6 +70,9 @@ struct bcm_data {
 	struct sk_buff_head	txq;
 
 	struct bcm_device	*dev;
+#ifdef CONFIG_PM
+	struct work_struct	pm_work;
+#endif
 };
 
 /* List of BCM BT UART devices */
@@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
 
 	bt_dev_dbg(bdev, "Host wake IRQ");
 
+	pm_runtime_get(&bdev->pdev->dev);
+	pm_runtime_mark_last_busy(&bdev->pdev->dev);
+	pm_runtime_put_autosuspend(&bdev->pdev->dev);
+
 	return IRQ_HANDLED;
 }
 
@@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
 			goto unlock;
 
 		device_init_wakeup(&bdev->pdev->dev, true);
+
+		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
+						 BCM_AUTOSUSPEND_DELAY);
+		pm_runtime_use_autosuspend(&bdev->pdev->dev);
+		pm_runtime_set_active(&bdev->pdev->dev);
+		pm_runtime_enable(&bdev->pdev->dev);
 	}
 
 unlock:
@@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
 	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
 	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
 	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
-	.combine_modes = 0,	/* Combine sleep and LPM flag */
+	.combine_modes = 1,	/* Combine sleep and LPM flag */
 	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
 	/* Irrelevant USB flags */
 	.usb_auto_sleep = 0,
@@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
 
 	return 0;
 }
+
+static void bcm_pm_runtime_work(struct work_struct *work)
+{
+	struct bcm_data *bcm = container_of(work, struct bcm_data, pm_work);
+
+	mutex_lock(&bcm_device_lock);
+	if (bcm_device_exists(bcm->dev)) {
+		pm_runtime_get(&bcm->dev->pdev->dev);
+		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
+		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
+	}
+	mutex_unlock(&bcm_device_lock);
+}
+
+static bool bcm_schedule_work(struct bcm_data *bcm)
+{
+	return schedule_work(&bcm->pm_work);
+}
 #else
 static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
 static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
+static inline bool bcm_schedule_work(struct bcm_data *bcm) { return false; }
 #endif
 
 static int bcm_open(struct hci_uart *hu)
@@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
 			hu->init_speed = dev->init_speed;
 #ifdef CONFIG_PM
 			dev->hu = hu;
+			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
 #endif
 			bcm_gpio_set_power(bcm->dev, true);
 			break;
@@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
 	if (bcm_device_exists(bdev)) {
 		bcm_gpio_set_power(bdev, false);
 #ifdef CONFIG_PM
+		cancel_work_sync(&bcm->pm_work);
+
+		pm_runtime_disable(&bdev->pdev->dev);
+		pm_runtime_set_suspended(&bdev->pdev->dev);
+
 		if (device_can_wakeup(&bdev->pdev->dev)) {
 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
 			device_init_wakeup(&bdev->pdev->dev, false);
@@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
 		return -EUNATCH;
 
+	/* mutex_lock/unlock can not be used here as this function is called
+	 * from hci_uart_tty_receive under spinlock.
+	 * Defer pm_runtime_* calls to work queue
+	 */
+	if (bcm->dev)
+		bcm_schedule_work(bcm);
+
 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
 	if (IS_ERR(bcm->rx_skb)) {
@@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
+	struct sk_buff *skb = NULL;
+	struct bcm_device *bdev = NULL;
+
+	mutex_lock(&bcm_device_lock);
+
+	if (bcm_device_exists(bcm->dev)) {
+		bdev = bcm->dev;
+		pm_runtime_get_sync(&bdev->pdev->dev);
+		/* Shall be resumed here */
+	}
+
+	skb = skb_dequeue(&bcm->txq);
+
+	if (bdev) {
+		pm_runtime_mark_last_busy(&bdev->pdev->dev);
+		pm_runtime_put_autosuspend(&bdev->pdev->dev);
+	}
 
-	return skb_dequeue(&bcm->txq);
+	mutex_unlock(&bcm_device_lock);
+
+	return skb;
 }
 
 #ifdef CONFIG_PM
@@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
 		hci_uart_set_flow_control(bdev->hu, false);
 	}
 }
+
+static int bcm_runtime_suspend(struct device *dev)
+{
+	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+
+	bt_dev_dbg(bdev, "");
+
+	/* bcm_device_lock held is not required here as bcm_runtime_suspend
+	 * is only called when device is attached.
+	 */
+	__bcm_suspend(bdev);
+
+	return 0;
+}
+
+static int bcm_runtime_resume(struct device *dev)
+{
+	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+
+	bt_dev_dbg(bdev, "");
+
+	/* bcm_device_lock held is not required here as bcm_runtime_resume
+	 * is only called when device is attached.
+	 */
+	__bcm_resume(bdev);
+
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_PM_SLEEP
@@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	__bcm_suspend(bdev);
+	if (pm_runtime_active(dev))
+		__bcm_suspend(bdev);
 
 	if (device_may_wakeup(&bdev->pdev->dev)) {
 		error = enable_irq_wake(bdev->irq);
@@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 }
 #endif
@@ -729,7 +829,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
 #endif
 
 /* Platform suspend and resume callbacks */
-static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
+static const struct dev_pm_ops bcm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
+	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
+};
 
 static struct platform_driver bcm_driver = {
 	.probe = bcm_probe,
-- 
1.9.1

Re: [PATCH v3 3/3] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Marcel Holtmann]

Hi Fred,

> Adds autosuspend runtime functionality to BCM UART driver.
> Autosuspend is enabled at end of bcm_setup.
> 
> Add a work queue to be able to perform pm_runtime commands during bcm_recv
> which is called by hci_uart_tty_receive() under spinlock.

is that actually a requirement coming from the TTY subsystem? Or is that something that we inherited from when Bluetooth subsystem was using tasklets and we never got around fixing it. If the TTY subsystem does not require using a spinlock, we should not either.

> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 29ed069..6aad699 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -33,6 +33,7 @@
> #include <linux/tty.h>
> #include <linux/interrupt.h>
> #include <linux/dmi.h>
> +#include <linux/pm_runtime.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -40,6 +41,8 @@
> #include "btbcm.h"
> #include "hci_uart.h"
> 
> +#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
> +
> struct bcm_device {
> 	struct list_head	list;
> 
> @@ -67,6 +70,9 @@ struct bcm_data {
> 	struct sk_buff_head	txq;
> 
> 	struct bcm_device	*dev;
> +#ifdef CONFIG_PM
> +	struct work_struct	pm_work;
> +#endif
> };
> 
> /* List of BCM BT UART devices */
> @@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
> 
> 	bt_dev_dbg(bdev, "Host wake IRQ");
> 
> +	pm_runtime_get(&bdev->pdev->dev);
> +	pm_runtime_mark_last_busy(&bdev->pdev->dev);
> +	pm_runtime_put_autosuspend(&bdev->pdev->dev);
> +
> 	return IRQ_HANDLED;
> }
> 
> @@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
> 			goto unlock;
> 
> 		device_init_wakeup(&bdev->pdev->dev, true);
> +
> +		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
> +						 BCM_AUTOSUSPEND_DELAY);
> +		pm_runtime_use_autosuspend(&bdev->pdev->dev);
> +		pm_runtime_set_active(&bdev->pdev->dev);
> +		pm_runtime_enable(&bdev->pdev->dev);
> 	}
> 
> unlock:
> @@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
> 	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
> 	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
> 	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
> -	.combine_modes = 0,	/* Combine sleep and LPM flag */
> +	.combine_modes = 1,	/* Combine sleep and LPM flag */
> 	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
> 	/* Irrelevant USB flags */
> 	.usb_auto_sleep = 0,
> @@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
> 
> 	return 0;
> }
> +
> +static void bcm_pm_runtime_work(struct work_struct *work)
> +{
> +	struct bcm_data *bcm = container_of(work, struct bcm_data, pm_work);
> +
> +	mutex_lock(&bcm_device_lock);
> +	if (bcm_device_exists(bcm->dev)) {
> +		pm_runtime_get(&bcm->dev->pdev->dev);
> +		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
> +		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
> +	}
> +	mutex_unlock(&bcm_device_lock);
> +}
> +
> +static bool bcm_schedule_work(struct bcm_data *bcm)
> +{

Put the bcm valid check here.

	if (!bcm)
		return false;

> +	return schedule_work(&bcm->pm_work);
> +}
> #else
> static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
> static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
> +static inline bool bcm_schedule_work(struct bcm_data *bcm) { return false; }
> #endif
> 
> static int bcm_open(struct hci_uart *hu)
> @@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
> 			hu->init_speed = dev->init_speed;
> #ifdef CONFIG_PM
> 			dev->hu = hu;
> +			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
> #endif
> 			bcm_gpio_set_power(bcm->dev, true);
> 			break;
> @@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
> 	if (bcm_device_exists(bdev)) {
> 		bcm_gpio_set_power(bdev, false);
> #ifdef CONFIG_PM
> +		cancel_work_sync(&bcm->pm_work);
> +
> +		pm_runtime_disable(&bdev->pdev->dev);
> +		pm_runtime_set_suspended(&bdev->pdev->dev);
> +
> 		if (device_can_wakeup(&bdev->pdev->dev)) {
> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
> 			device_init_wakeup(&bdev->pdev->dev, false);
> @@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> 		return -EUNATCH;
> 
> +	/* mutex_lock/unlock can not be used here as this function is called
> +	 * from hci_uart_tty_receive under spinlock.
> +	 * Defer pm_runtime_* calls to work queue
> +	 */
> +	if (bcm->dev)
> +		bcm_schedule_work(bcm);
> +

We are doing this for every single H:4 fragment we receive now. This seems to be a really bad idea since in theory the packets can arrive one byte at a time.

So this is a lot of overhead to schedule the work queue every single fragment and just hope for the best. I think we need to be a lot smarter here. Otherwise we get a nasty performance hit on smaller devices. The actual hu->recv callback is really only designed for basic reassembly of the packets. It really should stay simple.

For hci_intel we are doing this in the enqueue function when the Bluetooth subsystem has to send us data. Why would we do this on the TTY receiving side here? If the device is not active, we would not be receiving anything in the first place. I am failing to see the logic here.

> 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
> 	if (IS_ERR(bcm->rx_skb)) {
> @@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm = hu->priv;
> +	struct sk_buff *skb = NULL;
> +	struct bcm_device *bdev = NULL;
> +
> +	mutex_lock(&bcm_device_lock);
> +
> +	if (bcm_device_exists(bcm->dev)) {
> +		bdev = bcm->dev;
> +		pm_runtime_get_sync(&bdev->pdev->dev);
> +		/* Shall be resumed here */
> +	}
> +
> +	skb = skb_dequeue(&bcm->txq);
> +
> +	if (bdev) {
> +		pm_runtime_mark_last_busy(&bdev->pdev->dev);
> +		pm_runtime_put_autosuspend(&bdev->pdev->dev);
> +	}
> 
> -	return skb_dequeue(&bcm->txq);
> +	mutex_unlock(&bcm_device_lock);
> +
> +	return skb;
> }
> 
> #ifdef CONFIG_PM
> @@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
> 		hci_uart_set_flow_control(bdev->hu, false);
> 	}
> }
> +
> +static int bcm_runtime_suspend(struct device *dev)
> +{
> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
> +
> +	bt_dev_dbg(bdev, "");
> +
> +	/* bcm_device_lock held is not required here as bcm_runtime_suspend
> +	 * is only called when device is attached.
> +	 */
> +	__bcm_suspend(bdev);
> +
> +	return 0;
> +}
> +
> +static int bcm_runtime_resume(struct device *dev)
> +{
> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
> +
> +	bt_dev_dbg(bdev, "");
> +
> +	/* bcm_device_lock held is not required here as bcm_runtime_resume
> +	 * is only called when device is attached.
> +	 */
> +	__bcm_resume(bdev);
> +
> +	return 0;
> +}
> #endif
> 
> #ifdef CONFIG_PM_SLEEP
> @@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
> 	if (!bdev->hu)
> 		goto unlock;
> 
> -	__bcm_suspend(bdev);
> +	if (pm_runtime_active(dev))
> +		__bcm_suspend(bdev);
> 
> 	if (device_may_wakeup(&bdev->pdev->dev)) {
> 		error = enable_irq_wake(bdev->irq);
> @@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
> unlock:
> 	mutex_unlock(&bcm_device_lock);
> 
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> 	return 0;
> }
> #endif
> @@ -729,7 +829,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
> #endif
> 
> /* Platform suspend and resume callbacks */
> -static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
> +static const struct dev_pm_ops bcm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
> +	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
> +};
> 
> static struct platform_driver bcm_driver = {
> 	.probe = bcm_probe,

Regards

Marcel

Re: [PATCH v3 3/3] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Frederic Danis]

SGVsbG8gTWFyY2VsLAoKT24gMDkvMDkvMjAxNSAxODoyOSwgTWFyY2VsIEhvbHRtYW5uIHdyb3Rl
Ogo+IEhpIEZyZWQsCj4KPj4gQWRkcyBhdXRvc3VzcGVuZCBydW50aW1lIGZ1bmN0aW9uYWxpdHkg
dG8gQkNNIFVBUlQgZHJpdmVyLgo+PiBBdXRvc3VzcGVuZCBpcyBlbmFibGVkIGF0IGVuZCBvZiBi
Y21fc2V0dXAuCj4+Cj4+IEFkZCBhIHdvcmsgcXVldWUgdG8gYmUgYWJsZSB0byBwZXJmb3JtIHBt
X3J1bnRpbWUgY29tbWFuZHMgZHVyaW5nIGJjbV9yZWN2Cj4+IHdoaWNoIGlzIGNhbGxlZCBieSBo
Y2lfdWFydF90dHlfcmVjZWl2ZSgpIHVuZGVyIHNwaW5sb2NrLgo+IGlzIHRoYXQgYWN0dWFsbHkg
YSByZXF1aXJlbWVudCBjb21pbmcgZnJvbSB0aGUgVFRZIHN1YnN5c3RlbT8gT3IgaXMgdGhhdCBz
b21ldGhpbmcgdGhhdCB3ZSBpbmhlcml0ZWQgZnJvbSB3aGVuIEJsdWV0b290aCBzdWJzeXN0ZW0g
d2FzIHVzaW5nIHRhc2tsZXRzIGFuZCB3ZSBuZXZlciBnb3QgYXJvdW5kIGZpeGluZyBpdC4gSWYg
dGhlIFRUWSBzdWJzeXN0ZW0gZG9lcyBub3QgcmVxdWlyZSB1c2luZyBhIHNwaW5sb2NrLCB3ZSBz
aG91bGQgbm90IGVpdGhlci4KClRoZSBIQ0lfTERJU0Mgc3BpbmxvY2sgaHUtPnJ4X2xvY2sgc2Vl
bXMgdG8gYmUgb25seSB1c2VkIGluIApoY2lfdWFydF90dHlfcmVjZWl2ZSgpLgpJdCBzZWVtcyB0
byBtZSB0aGF0IHdlIG1heSBiZSBhYmxlIHRvIHJlbW92ZSBpdCwgYnV0IEkgZG8gbm90IApzdWZm
aWNpZW50bHkgdW5kZXJzdGFuZCBUVFkgc3Vic3lzdGVtIHRvIGJlIHN1cmUgdGhpcyB3aWxsIG5v
dCBpbnRyb2R1Y2UgCmFueSByZWdyZXNzaW9uLgoKPj4gU2lnbmVkLW9mZi1ieTogRnJlZGVyaWMg
RGFuaXMgPGZyZWRlcmljLmRhbmlzQGxpbnV4LmludGVsLmNvbT4KPj4gLS0tCj4+IGRyaXZlcnMv
Ymx1ZXRvb3RoL2hjaV9iY20uYyB8IDExMSArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKystLQo+PiAxIGZpbGUgY2hhbmdlZCwgMTA3IGluc2VydGlvbnMoKyksIDQgZGVs
ZXRpb25zKC0pCj4+Cj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2JsdWV0b290aC9oY2lfYmNtLmMg
Yi9kcml2ZXJzL2JsdWV0b290aC9oY2lfYmNtLmMKPj4gaW5kZXggMjllZDA2OS4uNmFhZDY5OSAx
MDA2NDQKPj4gLS0tIGEvZHJpdmVycy9ibHVldG9vdGgvaGNpX2JjbS5jCj4+ICsrKyBiL2RyaXZl
cnMvYmx1ZXRvb3RoL2hjaV9iY20uYwo+PiBAQCAtMzMsNiArMzMsNyBAQAo+PiAjaW5jbHVkZSA8
bGludXgvdHR5Lmg+Cj4+ICNpbmNsdWRlIDxsaW51eC9pbnRlcnJ1cHQuaD4KPj4gI2luY2x1ZGUg
PGxpbnV4L2RtaS5oPgo+PiArI2luY2x1ZGUgPGxpbnV4L3BtX3J1bnRpbWUuaD4KPj4KPj4gI2lu
Y2x1ZGUgPG5ldC9ibHVldG9vdGgvYmx1ZXRvb3RoLmg+Cj4+ICNpbmNsdWRlIDxuZXQvYmx1ZXRv
b3RoL2hjaV9jb3JlLmg+Cj4+IEBAIC00MCw2ICs0MSw4IEBACj4+ICNpbmNsdWRlICJidGJjbS5o
Igo+PiAjaW5jbHVkZSAiaGNpX3VhcnQuaCIKPj4KPj4gKyNkZWZpbmUgQkNNX0FVVE9TVVNQRU5E
X0RFTEFZCTUwMDAgLyogZGVmYXVsdCBhdXRvc2xlZXAgZGVsYXkgKi8KPj4gKwo+PiBzdHJ1Y3Qg
YmNtX2RldmljZSB7Cj4+IAlzdHJ1Y3QgbGlzdF9oZWFkCWxpc3Q7Cj4+Cj4+IEBAIC02Nyw2ICs3
MCw5IEBAIHN0cnVjdCBiY21fZGF0YSB7Cj4+IAlzdHJ1Y3Qgc2tfYnVmZl9oZWFkCXR4cTsKPj4K
Pj4gCXN0cnVjdCBiY21fZGV2aWNlCSpkZXY7Cj4+ICsjaWZkZWYgQ09ORklHX1BNCj4+ICsJc3Ry
dWN0IHdvcmtfc3RydWN0CXBtX3dvcms7Cj4+ICsjZW5kaWYKPj4gfTsKPj4KPj4gLyogTGlzdCBv
ZiBCQ00gQlQgVUFSVCBkZXZpY2VzICovCj4+IEBAIC0xNjAsNiArMTY2LDEwIEBAIHN0YXRpYyBp
cnFyZXR1cm5fdCBiY21faG9zdF93YWtlKGludCBpcnEsIHZvaWQgKmRhdGEpCj4+Cj4+IAlidF9k
ZXZfZGJnKGJkZXYsICJIb3N0IHdha2UgSVJRIik7Cj4+Cj4+ICsJcG1fcnVudGltZV9nZXQoJmJk
ZXYtPnBkZXYtPmRldik7Cj4+ICsJcG1fcnVudGltZV9tYXJrX2xhc3RfYnVzeSgmYmRldi0+cGRl
di0+ZGV2KTsKPj4gKwlwbV9ydW50aW1lX3B1dF9hdXRvc3VzcGVuZCgmYmRldi0+cGRldi0+ZGV2
KTsKPj4gKwo+PiAJcmV0dXJuIElSUV9IQU5ETEVEOwo+PiB9Cj4+Cj4+IEBAIC0xODMsNiArMTkz
LDEyIEBAIHN0YXRpYyBpbnQgYmNtX3JlcXVlc3RfaXJxKHN0cnVjdCBiY21fZGF0YSAqYmNtKQo+
PiAJCQlnb3RvIHVubG9jazsKPj4KPj4gCQlkZXZpY2VfaW5pdF93YWtldXAoJmJkZXYtPnBkZXYt
PmRldiwgdHJ1ZSk7Cj4+ICsKPj4gKwkJcG1fcnVudGltZV9zZXRfYXV0b3N1c3BlbmRfZGVsYXko
JmJkZXYtPnBkZXYtPmRldiwKPj4gKwkJCQkJCSBCQ01fQVVUT1NVU1BFTkRfREVMQVkpOwo+PiAr
CQlwbV9ydW50aW1lX3VzZV9hdXRvc3VzcGVuZCgmYmRldi0+cGRldi0+ZGV2KTsKPj4gKwkJcG1f
cnVudGltZV9zZXRfYWN0aXZlKCZiZGV2LT5wZGV2LT5kZXYpOwo+PiArCQlwbV9ydW50aW1lX2Vu
YWJsZSgmYmRldi0+cGRldi0+ZGV2KTsKPj4gCX0KPj4KPj4gdW5sb2NrOgo+PiBAQCAtMTk4LDcg
KzIxNCw3IEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgYmNtX3NldF9zbGVlcF9tb2RlIGRlZmF1bHRf
c2xlZXBfcGFyYW1zID0gewo+PiAJLmJ0X3dha2VfYWN0aXZlID0gMSwJLyogQlRfV0FLRSBhY3Rp
dmUgbW9kZTogMSA9IGhpZ2gsIDAgPSBsb3cgKi8KPj4gCS5ob3N0X3dha2VfYWN0aXZlID0gMCwJ
LyogSE9TVF9XQUtFIGFjdGl2ZSBtb2RlOiAxID0gaGlnaCwgMCA9IGxvdyAqLwo+PiAJLmFsbG93
X2hvc3Rfc2xlZXAgPSAxLAkvKiBBbGxvdyBob3N0IHNsZWVwIGluIFNDTyBmbGFnICovCj4+IC0J
LmNvbWJpbmVfbW9kZXMgPSAwLAkvKiBDb21iaW5lIHNsZWVwIGFuZCBMUE0gZmxhZyAqLwo+PiAr
CS5jb21iaW5lX21vZGVzID0gMSwJLyogQ29tYmluZSBzbGVlcCBhbmQgTFBNIGZsYWcgKi8KPj4g
CS50cmlzdGF0ZV9jb250cm9sID0gMCwJLyogQWxsb3cgdHJpLXN0YXRlIGNvbnRyb2wgb2YgVUFS
VCB0eCBmbGFnICovCj4+IAkvKiBJcnJlbGV2YW50IFVTQiBmbGFncyAqLwo+PiAJLnVzYl9hdXRv
X3NsZWVwID0gMCwKPj4gQEAgLTIyOCw5ICsyNDQsMjggQEAgc3RhdGljIGludCBiY21fc2V0dXBf
c2xlZXAoc3RydWN0IGhjaV91YXJ0ICpodSkKPj4KPj4gCXJldHVybiAwOwo+PiB9Cj4+ICsKPj4g
K3N0YXRpYyB2b2lkIGJjbV9wbV9ydW50aW1lX3dvcmsoc3RydWN0IHdvcmtfc3RydWN0ICp3b3Jr
KQo+PiArewo+PiArCXN0cnVjdCBiY21fZGF0YSAqYmNtID0gY29udGFpbmVyX29mKHdvcmssIHN0
cnVjdCBiY21fZGF0YSwgcG1fd29yayk7Cj4+ICsKPj4gKwltdXRleF9sb2NrKCZiY21fZGV2aWNl
X2xvY2spOwo+PiArCWlmIChiY21fZGV2aWNlX2V4aXN0cyhiY20tPmRldikpIHsKPj4gKwkJcG1f
cnVudGltZV9nZXQoJmJjbS0+ZGV2LT5wZGV2LT5kZXYpOwo+PiArCQlwbV9ydW50aW1lX21hcmtf
bGFzdF9idXN5KCZiY20tPmRldi0+cGRldi0+ZGV2KTsKPj4gKwkJcG1fcnVudGltZV9wdXRfYXV0
b3N1c3BlbmQoJmJjbS0+ZGV2LT5wZGV2LT5kZXYpOwo+PiArCX0KPj4gKwltdXRleF91bmxvY2so
JmJjbV9kZXZpY2VfbG9jayk7Cj4+ICt9Cj4+ICsKPj4gK3N0YXRpYyBib29sIGJjbV9zY2hlZHVs
ZV93b3JrKHN0cnVjdCBiY21fZGF0YSAqYmNtKQo+PiArewo+IFB1dCB0aGUgYmNtIHZhbGlkIGNo
ZWNrIGhlcmUuCj4KPiAJaWYgKCFiY20pCj4gCQlyZXR1cm4gZmFsc2U7CgpJIHdpbGwgYWRkIHRo
aXMKPj4gKwlyZXR1cm4gc2NoZWR1bGVfd29yaygmYmNtLT5wbV93b3JrKTsKPj4gK30KPj4gI2Vs
c2UKPj4gc3RhdGljIGlubGluZSBpbnQgYmNtX3JlcXVlc3RfaXJxKHN0cnVjdCBiY21fZGF0YSAq
YmNtKSB7IHJldHVybiAwOyB9Cj4+IHN0YXRpYyBpbmxpbmUgaW50IGJjbV9zZXR1cF9zbGVlcChz
dHJ1Y3QgaGNpX3VhcnQgKmh1KSB7IHJldHVybiAwOyB9Cj4+ICtzdGF0aWMgaW5saW5lIGJvb2wg
YmNtX3NjaGVkdWxlX3dvcmsoc3RydWN0IGJjbV9kYXRhICpiY20pIHsgcmV0dXJuIGZhbHNlOyB9
Cj4+ICNlbmRpZgo+Pgo+PiBzdGF0aWMgaW50IGJjbV9vcGVuKHN0cnVjdCBoY2lfdWFydCAqaHUp
Cj4+IEBAIC0yNjEsNiArMjk2LDcgQEAgc3RhdGljIGludCBiY21fb3BlbihzdHJ1Y3QgaGNpX3Vh
cnQgKmh1KQo+PiAJCQlodS0+aW5pdF9zcGVlZCA9IGRldi0+aW5pdF9zcGVlZDsKPj4gI2lmZGVm
IENPTkZJR19QTQo+PiAJCQlkZXYtPmh1ID0gaHU7Cj4+ICsJCQlJTklUX1dPUksoJmJjbS0+cG1f
d29yaywgYmNtX3BtX3J1bnRpbWVfd29yayk7Cj4+ICNlbmRpZgo+PiAJCQliY21fZ3Bpb19zZXRf
cG93ZXIoYmNtLT5kZXYsIHRydWUpOwo+PiAJCQlicmVhazsKPj4gQEAgLTI4NCw2ICszMjAsMTEg
QEAgc3RhdGljIGludCBiY21fY2xvc2Uoc3RydWN0IGhjaV91YXJ0ICpodSkKPj4gCWlmIChiY21f
ZGV2aWNlX2V4aXN0cyhiZGV2KSkgewo+PiAJCWJjbV9ncGlvX3NldF9wb3dlcihiZGV2LCBmYWxz
ZSk7Cj4+ICNpZmRlZiBDT05GSUdfUE0KPj4gKwkJY2FuY2VsX3dvcmtfc3luYygmYmNtLT5wbV93
b3JrKTsKPj4gKwo+PiArCQlwbV9ydW50aW1lX2Rpc2FibGUoJmJkZXYtPnBkZXYtPmRldik7Cj4+
ICsJCXBtX3J1bnRpbWVfc2V0X3N1c3BlbmRlZCgmYmRldi0+cGRldi0+ZGV2KTsKPj4gKwo+PiAJ
CWlmIChkZXZpY2VfY2FuX3dha2V1cCgmYmRldi0+cGRldi0+ZGV2KSkgewo+PiAJCQlkZXZtX2Zy
ZWVfaXJxKCZiZGV2LT5wZGV2LT5kZXYsIGJkZXYtPmlycSwgYmRldik7Cj4+IAkJCWRldmljZV9p
bml0X3dha2V1cCgmYmRldi0+cGRldi0+ZGV2LCBmYWxzZSk7Cj4+IEBAIC0zOTMsNiArNDM0LDEz
IEBAIHN0YXRpYyBpbnQgYmNtX3JlY3Yoc3RydWN0IGhjaV91YXJ0ICpodSwgY29uc3Qgdm9pZCAq
ZGF0YSwgaW50IGNvdW50KQo+PiAJaWYgKCF0ZXN0X2JpdChIQ0lfVUFSVF9SRUdJU1RFUkVELCAm
aHUtPmZsYWdzKSkKPj4gCQlyZXR1cm4gLUVVTkFUQ0g7Cj4+Cj4+ICsJLyogbXV0ZXhfbG9jay91
bmxvY2sgY2FuIG5vdCBiZSB1c2VkIGhlcmUgYXMgdGhpcyBmdW5jdGlvbiBpcyBjYWxsZWQKPj4g
KwkgKiBmcm9tIGhjaV91YXJ0X3R0eV9yZWNlaXZlIHVuZGVyIHNwaW5sb2NrLgo+PiArCSAqIERl
ZmVyIHBtX3J1bnRpbWVfKiBjYWxscyB0byB3b3JrIHF1ZXVlCj4+ICsJICovCj4+ICsJaWYgKGJj
bS0+ZGV2KQo+PiArCQliY21fc2NoZWR1bGVfd29yayhiY20pOwo+PiArCj4gV2UgYXJlIGRvaW5n
IHRoaXMgZm9yIGV2ZXJ5IHNpbmdsZSBIOjQgZnJhZ21lbnQgd2UgcmVjZWl2ZSBub3cuIFRoaXMg
c2VlbXMgdG8gYmUgYSByZWFsbHkgYmFkIGlkZWEgc2luY2UgaW4gdGhlb3J5IHRoZSBwYWNrZXRz
IGNhbiBhcnJpdmUgb25lIGJ5dGUgYXQgYSB0aW1lLgo+Cj4gU28gdGhpcyBpcyBhIGxvdCBvZiBv
dmVyaGVhZCB0byBzY2hlZHVsZSB0aGUgd29yayBxdWV1ZSBldmVyeSBzaW5nbGUgZnJhZ21lbnQg
YW5kIGp1c3QgaG9wZSBmb3IgdGhlIGJlc3QuIEkgdGhpbmsgd2UgbmVlZCB0byBiZSBhIGxvdCBz
bWFydGVyIGhlcmUuIE90aGVyd2lzZSB3ZSBnZXQgYSBuYXN0eSBwZXJmb3JtYW5jZSBoaXQgb24g
c21hbGxlciBkZXZpY2VzLiBUaGUgYWN0dWFsIGh1LT5yZWN2IGNhbGxiYWNrIGlzIHJlYWxseSBv
bmx5IGRlc2lnbmVkIGZvciBiYXNpYyByZWFzc2VtYmx5IG9mIHRoZSBwYWNrZXRzLiBJdCByZWFs
bHkgc2hvdWxkIHN0YXkgc2ltcGxlLgo+Cj4gRm9yIGhjaV9pbnRlbCB3ZSBhcmUgZG9pbmcgdGhp
cyBpbiB0aGUgZW5xdWV1ZSBmdW5jdGlvbiB3aGVuIHRoZSBCbHVldG9vdGggc3Vic3lzdGVtIGhh
cyB0byBzZW5kIHVzIGRhdGEuIFdoeSB3b3VsZCB3ZSBkbyB0aGlzIG9uIHRoZSBUVFkgcmVjZWl2
aW5nIHNpZGUgaGVyZT8gSWYgdGhlIGRldmljZSBpcyBub3QgYWN0aXZlLCB3ZSB3b3VsZCBub3Qg
YmUgcmVjZWl2aW5nIGFueXRoaW5nIGluIHRoZSBmaXJzdCBwbGFjZS4gSSBhbSBmYWlsaW5nIHRv
IHNlZSB0aGUgbG9naWMgaGVyZS4KaGNpX2ludGVsIGFsc28gcGVyZm9ybXMgdGhpcyB3aGVuIHJl
Y2VpdmluZyBMUE1fT1BfVFhfTk9USUZZIHBhY2tldHMuCkFmYWlrLCBCcm9hZGNvbSBkZXZpY2Ug
ZG9lcyBub3QgaGF2ZSB0aGlzIGZlYXR1cmUgYW5kIHdlIG5lZWQgdG8gZGVsYXkgCnRoZSBzdXNw
ZW5kIHdoZW4gd2UgcmVjZWl2ZSBhIHBhY2tldC4KCkkgd2lsbCBtb3ZlIGJjbV9zY2hlZHVsZV93
b3JrKCkgYWZ0ZXIgaDRfcmVjdl9idWYoKSB0byBwZXJmb3JtIGl0IG9ubHkgCm9uIGNvbXBsZXRl
ZCBwYWNrZXQuCgpSZWdhcmRzCgpGcmVkCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpJbnRlbCBDb3Jwb3JhdGlvbiBT
QVMgKEZyZW5jaCBzaW1wbGlmaWVkIGpvaW50IHN0b2NrIGNvbXBhbnkpClJlZ2lzdGVyZWQgaGVh
ZHF1YXJ0ZXJzOiAiTGVzIE1vbnRhbGV0cyItIDIsIHJ1ZSBkZSBQYXJpcywgCjkyMTk2IE1ldWRv
biBDZWRleCwgRnJhbmNlClJlZ2lzdHJhdGlvbiBOdW1iZXI6ICAzMDIgNDU2IDE5OSBSLkMuUy4g
TkFOVEVSUkUKQ2FwaXRhbDogNCw1NzIsMDAwIEV1cm9zCgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0
dGFjaG1lbnRzIG1heSBjb250YWluIGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUg
dXNlIG9mIHRoZSBpbnRlbmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0
aW9uCmJ5IG90aGVycyBpcyBzdHJpY3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUg
aW50ZW5kZWQKcmVjaXBpZW50LCBwbGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFuZCBkZWxldGUg
YWxsIGNvcGllcy4K

Re: [PATCH v3 3/3] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Loic Poulain]

Hi Fred

>> For hci_intel we are doing this in the enqueue function when the
>> Bluetooth subsystem has to send us data. Why would we do this on the
>> TTY receiving side here? If the device is not active, we would not be
>> receiving anything in the first place. I am failing to see the logic
>> here.
> hci_intel also performs this when receiving LPM_OP_TX_NOTIFY packets.
> Afaik, Broadcom device does not have this feature and we need to delay
> the suspend when we receive a packet.
>
> I will move bcm_schedule_work() after h4_recv_buf() to perform it only
> on completed packet.

What about using a delayed work with work_delay << autosuspend_delay so 
that the work is not queued each time but every work_delay in worst case.

hci_uart proto callback is called with rx spinlock in
hci_uart_tty_receive which is the receive_buf ldisc callback.
In drivers/tty/tty_buffer.c we can see that flush_to_ldisc (work) locks
a mutex (&buf->lock) and push the data to ldisc via receive_buf.

So, I don't see any problem to remove the hci_ldisc rx_spinlock and 
replace it by a mutex, or even don't use locking at all.

Regards,
Loic
-- 
Intel Open Source Technology Center
http://oss.intel.com/