[PATCH BlueZ] input/hog: Fix double registering report value callbacks

[PATCH BlueZ] input/hog: Fix double registering report value callbacks

[Sonny Sasaka]

In commit 23b69ab3e484 ("input/hog: Cache the HID report map"), we
optimized HOG reconnection by registering report value callbacks early,
but there was a bug where we also re-register the same report value
callbacks after at CCC write callback. We should handle this case by
avoiding the second callback register if we know we have done it
earlier.

---
 profiles/input/hog-lib.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 1f132aa4c..089f42826 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -80,6 +80,7 @@ struct bt_hog {
 	struct bt_uhid		*uhid;
 	int			uhid_fd;
 	bool			uhid_created;
+	bool			report_value_cb_registered;
 	gboolean		has_report_id;
 	uint16_t		bcdhid;
 	uint8_t			bcountrycode;
@@ -336,6 +337,13 @@ static void report_ccc_written_cb(guint8 status, const guint8 *pdu,
 		return;
 	}
 
+	/* If we already had the report map cache, we must have registered UHID
+	 * and the report value callbacks. In that case, don't re-register the
+	 * report value callbacks here.
+	 */
+	if (hog->report_value_cb_registered)
+		return;
+
 	report->notifyid = g_attrib_register(hog->attrib,
 					ATT_OP_HANDLE_NOTIFY,
 					report->value_handle,
@@ -1703,6 +1711,8 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
 					report_value_cb, r, NULL);
 	}
 
+	hog->report_value_cb_registered = true;
+
 	return true;
 }
 
@@ -1753,6 +1763,8 @@ void bt_hog_detach(struct bt_hog *hog)
 		}
 	}
 
+	hog->report_value_cb_registered = false;
+
 	if (hog->scpp)
 		bt_scpp_detach(hog->scpp);
 
-- 
2.29.2

RE: [BlueZ] input/hog: Fix double registering report value callbacks

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 557 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=411857

---Test result---

##############################
Test: CheckPatch - PASS

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

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

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



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ] input/hog: Fix double registering report value callbacks

[Luiz Augusto von Dentz]

Hi Sonny,

On Sat, Jan 9, 2021 at 4:34 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> In commit 23b69ab3e484 ("input/hog: Cache the HID report map"), we
> optimized HOG reconnection by registering report value callbacks early,
> but there was a bug where we also re-register the same report value
> callbacks after at CCC write callback. We should handle this case by
> avoiding the second callback register if we know we have done it
> earlier.
>
> ---
>  profiles/input/hog-lib.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 1f132aa4c..089f42826 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -80,6 +80,7 @@ struct bt_hog {
>         struct bt_uhid          *uhid;
>         int                     uhid_fd;
>         bool                    uhid_created;
> +       bool                    report_value_cb_registered;
>         gboolean                has_report_id;
>         uint16_t                bcdhid;
>         uint8_t                 bcountrycode;
> @@ -336,6 +337,13 @@ static void report_ccc_written_cb(guint8 status, const guint8 *pdu,
>                 return;
>         }
>
> +       /* If we already had the report map cache, we must have registered UHID
> +        * and the report value callbacks. In that case, don't re-register the
> +        * report value callbacks here.
> +        */
> +       if (hog->report_value_cb_registered)
> +               return;
> +

Can't we check the report->notifyid instead of introducing yet another
flag that seems to have the same intent of tracking if the report has
been subscribed? In fact it seem there is something odd related to how
we handle the CCC here, we do read it on ccc_read_cb but we don't
check if its value is already set. Pehaps something like the following
would be more complete solution, though we should really look into
convert hog-lib to use bt_gatt_client instead of keep using GAttrib:

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 1f132aa4c..34a4d7c3b 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -360,15 +360,30 @@ static void ccc_read_cb(guint8 status, const
guint8 *pdu, guint16 len,
 {
        struct gatt_request *req = user_data;
        struct report *report = req->user_data;
+       uint16_t value;

        destroy_gatt_req(req);

-       if (status != 0) {
+       if (status || !len) {
                error("Error reading CCC value: %s", att_ecode2str(status));
                return;
        }

-       write_ccc(report->hog, report->hog->attrib, report->ccc_handle, report);
+       if (len == 1)
+               value = *pdu;
+       else
+               value = get_le16(pdu);
+
+       if (!value) {
+               write_ccc(report->hog, report->hog->attrib, report->ccc_handle,
+                         report);
+               return;
+       }
+
+       report->notifyid = g_attrib_register(report->hog->attrib,
+                                       ATT_OP_HANDLE_NOTIFY,
+                                       report->value_handle,
+                                       report_value_cb, report, NULL);
 }

Re: [PATCH BlueZ] input/hog: Fix double registering report value callbacks

[Sonny Sasaka]

Hi Luiz,

I have revised the patch to using the notifyid to detect double
registration. Please take a another look. Thanks!

On Mon, Jan 11, 2021 at 9:47 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Sat, Jan 9, 2021 at 4:34 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > In commit 23b69ab3e484 ("input/hog: Cache the HID report map"), we
> > optimized HOG reconnection by registering report value callbacks early,
> > but there was a bug where we also re-register the same report value
> > callbacks after at CCC write callback. We should handle this case by
> > avoiding the second callback register if we know we have done it
> > earlier.
> >
> > ---
> >  profiles/input/hog-lib.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > index 1f132aa4c..089f42826 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -80,6 +80,7 @@ struct bt_hog {
> >         struct bt_uhid          *uhid;
> >         int                     uhid_fd;
> >         bool                    uhid_created;
> > +       bool                    report_value_cb_registered;
> >         gboolean                has_report_id;
> >         uint16_t                bcdhid;
> >         uint8_t                 bcountrycode;
> > @@ -336,6 +337,13 @@ static void report_ccc_written_cb(guint8 status, const guint8 *pdu,
> >                 return;
> >         }
> >
> > +       /* If we already had the report map cache, we must have registered UHID
> > +        * and the report value callbacks. In that case, don't re-register the
> > +        * report value callbacks here.
> > +        */
> > +       if (hog->report_value_cb_registered)
> > +               return;
> > +
>
> Can't we check the report->notifyid instead of introducing yet another
> flag that seems to have the same intent of tracking if the report has
> been subscribed? In fact it seem there is something odd related to how
> we handle the CCC here, we do read it on ccc_read_cb but we don't
> check if its value is already set. Pehaps something like the following
> would be more complete solution, though we should really look into
> convert hog-lib to use bt_gatt_client instead of keep using GAttrib:
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 1f132aa4c..34a4d7c3b 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -360,15 +360,30 @@ static void ccc_read_cb(guint8 status, const
> guint8 *pdu, guint16 len,
>  {
>         struct gatt_request *req = user_data;
>         struct report *report = req->user_data;
> +       uint16_t value;
>
>         destroy_gatt_req(req);
>
> -       if (status != 0) {
> +       if (status || !len) {
>                 error("Error reading CCC value: %s", att_ecode2str(status));
>                 return;
>         }
>
> -       write_ccc(report->hog, report->hog->attrib, report->ccc_handle, report);
> +       if (len == 1)
> +               value = *pdu;
> +       else
> +               value = get_le16(pdu);
> +
> +       if (!value) {
> +               write_ccc(report->hog, report->hog->attrib, report->ccc_handle,
> +                         report);
> +               return;
> +       }
> +
> +       report->notifyid = g_attrib_register(report->hog->attrib,
> +                                       ATT_OP_HANDLE_NOTIFY,
> +                                       report->value_handle,
> +                                       report_value_cb, report, NULL);
>  }