Hello Marcel,
On 09/09/2015 18:29, Marcel Holtmann wrote:
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.
The HCI_LDISC spinlock hu->rx_lock seems to be only used in
hci_uart_tty_receive().
It seems to me that we may be able to remove it, but I do not
sufficiently understand TTY subsystem to be sure this will not introduce
any regression.
Signed-off-by: Frederic Danis <frederic.danis@xxxxxxxxxxxxxxx>
---
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;
I will add this
+ 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.
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.
Regards
Fred
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
