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

[PATCH BlueZ v2 1/2] 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

[PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map is broken

[Sonny Sasaka]

The report map in the cache could be dirty, for example when reading a
report map from peer was cancelled, we should be able to detect it and
not try to create UHID. Instead we will read it again from the peer.

---
 profiles/input/hog-lib.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 089f42826..d6a3bda4d 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 	struct uhid_event ev;
 	ssize_t vlen = report_map_len;
 	char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
-	int i, err;
+	int i, err, collection_depth = 0;
 	GError *gerr = NULL;
 
 	DBG("Report MAP:");
@@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 			if (!long_item && (value[i] & 0xfc) == 0x84)
 				hog->has_report_id = TRUE;
 
+			// Start Collection
+			if (value[i] == 0xa1)
+				collection_depth++;
+
+			// End Collection
+			if (value[i] == 0xc0)
+				collection_depth--;
+
 			DBG("\t%s", item2string(itemstr, &value[i], ilen));
 
 			i += ilen;
@@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 
 			/* Just print remaining items at once and break */
 			DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
-			break;
+			return;
 		}
 	}
 
+	if (collection_depth != 0) {
+		error("Report Map error: unbalanced collection");
+		return;
+	}
+
 	/* create uHID device */
 	memset(&ev, 0, sizeof(ev));
 	ev.type = UHID_CREATE;
@@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
 			 * UHID to optimize reconnection.
 			 */
 			uhid_create(hog, report_map.value, report_map.length);
-		} else {
+		}
+
+		if (!hog->uhid_created) {
 			read_char(hog, hog->attrib, value_handle,
 						report_map_read_cb, hog);
 		}
-- 
2.29.2

Re: [PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map is broken

[Luiz Augusto von Dentz]

Hi Sonny,

On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> The report map in the cache could be dirty, for example when reading a
> report map from peer was cancelled, we should be able to detect it and
> not try to create UHID. Instead we will read it again from the peer.

Don't we clean the cache if it had failed? Or you meant to say the
read long procedure was not complete so we got just part of the report
map? In that case we should have failed, also if we need to protect
uhid from malformed report map, which sounds like a kernel bug, then
we should at least have it inside bt_uhid instance so we can at least
attempt to have some unit testing done with broken report maps.

> ---
>  profiles/input/hog-lib.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 089f42826..d6a3bda4d 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
>         struct uhid_event ev;
>         ssize_t vlen = report_map_len;
>         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> -       int i, err;
> +       int i, err, collection_depth = 0;
>         GError *gerr = NULL;
>
>         DBG("Report MAP:");
> @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
>                         if (!long_item && (value[i] & 0xfc) == 0x84)
>                                 hog->has_report_id = TRUE;
>
> +                       // Start Collection
> +                       if (value[i] == 0xa1)
> +                               collection_depth++;
> +
> +                       // End Collection
> +                       if (value[i] == 0xc0)
> +                               collection_depth--;
> +
>                         DBG("\t%s", item2string(itemstr, &value[i], ilen));
>
>                         i += ilen;
> @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
>
>                         /* Just print remaining items at once and break */
>                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
> -                       break;
> +                       return;
>                 }
>         }
>
> +       if (collection_depth != 0) {
> +               error("Report Map error: unbalanced collection");
> +               return;
> +       }
> +
>         /* create uHID device */
>         memset(&ev, 0, sizeof(ev));
>         ev.type = UHID_CREATE;
> @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
>                          * UHID to optimize reconnection.
>                          */
>                         uhid_create(hog, report_map.value, report_map.length);
> -               } else {
> +               }
> +
> +               if (!hog->uhid_created) {
>                         read_char(hog, hog->attrib, value_handle,
>                                                 report_map_read_cb, hog);
>                 }
> --
> 2.29.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map is broken

[Sonny Sasaka]

Hi Luiz,

On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > The report map in the cache could be dirty, for example when reading a
> > report map from peer was cancelled, we should be able to detect it and
> > not try to create UHID. Instead we will read it again from the peer.
>
> Don't we clean the cache if it had failed? Or you meant to say the
> read long procedure was not complete so we got just part of the report
> map?
Looks like this is the case. It happened to me once when I cancel
reconnection (trigger pairing mode during reconnection) from the
keyboard side. It's hard to confirm since I have to get the timing
right.

> In that case we should have failed
I agree. However it seems that the code already tries to fail by
looking at the status inside report_map_read_cb, but somehow it still
gets through. It could be the keyboard bug that we have to detect
anyway?

