Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1636011944-3424006-1-git-send-email-jiasheng@iscas.ac.cn>
In-Reply-To: <1636011944-3424006-1-git-send-email-jiasheng@iscas.ac.cn>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Fri, 5 Nov 2021 19:36:40 +0100
Message-ID: <CAO-hwJJZdSnJ1ysOk_UACsxwhg-1u3SEg8rvykCEaHzqY5-pmQ@mail.gmail.com>
Subject: Re: [PATCH] HID: wacom: Add parse before start
To:     Jiasheng Jiang <jiasheng@iscas.ac.cn>
Cc:     Jiri Kosina <jikos@kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jiasheng,

On Thu, Nov 4, 2021 at 8:52 AM Jiasheng Jiang <jiasheng@iscas.ac.cn> wrote:
>
> It might be better to add  hid_parse() before
> wacom_parse_and_register() to ask for the report descriptor
> like what wacom_probe() does.
>
> Fixes: 471d171 ("Input: wacom - move the USB (now hid) Wacom driver in drivers/hid")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  drivers/hid/wacom_sys.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 57bfa0a..48cb2e4 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2486,6 +2486,9 @@ static void wacom_wireless_work(struct work_struct *work)
>
>                 wacom_wac1->pid = wacom_wac->pid;
>                 hid_hw_stop(hdev1);
> +               error = hid_parse(wacom1->hdev);
> +               if (error)
> +                       goto fail;

I am pretty sure we don't need those calls (everywhere in this patch).

hid_parse() has the effect of requesting the transport layer to pull
the report descriptor and then parses it at the hid core level.
However, we are called here in callbacks after wacom_probe() has been
called already once for those devices (wacom1 and wacom2).
So basically, hid_parse() is called in wacom_probe(), we store the hid
device in a shared space in the driver, and then when the work is
called because a new device is connected, we just pull that hid device
(already parsed) and present it to the userspace.

Another fact that makes me think we are already doing the right thing
is that if hid_parse() is not called on a hid device, we have a
segfault while processing the events. And here, we don't.

Cheers,
Benjamin

>                 error = wacom_parse_and_register(wacom1, true);
>                 if (error)
>                         goto fail;
> @@ -2498,6 +2501,9 @@ static void wacom_wireless_work(struct work_struct *work)
>                                 *((struct wacom_features *)id->driver_data);
>                         wacom_wac2->pid = wacom_wac->pid;
>                         hid_hw_stop(hdev2);
> +                       error = hid_parse(wacom2->hdev);
> +                       if (error)
> +                               goto fail;
>                         error = wacom_parse_and_register(wacom2, true);
>                         if (error)
>                                 goto fail;
> @@ -2710,12 +2716,18 @@ static void wacom_mode_change_work(struct work_struct *work)
>         }
>
>         if (wacom1) {
> +               error = hid_parse(wacom1->hdev);
> +               if (error)
> +                       return;
>                 error = wacom_parse_and_register(wacom1, false);
>                 if (error)
>                         return;
>         }
>
>         if (wacom2) {
> +               error = hid_parse(wacom2->hdev);
> +               if (error)
> +                       return;
>                 error = wacom_parse_and_register(wacom2, false);
>                 if (error)
>                         return;
> --
> 2.7.4
>