| Message ID | 1636011944-3424006-1-git-send-email-jiasheng@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: wacom: Add parse before start | expand |
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 >
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; 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;
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(+)