[PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[Matthew Garrett]

Bluetooth runtime PM interacts badly with input devices - the connection
will be dropped if the device becomes idle, resulting in noticable lag when
the user interacts with the input device again. Bump the pm runtime count
when the device is associated and release it when it's disassociated in
order to avoid this.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 net/bluetooth/hidp/core.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index bfe641b..a4489a7 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #include <linux/file.h>
 #include <linux/init.h>
 #include <linux/wait.h>
+#include <linux/pm_runtime.h>
 #include <net/sock.h>
 
 #include <linux/input.h>
@@ -622,6 +623,14 @@ static int hidp_session(void *arg)
 	return 0;
 }
 
+static struct hci_dev *hidp_get_hci(struct hidp_session *session)
+{
+	bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
+	bdaddr_t *dst = &bt_sk(session->ctrl_sock->sk)->dst;
+
+	return hci_get_route(dst, src);
+}
+
 static struct device *hidp_get_device(struct hidp_session *session)
 {
 	bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
@@ -819,6 +828,7 @@ fault:
 int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
 {
 	struct hidp_session *session, *s;
+	struct hci_dev *hdev;
 	int err;
 
 	BT_DBG("");
@@ -889,6 +899,10 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
 		hidp_input_event(session->input, EV_LED, 0, 0);
 	}
 
+	hdev = hidp_get_hci(session);
+	pm_runtime_get(hdev->parent);
+	hci_dev_put(hdev);
+
 	up_write(&hidp_session_sem);
 	return 0;
 
@@ -925,6 +939,7 @@ failed:
 int hidp_del_connection(struct hidp_conndel_req *req)
 {
 	struct hidp_session *session;
+	struct hci_dev *hdev;
 	int err = 0;
 
 	BT_DBG("");
@@ -952,6 +967,9 @@ int hidp_del_connection(struct hidp_conndel_req *req)
 	} else
 		err = -ENOENT;
 
+	hdev = hidp_get_hci(session);
+	pm_runtime_put(hdev->parent);
+	hci_dev_put(hdev);
 	up_read(&hidp_session_sem);
 	return err;
 }
-- 
1.7.2.3

[PATCH 2/3] bluetooth: Remove some unnecessary error messages

[Matthew Garrett]

The main reason for these urbs to error out on submission is that runtime
pm has kicked in, which is unnecessary noise. Let's just drop them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/bluetooth/btusb.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d22ce3c..3ace025 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
 	usb_anchor_urb(urb, &data->intr_anchor);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
-	if (err < 0) {
-		BT_ERR("%s urb %p failed to resubmit (%d)",
-						hdev->name, urb, -err);
+	if (err < 0)
 		usb_unanchor_urb(urb);
-	}
 }
 
 static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
@@ -313,11 +310,8 @@ static void btusb_bulk_complete(struct urb *urb)
 	usb_mark_last_busy(data->udev);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
-	if (err < 0) {
-		BT_ERR("%s urb %p failed to resubmit (%d)",
-						hdev->name, urb, -err);
+	if (err < 0)
 		usb_unanchor_urb(urb);
-	}
 }
 
 static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags)
@@ -402,11 +396,8 @@ static void btusb_isoc_complete(struct urb *urb)
 	usb_anchor_urb(urb, &data->isoc_anchor);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
-	if (err < 0) {
-		BT_ERR("%s urb %p failed to resubmit (%d)",
-						hdev->name, urb, -err);
+	if (err < 0)
 		usb_unanchor_urb(urb);
-	}
 }
 
 static void inline __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
-- 
1.7.2.3

[PATCH 3/3] bluetooth: Enable USB autosuspend by default on btusb

[Matthew Garrett]

We've done this for a while in Fedora without any obvious problems other
than some interaction with input devices. Those should be fixed now, so
let's try this in mainline.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/bluetooth/btusb.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3ace025..03b64e4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1014,6 +1014,8 @@ static int btusb_probe(struct usb_interface *intf,
 
 	usb_set_intfdata(intf, data);
 
+	usb_enable_autosuspend(interface_to_usbdev(intf));
+
 	return 0;
 }
 
-- 
1.7.2.3

Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages

[Andrei Emeltchenko]

Hi Matthew,

On Thu, Sep 16, 2010 at 8:58 PM, Matthew Garrett <mjg@redhat.com> wrote:
> The main reason for these urbs to error out on submission is that runtime
> pm has kicked in, which is unnecessary noise. Let's just drop them.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
> =A0drivers/bluetooth/btusb.c | =A0 15 +++------------
> =A01 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d22ce3c..3ace025 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_anchor_urb(urb, &data->intr_anchor);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>

I do not get the point here. Cannot you just undef BT_ERR if you
enabled debug? I believe this is standard behavior.

IMO NACK

Regards,
Andrei

