Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] 100% CPU usage on keyboard disconnect
@ 2020-10-15  7:16 Steven Newbury
  2020-10-15  7:32 ` bluez.test.bot
  2020-10-15 17:12 ` [PATCH] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 4+ messages in thread
From: Steven Newbury @ 2020-10-15  7:16 UTC (permalink / raw)
  To: linux-bluetooth

There are a couple of issues with g_io_channel usage in bluez which
cause CPUs to spin on half-closed channels.

This patch fixes bugs where bluetooth keyboards fail to work on initial
connection, and cause 100% cpu on disconnect.

Also fix bug with similar symptoms triggered by some other HID devices
such as Sony PS3 BD Remotes.

In the previous discussion on the kernel bugzilla below, it was
suggested to remove sec_watch, and I attached a follow-up patch to do
so, however that change causes problems with current bluez-5 releases
where a fd is used after being closed.

See https://bugzilla.kernel.org/show_bug.cgi?id=204275

Signed-off-by: Steven Newbury <steve@snewbury.org.uk>
---
 attrib/interactive.c    | 4 +++-
 profiles/input/device.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/attrib/interactive.c b/attrib/interactive.c
index 9a7976d34..453ff064e 100644
--- a/attrib/interactive.c
+++ b/attrib/interactive.c
@@ -64,6 +64,7 @@ static int opt_psm = 0;
 static int opt_mtu = 0;
 static int start;
 static int end;
+static guint gsrc;
 
 static void cmd_help(int argcp, char **argvp);
 
@@ -193,6 +194,7 @@ static void disconnect_io()
 	attrib = NULL;
 	opt_mtu = 0;
 
+	g_source_remove(gsrc);
 	g_io_channel_shutdown(iochannel, FALSE, NULL);
 	g_io_channel_unref(iochannel);
 	iochannel = NULL;
@@ -415,7 +417,7 @@ static void cmd_connect(int argcp, char **argvp)
 		error("%s\n", gerr->message);
 		g_error_free(gerr);
 	} else
-		g_io_add_watch(iochannel, G_IO_HUP, channel_watcher, NULL);
+		gsrc = g_io_add_watch(iochannel, G_IO_HUP, channel_watcher, NULL);
 }
 
 static void cmd_disconnect(int argcp, char **argvp)
diff --git a/profiles/input/device.c b/profiles/input/device.c
index a711ef527..9abf595f6 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -982,7 +982,8 @@ static int hidp_add_connection(struct input_device *idev)
 		}
 
 		idev->req = req;
