* re: HID: usbhid: Check HID report descriptor contents after device reset
@ 2015-02-27 9:51 Dan Carpenter
2015-02-27 12:29 ` Simon Haggett
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2015-02-27 9:51 UTC (permalink / raw)
To: simon.haggett; +Cc: linux-input
Hello Simon Haggett,
The patch dc3c78e43469: "HID: usbhid: Check HID report descriptor
contents after device reset" from Apr 3, 2012, leads to the following
static checker warning:
drivers/hid/usbhid/hid-core.c:1591 hid_reset_resume()
warn: bool comparison is always 'true'
drivers/hid/usbhid/hid-core.c
1421 static int hid_post_reset(struct usb_interface *intf)
1422 {
1423 struct usb_device *dev = interface_to_usbdev (intf);
1424 struct hid_device *hid = usb_get_intfdata(intf);
1425 struct usbhid_device *usbhid = hid->driver_data;
1426 struct usb_host_interface *interface = intf->cur_altsetting;
1427 int status;
1428 char *rdesc;
1429
1430 /* Fetch and examine the HID report descriptor. If this
1431 * has changed, then rebind. Since usbcore's check of the
1432 * configuration descriptors passed, we already know that
1433 * the size of the HID report descriptor has not changed.
1434 */
1435 rdesc = kmalloc(hid->dev_rsize, GFP_KERNEL);
1436 if (!rdesc) {
1437 dbg_hid("couldn't allocate rdesc memory (post_reset)\n");
1438 return 1;
We return 1 on error now in this function. I can't figure out if this
is intentional.
1439 }
1440 status = hid_get_class_descriptor(dev,
1441 interface->desc.bInterfaceNumber,
1442 HID_DT_REPORT, rdesc, hid->dev_rsize);
1443 if (status < 0) {
1444 dbg_hid("reading report descriptor failed (post_reset)\n");
1445 kfree(rdesc);
1446 return 1;
1447 }
1448 status = memcmp(rdesc, hid->dev_rdesc, hid->dev_rsize);
1449 kfree(rdesc);
1450 if (status != 0) {
1451 dbg_hid("report descriptor changed\n");
1452 return 1;
1453 }
1454
1455 spin_lock_irq(&usbhid->lock);
1456 clear_bit(HID_RESET_PENDING, &usbhid->iofl);
1457 spin_unlock_irq(&usbhid->lock);
1458 hid_set_idle(dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0);
1459 status = hid_start_in(hid);
1460 if (status < 0)
1461 hid_io_error(hid);
1462 usbhid_restart_queues(usbhid);
Because we hit an error then we don't do this stuff.
1463
1464 return 0;
1465 }
[ snip ]
1589 clear_bit(HID_SUSPENDED, &usbhid->iofl);
1590 status = hid_post_reset(intf);
1591 if (status >= 0 && hid->driver && hid->driver->reset_resume) {
^^^^^^^^^^^
This condition is always true.
1592 int ret = hid->driver->reset_resume(hid);
1593 if (ret < 0)
1594 status = ret;
Maybe the ->reset_resume() can succeed even though hid_post_reset()
failed? In that case, we return the "1" error code and print it in
dmesg in usb_resume_interface(). Strange and uncommented.
1595 }
1596 return status;
1597 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: HID: usbhid: Check HID report descriptor contents after device reset
2015-02-27 9:51 HID: usbhid: Check HID report descriptor contents after device reset Dan Carpenter
@ 2015-02-27 12:29 ` Simon Haggett
0 siblings, 0 replies; 2+ messages in thread
From: Simon Haggett @ 2015-02-27 12:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-input
Hi Dan
>
> The patch dc3c78e43469: "HID: usbhid: Check HID report descriptor
> contents after device reset" from Apr 3, 2012, leads to the following
> static checker warning:
>
> drivers/hid/usbhid/hid-core.c:1591 hid_reset_resume()
> warn: bool comparison is always 'true'
>
> drivers/hid/usbhid/hid-core.c
> 1421 static int hid_post_reset(struct usb_interface *intf)
> 1422 {
> 1423 struct usb_device *dev = interface_to_usbdev (intf);
> 1424 struct hid_device *hid = usb_get_intfdata(intf);
> 1425 struct usbhid_device *usbhid = hid->driver_data;
> 1426 struct usb_host_interface *interface = intf->cur_altsetting;
> 1427 int status;
> 1428 char *rdesc;
> 1429
> 1430 /* Fetch and examine the HID report descriptor. If this
> 1431 * has changed, then rebind. Since usbcore's check of the
> 1432 * configuration descriptors passed, we already know that
> 1433 * the size of the HID report descriptor has not changed.
> 1434 */
> 1435 rdesc = kmalloc(hid->dev_rsize, GFP_KERNEL);
> 1436 if (!rdesc) {
> 1437 dbg_hid("couldn't allocate rdesc memory (post_reset)\n");
> 1438 return 1;
>
> We return 1 on error now in this function. I can't figure out if this
> is intentional.
Originally, this was intentional. The thinking was that if we're stopped
from determining whether or not the report descriptor contents have
changed, we should act as though they have and indicate to
usb_reset_device() that a rebind should occur.
< snip >
>
> 1463
> 1464 return 0;
> 1465 }
>
> [ snip ]
>
> 1589 clear_bit(HID_SUSPENDED, &usbhid->iofl);
> 1590 status = hid_post_reset(intf);
> 1591 if (status >= 0 && hid->driver && hid->driver->reset_resume) {
> ^^^^^^^^^^^
> This condition is always true.
>
> 1592 int ret = hid->driver->reset_resume(hid);
> 1593 if (ret < 0)
> 1594 status = ret;
>
> Maybe the ->reset_resume() can succeed even though hid_post_reset()
> failed? In that case, we return the "1" error code and print it in
> dmesg in usb_resume_interface(). Strange and uncommented.
Yes, it does look like hid_post_reset() should be returning proper error
codes on failure, to allow correct behaviour here. usb_reset_device()
should still interpret such error code returns as a need to attempt a
rebind.
>
> 1595 }
> 1596 return status;
> 1597 }
>
> regards,
> dan carpenter
>
Many thanks
Simon
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-02-27 12:45 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-27 9:51 HID: usbhid: Check HID report descriptor contents after device reset Dan Carpenter
2015-02-27 12:29 ` Simon Haggett
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).