> =A0static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags=
)
> @@ -313,11 +310,8 @@ static void btusb_bulk_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_mark_last_busy(data->udev);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>
> =A0static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags=
)
> @@ -402,11 +396,8 @@ static void btusb_isoc_complete(struct urb *urb)
> =A0 =A0 =A0 =A0usb_anchor_urb(urb, &data->isoc_anchor);
>
> =A0 =A0 =A0 =A0err =3D usb_submit_urb(urb, GFP_ATOMIC);
> - =A0 =A0 =A0 if (err < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("%s urb %p failed to resubmit (%d)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 hdev->name, urb, -err);
> + =A0 =A0 =A0 if (err < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0usb_unanchor_urb(urb);
> - =A0 =A0 =A0 }
> =A0}
>
> =A0static void inline __fill_isoc_descriptor(struct urb *urb, int len, in=
t mtu)
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[Oliver Neukum]

Am Donnerstag, 16. September 2010, 19:58:13 schrieb Matthew Garrett:
> Bluetooth runtime PM interacts badly with input devices - the connection
> will be dropped if the device becomes idle, resulting in noticable lag when
> the user interacts with the input device again. Bump the pm runtime count
> when the device is associated and release it when it's disassociated in
> order to avoid this.

What is the effect on battery live of the external devices?

	Regards
		Oliver

Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages

[Oliver Neukum]

Am Freitag, 17. September 2010, 10:33:30 schrieb Andrei Emeltchenko:
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d22ce3c..3ace025 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -229,11 +229,8 @@ static void btusb_intr_complete(struct urb *urb)
> >        usb_anchor_urb(urb, &data->intr_anchor);
> >
> >        err = usb_submit_urb(urb, GFP_ATOMIC);
> > -       if (err < 0) {
> > -               BT_ERR("%s urb %p failed to resubmit (%d)",
> > -                                               hdev->name, urb, -err);
> > +       if (err < 0)
> >                usb_unanchor_urb(urb);
> > -       }
> >  }
> >
> 
> I do not get the point here. Cannot you just undef BT_ERR if you
> enabled debug? I believe this is standard behavior.

You can undefine BT_ERR, but then you'll miss error reports.
This message may indicate an error, but usually doesn't if
you use runtime pm.

	Regards
		Oliver

Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[Matthew Garrett]

On Fri, Sep 17, 2010 at 10:39:54AM +0200, Oliver Neukum wrote:
> Am Donnerstag, 16. September 2010, 19:58:13 schrieb Matthew Garrett:
> > Bluetooth runtime PM interacts badly with input devices - the connection
> > will be dropped if the device becomes idle, resulting in noticable lag when
> > the user interacts with the input device again. Bump the pm runtime count
> > when the device is associated and release it when it's disassociated in
> > order to avoid this.
> 
> What is the effect on battery live of the external devices?

That's a reasonable question. I'll see what I can measure.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[Marcel Holtmann]

Hi Matthew,

> Bluetooth runtime PM interacts badly with input devices - the connection
> will be dropped if the device becomes idle, resulting in noticable lag when
> the user interacts with the input device again. Bump the pm runtime count
> when the device is associated and release it when it's disassociated in
> order to avoid this.

we already have hci_conn_hold_device() and hci_conn_put_device() calls
for HID to hold reference of the ACL connection. Why do we need more?

Regards

Marcel

Re: [PATCH 3/3] bluetooth: Enable USB autosuspend by default on btusb

[Marcel Holtmann]

Hi Matthew,

> We've done this for a while in Fedora without any obvious problems other
> than some interaction with input devices. Those should be fixed now, so
> let's try this in mainline.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

sounds good to me.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 3/3] bluetooth: Enable USB autosuspend by default on btusb

[Gustavo F. Padovan]

Hi Matthew,

* Matthew Garrett <mjg@redhat.com> [2010-09-16 13:58:15 -0400]:

> We've done this for a while in Fedora without any obvious problems other
> than some interaction with input devices. Those should be fixed now, so
> let's try this in mainline.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/bluetooth/btusb.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Patch has been applied, thanks.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages

[Marcel Holtmann]

Hi Matthew,

> The main reason for these urbs to error out on submission is that runtime
> pm has kicked in, which is unnecessary noise. Let's just drop them.

do we wanna remove them or just turn into BT_DBG which is under dynamic
debug control. Or do we have a set of error results that we do wanna
print and others that we wanna ignore?

Regards

Marcel

Re: [PATCH 2/3] bluetooth: Remove some unnecessary error messages

[Matthew Garrett]

On Mon, Oct 18, 2010 at 01:55:17PM +0200, Marcel Holtmann wrote:
> Hi Matthew,
> 
> > The main reason for these urbs to error out on submission is that runtime
> > pm has kicked in, which is unnecessary noise. Let's just drop them.
> 
> do we wanna remove them or just turn into BT_DBG which is under dynamic
> debug control. Or do we have a set of error results that we do wanna
> print and others that we wanna ignore?

BT_DBG doesn't seem like an obvious problem. Are there any cases where 
this will actually trigger in a problematic way?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/3] bluetooth: Take a runtime pm reference on hid connections

[Matthew Garrett]

On Wed, Oct 13, 2010 at 12:32:25PM +0300, Marcel Holtmann wrote:
> Hi Matthew,
> 
> > Bluetooth runtime PM interacts badly with input devices - the connection
> > will be dropped if the device becomes idle, resulting in noticable lag when
> > the user interacts with the input device again. Bump the pm runtime count
> > when the device is associated and release it when it's disassociated in
> > order to avoid this.
> 
> we already have hci_conn_hold_device() and hci_conn_put_device() calls
> for HID to hold reference of the ACL connection. Why do we need more?

Because doing so doesn't do anything to influence runtime PM behaviour. 
Putting the calls in these functions might be reasonable though - would 
you prefer that?

-- 
Matthew Garrett | mjg59@srcf.ucam.org