> also if we need to protect
> uhid from malformed report map, which sounds like a kernel bug, then
> we should at least have it inside bt_uhid instance so we can at least
> attempt to have some unit testing done with broken report maps.
>
> > ---
> >  profiles/input/hog-lib.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > index 089f42826..d6a3bda4d 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> >         struct uhid_event ev;
> >         ssize_t vlen = report_map_len;
> >         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > -       int i, err;
> > +       int i, err, collection_depth = 0;
> >         GError *gerr = NULL;
> >
> >         DBG("Report MAP:");
> > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> >                         if (!long_item && (value[i] & 0xfc) == 0x84)
> >                                 hog->has_report_id = TRUE;
> >
> > +                       // Start Collection
> > +                       if (value[i] == 0xa1)
> > +                               collection_depth++;
> > +
> > +                       // End Collection
> > +                       if (value[i] == 0xc0)
> > +                               collection_depth--;
> > +
> >                         DBG("\t%s", item2string(itemstr, &value[i], ilen));
> >
> >                         i += ilen;
> > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> >
> >                         /* Just print remaining items at once and break */
> >                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
> > -                       break;
> > +                       return;
> >                 }
> >         }
> >
> > +       if (collection_depth != 0) {
> > +               error("Report Map error: unbalanced collection");
> > +               return;
> > +       }
> > +
> >         /* create uHID device */
> >         memset(&ev, 0, sizeof(ev));
> >         ev.type = UHID_CREATE;
> > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> >                          * UHID to optimize reconnection.
> >                          */
> >                         uhid_create(hog, report_map.value, report_map.length);
> > -               } else {
> > +               }
> > +
> > +               if (!hog->uhid_created) {
> >                         read_char(hog, hog->attrib, value_handle,
> >                                                 report_map_read_cb, hog);
> >                 }
> > --
> > 2.29.2
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map is broken

[Sonny Sasaka]

Hi Luiz,

I have been trying to reproduce the issue again but it turns out to be
very rare. Let's defer this patch until I can get a clear log of what
is happening and why we get the corrupted cache.



On Thu, Jan 21, 2021 at 5:24 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
> On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > The report map in the cache could be dirty, for example when reading a
> > > report map from peer was cancelled, we should be able to detect it and
> > > not try to create UHID. Instead we will read it again from the peer.
> >
> > Don't we clean the cache if it had failed? Or you meant to say the
> > read long procedure was not complete so we got just part of the report
> > map?
> Looks like this is the case. It happened to me once when I cancel
> reconnection (trigger pairing mode during reconnection) from the
> keyboard side. It's hard to confirm since I have to get the timing
> right.
>
> > In that case we should have failed
> I agree. However it seems that the code already tries to fail by
> looking at the status inside report_map_read_cb, but somehow it still
> gets through. It could be the keyboard bug that we have to detect
> anyway?
>
> > also if we need to protect
> > uhid from malformed report map, which sounds like a kernel bug, then
> > we should at least have it inside bt_uhid instance so we can at least
> > attempt to have some unit testing done with broken report maps.
> >
> > > ---
> > >  profiles/input/hog-lib.c | 21 ++++++++++++++++++---
> > >  1 file changed, 18 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > > index 089f42826..d6a3bda4d 100644
> > > --- a/profiles/input/hog-lib.c
> > > +++ b/profiles/input/hog-lib.c
> > > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > >         struct uhid_event ev;
> > >         ssize_t vlen = report_map_len;
> > >         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > > -       int i, err;
> > > +       int i, err, collection_depth = 0;
> > >         GError *gerr = NULL;
> > >
> > >         DBG("Report MAP:");
> > > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > >                         if (!long_item && (value[i] & 0xfc) == 0x84)
> > >                                 hog->has_report_id = TRUE;
> > >
> > > +                       // Start Collection
> > > +                       if (value[i] == 0xa1)
> > > +                               collection_depth++;
> > > +
> > > +                       // End Collection
> > > +                       if (value[i] == 0xc0)
> > > +                               collection_depth--;
> > > +
> > >                         DBG("\t%s", item2string(itemstr, &value[i], ilen));
> > >
> > >                         i += ilen;
> > > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > >
> > >                         /* Just print remaining items at once and break */
> > >                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
> > > -                       break;
> > > +                       return;
> > >                 }
> > >         }
> > >
> > > +       if (collection_depth != 0) {
> > > +               error("Report Map error: unbalanced collection");
> > > +               return;
> > > +       }
> > > +
> > >         /* create uHID device */
> > >         memset(&ev, 0, sizeof(ev));
> > >         ev.type = UHID_CREATE;
> > > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > >                          * UHID to optimize reconnection.
> > >                          */
> > >                         uhid_create(hog, report_map.value, report_map.length);
> > > -               } else {
> > > +               }
> > > +
> > > +               if (!hog->uhid_created) {
> > >                         read_char(hog, hog->attrib, value_handle,
> > >                                                 report_map_read_cb, hog);
> > >                 }
> > > --
> > > 2.29.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map is broken

