| Message ID | f6d623eecc635022b85a31359b2f11b0104267c5.camel@snewbury.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | 100% CPU usage on keyboard disconnect | expand |
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
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.
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;
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(-)