-		idev->sec_watch = g_io_add_watch(idev->intr_io, G_IO_OUT,
+		if (!idev->sec_watch)
+			idev->sec_watch = g_io_add_watch(idev->intr_io, G_IO_IN,
 							encrypt_notify, idev);
 
 		return 0;
-- 
2.22.0



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

* RE: 100% CPU usage on keyboard disconnect
  2020-10-15  7:16 [PATCH] 100% CPU usage on keyboard disconnect Steven Newbury
@ 2020-10-15  7:32 ` bluez.test.bot
  2020-10-15 17:12 ` [PATCH] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2020-10-15  7:32 UTC (permalink / raw)
  To: linux-bluetooth, steve


[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=364897

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
100% CPU usage on keyboard disconnect
WARNING:LONG_LINE: line over 80 characters
#49: FILE: attrib/interactive.c:407:
+		gsrc = g_io_add_watch(iochannel, G_IO_HUP, channel_watcher, NULL);

- total: 0 errors, 1 warnings, 31 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth


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

* Re: [PATCH] 100% CPU usage on keyboard disconnect
  2020-10-15  7:16 [PATCH] 100% CPU usage on keyboard disconnect Steven Newbury
  2020-10-15  7:32 ` bluez.test.bot
@ 2020-10-15 17:12 ` Luiz Augusto von Dentz
  2020-10-15 20:36   ` Steven Newbury
  1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-15 17:12 UTC (permalink / raw)
  To: Steven Newbury; +Cc: linux-bluetooth

Hi Steven,

On Thu, Oct 15, 2020 at 4:13 AM Steven Newbury <steve@snewbury.org.uk> wrote:
>
> There are a couple of issues with g_io_channel usage in bluez which
> cause CPUs to spin on half-closed channels.
>
> This patch fixes bugs where bluetooth keyboards fail to work on initial
> connection, and cause 100% cpu on disconnect.
>
> Also fix bug with similar symptoms triggered by some other HID devices
> such as Sony PS3 BD Remotes.
>
> In the previous discussion on the kernel bugzilla below, it was
> suggested to remove sec_watch, and I attached a follow-up patch to do
> so, however that change causes problems with current bluez-5 releases
> where a fd is used after being closed.
>
> See https://bugzilla.kernel.org/show_bug.cgi?id=204275
>
> Signed-off-by: Steven Newbury <steve@snewbury.org.uk>
> ---
>  attrib/interactive.c    | 4 +++-
>  profiles/input/device.c | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/attrib/interactive.c b/attrib/interactive.c
> index 9a7976d34..453ff064e 100644
> --- a/attrib/interactive.c
> +++ b/attrib/interactive.c
> @@ -64,6 +64,7 @@ static int opt_psm = 0;
>  static int opt_mtu = 0;
>  static int start;
>  static int end;
> +static guint gsrc;
>
>  static void cmd_help(int argcp, char **argvp);
>
> @@ -193,6 +194,7 @@ static void disconnect_io()
>         attrib = NULL;
>         opt_mtu = 0;
>
> +       g_source_remove(gsrc);
>         g_io_channel_shutdown(iochannel, FALSE, NULL);
>         g_io_channel_unref(iochannel);
>         iochannel = NULL;
> @@ -415,7 +417,7 @@ static void cmd_connect(int argcp, char **argvp)
>                 error("%s\n", gerr->message);
>                 g_error_free(gerr);
>         } else
> -               g_io_add_watch(iochannel, G_IO_HUP, channel_watcher, NULL);
> +               gsrc = g_io_add_watch(iochannel, G_IO_HUP, channel_watcher, NULL);
>  }

I wouldn't bother with the fix above since the attrib part will be
going away soon.

>  static void cmd_disconnect(int argcp, char **argvp)
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index a711ef527..9abf595f6 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -982,7 +982,8 @@ static int hidp_add_connection(struct input_device *idev)
>                 }
>
>                 idev->req = req;
> -               idev->sec_watch = g_io_add_watch(idev->intr_io, G_IO_OUT,
> +               if (!idev->sec_watch)
> +                       idev->sec_watch = g_io_add_watch(idev->intr_io, G_IO_IN,
>                                                         encrypt_notify, idev);

If this is happening isn't there a idev->req already set and we are
overwriting it?

>                 return 0;
> --
> 2.22.0
>
>


-- 
Luiz Augusto von Dentz

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

* Re: [PATCH] 100% CPU usage on keyboard disconnect
  2020-10-15 17:12 ` [PATCH] " Luiz Augusto von Dentz
@ 2020-10-15 20:36   ` Steven Newbury
  0 siblings, 0 replies; 4+ messages in thread
From: Steven Newbury @ 2020-10-15 20:36 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

On Thu, 2020-10-15 at 10:12 -0700, Luiz Augusto von Dentz wrote:
> Hi Steven,
> 
> On Thu, Oct 15, 2020 at 4:13 AM Steven Newbury <steve@snewbury.org.uk
> > wrote:
> > There are a couple of issues with g_io_channel usage in bluez which
> > cause CPUs to spin on half-closed channels.
> > 
> > This patch fixes bugs where bluetooth keyboards fail to work on
> > initial
> > connection, and cause 100% cpu on disconnect.
> > 
> > Also fix bug with similar symptoms triggered by some other HID
> > devices
> > such as Sony PS3 BD Remotes.
> > 
> > In the previous discussion on the kernel bugzilla below, it was
> > suggested to remove sec_watch, and I attached a follow-up patch to
> > do
> > so, however that change causes problems with current bluez-5
> > releases
> > where a fd is used after being closed.
> > 
> > See https://bugzilla.kernel.org/show_bug.cgi?id=204275
> > 
> > Signed-off-by: Steven Newbury <steve@snewbury.org.uk>
> > ---
> >  attrib/interactive.c    | 4 +++-
> >  profiles/input/device.c | 3 ++-
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/attrib/interactive.c b/attrib/interactive.c
> > index 9a7976d34..453ff064e 100644
> > --- a/attrib/interactive.c
> > +++ b/attrib/interactive.c
> > @@ -64,6 +64,7 @@ static int opt_psm = 0;
> >  static int opt_mtu = 0;
> >  static int start;
> >  static int end;
> > +static guint gsrc;
> > 
> >  static void cmd_help(int argcp, char **argvp);
> > 
> > @@ -193,6 +194,7 @@ static void disconnect_io()
> >         attrib = NULL;
> >         opt_mtu = 0;
> > 
> > +       g_source_remove(gsrc);
> >         g_io_channel_shutdown(iochannel, FALSE, NULL);
> >         g_io_channel_unref(iochannel);
> >         iochannel = NULL;
> > @@ -415,7 +417,7 @@ static void cmd_connect(int argcp, char
> > **argvp)
> >                 error("%s\n", gerr->message);
> >                 g_error_free(gerr);
> >         } else
> > -               g_io_add_watch(iochannel, G_IO_HUP,
> > channel_watcher, NULL);
> > +               gsrc = g_io_add_watch(iochannel, G_IO_HUP,
> > channel_watcher, NULL);
> >  }
> 
> I wouldn't bother with the fix above since the attrib part will be
> going away soon.
> 
> >  static void cmd_disconnect(int argcp, char **argvp)
> > diff --git a/profiles/input/device.c b/profiles/input/device.c
> > index a711ef527..9abf595f6 100644
> > --- a/profiles/input/device.c
> > +++ b/profiles/input/device.c
> > @@ -982,7 +982,8 @@ static int hidp_add_connection(struct
> > input_device *idev)
> >                 }
> > 
> >                 idev->req = req;
> > -               idev->sec_watch = g_io_add_watch(idev->intr_io,
> > G_IO_OUT,
> > +               if (!idev->sec_watch)
> > +                       idev->sec_watch = g_io_add_watch(idev-
> > >intr_io, G_IO_IN,
> >                                                         encrypt_not
> > ify, idev);
> 
> If this is happening isn't there a idev->req already set and we are
> overwriting it?
> 
It was definitely causing a problem, but I can't remember exactly what
occurred, I wrote the patch originally years ago!  I think it leaked
the watch.



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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15  7:16 [PATCH] 100% CPU usage on keyboard disconnect Steven Newbury
2020-10-15  7:32 ` bluez.test.bot
2020-10-15 17:12 ` [PATCH] " Luiz Augusto von Dentz
2020-10-15 20:36   ` Steven Newbury

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git