[Luiz Augusto von Dentz]

Hi Sonny,

On Mon, Jan 25, 2021 at 11:36 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
> I have been trying to reproduce the issue again but it turns out to be
> very rare. Let's defer this patch until I can get a clear log of what
> is happening and why we get the corrupted cache.

Ok, let me update in the pw, if you see this again let me know.

>
>
> On Thu, Jan 21, 2021 at 5:24 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > The report map in the cache could be dirty, for example when reading a
> > > > report map from peer was cancelled, we should be able to detect it and
> > > > not try to create UHID. Instead we will read it again from the peer.
> > >
> > > Don't we clean the cache if it had failed? Or you meant to say the
> > > read long procedure was not complete so we got just part of the report
> > > map?
> > Looks like this is the case. It happened to me once when I cancel
> > reconnection (trigger pairing mode during reconnection) from the
> > keyboard side. It's hard to confirm since I have to get the timing
> > right.
> >
> > > In that case we should have failed
> > I agree. However it seems that the code already tries to fail by
> > looking at the status inside report_map_read_cb, but somehow it still
> > gets through. It could be the keyboard bug that we have to detect
> > anyway?
> >
> > > also if we need to protect
> > > uhid from malformed report map, which sounds like a kernel bug, then
> > > we should at least have it inside bt_uhid instance so we can at least
> > > attempt to have some unit testing done with broken report maps.
> > >
> > > > ---
> > > >  profiles/input/hog-lib.c | 21 ++++++++++++++++++---
> > > >  1 file changed, 18 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > > > index 089f42826..d6a3bda4d 100644
> > > > --- a/profiles/input/hog-lib.c
> > > > +++ b/profiles/input/hog-lib.c
> > > > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > > >         struct uhid_event ev;
> > > >         ssize_t vlen = report_map_len;
> > > >         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > > > -       int i, err;
> > > > +       int i, err, collection_depth = 0;
> > > >         GError *gerr = NULL;
> > > >
> > > >         DBG("Report MAP:");
> > > > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > > >                         if (!long_item && (value[i] & 0xfc) == 0x84)
> > > >                                 hog->has_report_id = TRUE;
> > > >
> > > > +                       // Start Collection
> > > > +                       if (value[i] == 0xa1)
> > > > +                               collection_depth++;
> > > > +
> > > > +                       // End Collection
> > > > +                       if (value[i] == 0xc0)
> > > > +                               collection_depth--;
> > > > +
> > > >                         DBG("\t%s", item2string(itemstr, &value[i], ilen));
> > > >
> > > >                         i += ilen;
> > > > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > > >
> > > >                         /* Just print remaining items at once and break */
> > > >                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
> > > > -                       break;
> > > > +                       return;
> > > >                 }
> > > >         }
> > > >
> > > > +       if (collection_depth != 0) {
> > > > +               error("Report Map error: unbalanced collection");
> > > > +               return;
> > > > +       }
> > > > +
> > > >         /* create uHID device */
> > > >         memset(&ev, 0, sizeof(ev));
> > > >         ev.type = UHID_CREATE;
> > > > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > > >                          * UHID to optimize reconnection.
> > > >                          */
> > > >                         uhid_create(hog, report_map.value, report_map.length);
> > > > -               } else {
> > > > +               }
> > > > +
> > > > +               if (!hog->uhid_created) {
> > > >                         read_char(hog, hog->attrib, value_handle,
> > > >                                                 report_map_read_cb, hog);
> > > >                 }
> > > > --
> > > > 2.29.2
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

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

[Luiz Augusto von Dentz]

Hi Sonny,

On Thu, Jan 21, 2021 at 4:19 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;
> +

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

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

[Sonny Sasaka]

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
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Thu, Jan 21, 2021 at 4:19 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;
> > +
>
> 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

RE: [BlueZ,v2,1/2] 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=419555

---Test result---

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

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

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

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



---
Regards,
Linux Bluetooth