| Message ID | 1404163586-29582-6-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Mon, Jun 30, 2014 at 2:26 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > This removes an USB dependency and is more accurate: the computed pktlen > is the actual maximum size of the reports forwarded by the device. > > Given that the pktlen is correctly computed/validated, we can store it now > in the features struct instead of having a special handling in the rest of > the code. > > Likewise, this information is not mandatory anymore in the description > of devices in wacom_wac.c. They will be removed in a separate patch. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> I'm concerned this new function could be fooled if we release a tablet that has one report which is longer than the desired report, but none of the hardware I tested was like this and it would be fairly easy to address even if it did happen. Thought I should mention it though. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... > --- > drivers/input/tablet/wacom_sys.c | 58 ++++++++++++++++++---------------------- > 1 file changed, 26 insertions(+), 32 deletions(-) > > diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c > index cd3d936..3f1cee6 100644 > --- a/drivers/input/tablet/wacom_sys.c > +++ b/drivers/input/tablet/wacom_sys.c > @@ -149,7 +149,6 @@ static int wacom_parse_logical_collection(unsigned char *report, > if (features->type == BAMBOO_PT) { > > /* Logical collection is only used by 3rd gen Bamboo Touch */ > - features->pktlen = WACOM_PKGLEN_BBTOUCH3; > features->device_type = BTN_TOOL_FINGER; > > features->x_max = features->y_max = > @@ -240,29 +239,6 @@ static int wacom_parse_hid(struct hid_device *hdev, > features->device_type = BTN_TOOL_FINGER; > /* touch device at least supports one touch point */ > touch_max = 1; > - switch (features->type) { > - case TABLETPC2FG: > - features->pktlen = WACOM_PKGLEN_TPC2FG; > - break; > - > - case MTSCREEN: > - case WACOM_24HDT: > - features->pktlen = WACOM_PKGLEN_MTOUCH; > - break; > - > - case MTTPC: > - case MTTPC_B: > - features->pktlen = WACOM_PKGLEN_MTTPC; > - break; > - > - case BAMBOO_PT: > - features->pktlen = WACOM_PKGLEN_BBTOUCH; > - break; > - > - default: > - features->pktlen = WACOM_PKGLEN_GRAPHIRE; > - break; > - } > > switch (features->type) { > case BAMBOO_PT: > @@ -305,8 +281,6 @@ static int wacom_parse_hid(struct hid_device *hdev, > } > } else if (pen) { > /* penabled only accepts exact bytes of data */ > - if (features->type >= TABLETPC) > - features->pktlen = WACOM_PKGLEN_GRAPHIRE; > features->device_type = BTN_TOOL_PEN; > features->x_max = > get_unaligned_le16(&report[i + 3]); > @@ -1227,12 +1201,34 @@ static void wacom_calculate_res(struct wacom_features *features) > features->unitExpo); > } > > +static int wacom_hid_report_len(struct hid_report *report) > +{ > + /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ > + return ((report->size - 1) >> 3) + 1 + (report->id > 0); > +} > + > +static size_t wacom_compute_pktlen(struct hid_device *hdev) > +{ > + struct hid_report_enum *report_enum; > + struct hid_report *report; > + size_t size = 0; > + > + report_enum = hdev->report_enum + HID_INPUT_REPORT; > + > + list_for_each_entry(report, &report_enum->report_list, list) { > + size_t report_size = wacom_hid_report_len(report); > + if (report_size > size) > + size = report_size; > + } > + > + return size; > +} > + > static int wacom_probe(struct hid_device *hdev, > const struct hid_device_id *id) > { > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > struct usb_device *dev = interface_to_usbdev(intf); > - struct usb_endpoint_descriptor *endpoint; > struct wacom *wacom; > struct wacom_wac *wacom_wac; > struct wacom_features *features; > @@ -1258,6 +1254,7 @@ static int wacom_probe(struct hid_device *hdev, > wacom_wac = &wacom->wacom_wac; > wacom_wac->features = *((struct wacom_features *)id->driver_data); > features = &wacom_wac->features; > + features->pktlen = wacom_compute_pktlen(hdev); > if (features->pktlen > WACOM_PKGLEN_MAX) { > error = -EINVAL; > goto fail1; > @@ -1275,8 +1272,6 @@ static int wacom_probe(struct hid_device *hdev, > usb_make_path(dev, wacom->phys, sizeof(wacom->phys)); > strlcat(wacom->phys, "/input0", sizeof(wacom->phys)); > > - endpoint = &intf->cur_altsetting->endpoint[0].desc; > - > /* set the default size in case we do not get them from hid */ > wacom_set_default_phy(features); > > @@ -1287,13 +1282,12 @@ static int wacom_probe(struct hid_device *hdev, > > /* > * Intuos5 has no useful data about its touch interface in its > - * HID descriptor. If this is the touch interface (wMaxPacketSize > + * HID descriptor. If this is the touch interface (PacketSize > * of WACOM_PKGLEN_BBTOUCH3), override the table values. > */ > if (features->type >= INTUOS5S && features->type <= INTUOSHT) { > - if (endpoint->wMaxPacketSize == WACOM_PKGLEN_BBTOUCH3) { > + if (features->pktlen == WACOM_PKGLEN_BBTOUCH3) { > features->device_type = BTN_TOOL_FINGER; > - features->pktlen = WACOM_PKGLEN_BBTOUCH3; > > features->x_max = 4096; > features->y_max = 4096; > -- > 2.0.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 10, 2014 at 9:09 PM, Jason Gerecke <killertofu@gmail.com> wrote: > On Mon, Jun 30, 2014 at 2:26 PM, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: >> This removes an USB dependency and is more accurate: the computed pktlen >> is the actual maximum size of the reports forwarded by the device. >> >> Given that the pktlen is correctly computed/validated, we can store it now >> in the features struct instead of having a special handling in the rest of >> the code. >> >> Likewise, this information is not mandatory anymore in the description >> of devices in wacom_wac.c. They will be removed in a separate patch. >> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > I'm concerned this new function could be fooled if we release a tablet > that has one report which is longer than the desired report, but none > of the hardware I tested was like this and it would be fairly easy to > address even if it did happen. Thought I should mention it though. > You are right to mention it. But in the other hand, such a device would violate the HID specification, and I am not sure the Windows driver (which I don't know its internal) would be happy too. Anyway, as you said, if there is such a device, we an use the report descriptor fixup capability of HID to prevent this from happening. Cheers, Benjamin > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > > >> --- >> drivers/input/tablet/wacom_sys.c | 58 ++++++++++++++++++---------------------- >> 1 file changed, 26 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c >> index cd3d936..3f1cee6 100644 >> --- a/drivers/input/tablet/wacom_sys.c >> +++ b/drivers/input/tablet/wacom_sys.c >> @@ -149,7 +149,6 @@ static int wacom_parse_logical_collection(unsigned char *report, >> if (features->type == BAMBOO_PT) { >> >> /* Logical collection is only used by 3rd gen Bamboo Touch */ >> - features->pktlen = WACOM_PKGLEN_BBTOUCH3; >> features->device_type = BTN_TOOL_FINGER; >> >> features->x_max = features->y_max = >> @@ -240,29 +239,6 @@ static int wacom_parse_hid(struct hid_device *hdev, >> features->device_type = BTN_TOOL_FINGER; >> /* touch device at least supports one touch point */ >> touch_max = 1; >> - switch (features->type) { >> - case TABLETPC2FG: >> - features->pktlen = WACOM_PKGLEN_TPC2FG; >> - break; >> - >> - case MTSCREEN: >> - case WACOM_24HDT: >> - features->pktlen = WACOM_PKGLEN_MTOUCH; >> - break; >> - >> - case MTTPC: >> - case MTTPC_B: >> - features->pktlen = WACOM_PKGLEN_MTTPC; >> - break; >> - >> - case BAMBOO_PT: >> - features->pktlen = WACOM_PKGLEN_BBTOUCH; >> - break; >> - >> - default: >> - features->pktlen = WACOM_PKGLEN_GRAPHIRE; >> - break; >> - } >> >> switch (features->type) { >> case BAMBOO_PT: >> @@ -305,8 +281,6 @@ static int wacom_parse_hid(struct hid_device *hdev, >> } >> } else if (pen) { >> /* penabled only accepts exact bytes of data */ >> - if (features->type >= TABLETPC) >> - features->pktlen = WACOM_PKGLEN_GRAPHIRE; >> features->device_type = BTN_TOOL_PEN; >> features->x_max = >> get_unaligned_le16(&report[i + 3]); >> @@ -1227,12 +1201,34 @@ static void wacom_calculate_res(struct wacom_features *features) >> features->unitExpo); >> } >> >> +static int wacom_hid_report_len(struct hid_report *report) >> +{ >> + /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ >> + return ((report->size - 1) >> 3) + 1 + (report->id > 0); >> +} >> + >> +static size_t wacom_compute_pktlen(struct hid_device *hdev) >> +{ >> + struct hid_report_enum *report_enum; >> + struct hid_report *report; >> + size_t size = 0; >> + >> + report_enum = hdev->report_enum + HID_INPUT_REPORT; >> + >> + list_for_each_entry(report, &report_enum->report_list, list) { >> + size_t report_size = wacom_hid_report_len(report); >> + if (report_size > size) >> + size = report_size; >> + } >> + >> + return size; >> +} >> + >> static int wacom_probe(struct hid_device *hdev, >> const struct hid_device_id *id) >> { >> struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> struct usb_device *dev = interface_to_usbdev(intf); >> - struct usb_endpoint_descriptor *endpoint; >> struct wacom *wacom; >> struct wacom_wac *wacom_wac; >> struct wacom_features *features; >> @@ -1258,6 +1254,7 @@ static int wacom_probe(struct hid_device *hdev, >> wacom_wac = &wacom->wacom_wac; >> wacom_wac->features = *((struct wacom_features *)id->driver_data); >> features = &wacom_wac->features; >> + features->pktlen = wacom_compute_pktlen(hdev); >> if (features->pktlen > WACOM_PKGLEN_MAX) { >> error = -EINVAL; >> goto fail1; >> @@ -1275,8 +1272,6 @@ static int wacom_probe(struct hid_device *hdev, >> usb_make_path(dev, wacom->phys, sizeof(wacom->phys)); >> strlcat(wacom->phys, "/input0", sizeof(wacom->phys)); >> >> - endpoint = &intf->cur_altsetting->endpoint[0].desc; >> - >> /* set the default size in case we do not get them from hid */ >> wacom_set_default_phy(features); >> >> @@ -1287,13 +1282,12 @@ static int wacom_probe(struct hid_device *hdev, >> >> /* >> * Intuos5 has no useful data about its touch interface in its >> - * HID descriptor. If this is the touch interface (wMaxPacketSize >> + * HID descriptor. If this is the touch interface (PacketSize >> * of WACOM_PKGLEN_BBTOUCH3), override the table values. >> */ >> if (features->type >= INTUOS5S && features->type <= INTUOSHT) { >> - if (endpoint->wMaxPacketSize == WACOM_PKGLEN_BBTOUCH3) { >> + if (features->pktlen == WACOM_PKGLEN_BBTOUCH3) { >> features->device_type = BTN_TOOL_FINGER; >> - features->pktlen = WACOM_PKGLEN_BBTOUCH3; >> >> features->x_max = 4096; >> features->y_max = 4096; >> -- >> 2.0.0 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c index cd3d936..3f1cee6 100644 --- a/drivers/input/tablet/wacom_sys.c +++ b/drivers/input/tablet/wacom_sys.c @@ -149,7 +149,6 @@ static int wacom_parse_logical_collection(unsigned char *report, if (features->type == BAMBOO_PT) { /* Logical collection is only used by 3rd gen Bamboo Touch */ - features->pktlen = WACOM_PKGLEN_BBTOUCH3; features->device_type = BTN_TOOL_FINGER; features->x_max = features->y_max = @@ -240,29 +239,6 @@ static int wacom_parse_hid(struct hid_device *hdev, features->device_type = BTN_TOOL_FINGER; /* touch device at least supports one touch point */ touch_max = 1; - switch (features->type) { - case TABLETPC2FG: - features->pktlen = WACOM_PKGLEN_TPC2FG; - break; - - case MTSCREEN: - case WACOM_24HDT: - features->pktlen = WACOM_PKGLEN_MTOUCH; - break; - - case MTTPC: - case MTTPC_B: - features->pktlen = WACOM_PKGLEN_MTTPC; - break; - - case BAMBOO_PT: - features->pktlen = WACOM_PKGLEN_BBTOUCH; - break; - - default: - features->pktlen = WACOM_PKGLEN_GRAPHIRE; - break; - } switch (features->type) { case BAMBOO_PT: @@ -305,8 +281,6 @@ static int wacom_parse_hid(struct hid_device *hdev, } } else if (pen) { /* penabled only accepts exact bytes of data */ - if (features->type >= TABLETPC) - features->pktlen = WACOM_PKGLEN_GRAPHIRE; features->device_type = BTN_TOOL_PEN; features->x_max = get_unaligned_le16(&report[i + 3]); @@ -1227,12 +1201,34 @@ static void wacom_calculate_res(struct wacom_features *features) features->unitExpo); } +static int wacom_hid_report_len(struct hid_report *report) +{ + /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */ + return ((report->size - 1) >> 3) + 1 + (report->id > 0); +} + +static size_t wacom_compute_pktlen(struct hid_device *hdev) +{ + struct hid_report_enum *report_enum; + struct hid_report *report; + size_t size = 0; + + report_enum = hdev->report_enum + HID_INPUT_REPORT; + + list_for_each_entry(report, &report_enum->report_list, list) { + size_t report_size = wacom_hid_report_len(report); + if (report_size > size) + size = report_size; + } + + return size; +} + static int wacom_probe(struct hid_device *hdev, const struct hid_device_id *id) { struct usb_interface *intf = to_usb_interface(hdev->dev.parent); struct usb_device *dev = interface_to_usbdev(intf); - struct usb_endpoint_descriptor *endpoint; struct wacom *wacom; struct wacom_wac *wacom_wac; struct wacom_features *features; @@ -1258,6 +1254,7 @@ static int wacom_probe(struct hid_device *hdev, wacom_wac = &wacom->wacom_wac; wacom_wac->features = *((struct wacom_features *)id->driver_data); features = &wacom_wac->features; + features->pktlen = wacom_compute_pktlen(hdev); if (features->pktlen > WACOM_PKGLEN_MAX) { error = -EINVAL; goto fail1; @@ -1275,8 +1272,6 @@ static int wacom_probe(struct hid_device *hdev, usb_make_path(dev, wacom->phys, sizeof(wacom->phys)); strlcat(wacom->phys, "/input0", sizeof(wacom->phys)); - endpoint = &intf->cur_altsetting->endpoint[0].desc; - /* set the default size in case we do not get them from hid */ wacom_set_default_phy(features); @@ -1287,13 +1282,12 @@ static int wacom_probe(struct hid_device *hdev, /* * Intuos5 has no useful data about its touch interface in its - * HID descriptor. If this is the touch interface (wMaxPacketSize + * HID descriptor. If this is the touch interface (PacketSize * of WACOM_PKGLEN_BBTOUCH3), override the table values. */ if (features->type >= INTUOS5S && features->type <= INTUOSHT) { - if (endpoint->wMaxPacketSize == WACOM_PKGLEN_BBTOUCH3) { + if (features->pktlen == WACOM_PKGLEN_BBTOUCH3) { features->device_type = BTN_TOOL_FINGER; - features->pktlen = WACOM_PKGLEN_BBTOUCH3; features->x_max = 4096; features->y_max = 4096;
This removes an USB dependency and is more accurate: the computed pktlen is the actual maximum size of the reports forwarded by the device. Given that the pktlen is correctly computed/validated, we can store it now in the features struct instead of having a special handling in the rest of the code. Likewise, this information is not mandatory anymore in the description of devices in wacom_wac.c. They will be removed in a separate patch. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/input/tablet/wacom_sys.c | 58 ++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 32 deletions(-)