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
Hi Sonny,
On Thu, Jan 21, 2021 at 4:19 PM Sonny Sasaka <[email protected]> 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;
> +
Didn't I comment on this problem before? I do recall suggesting using
notifyid instead of adding yet another flag.
> 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
>
--
Luiz Augusto von Dentz
Hi Luiz,
Sorry I missed your reply before. I think it's a good idea using
notifyid, let me give it a try.
On Thu, Jan 21, 2021 at 4:39 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Sonny,
>
> On Thu, Jan 21, 2021 at 4:19 PM Sonny Sasaka <[email protected]> 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;
> > +
>
> Didn't I comment on this problem before? I do recall suggesting using
> notifyid instead of adding yet another flag.
>
> > 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
> >
>
>
> --
> Luiz Augusto von Dentz
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=419555
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - PASS
##############################
Test: CheckBuild - PASS
##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth