Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210122001326.14263-1-sonnysasaka@chromium.org>
In-Reply-To: <20210122001326.14263-1-sonnysasaka@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 Jan 2021 16:39:14 -0800
Message-ID: <CABBYNZ+OPd_QnHnuhtf0eCx_Vvqy7BSanwjG4ifYVGGEUduHdg@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 1/2] input/hog: Fix double registering report
 value callbacks
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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