On Tue, 22 May 2007, Oliver Neukum wrote:
> - Jiri, is there a _category_ of HID devices that should not be
> autosuspended at all?
Hi Oliver,
thanks for updating the patch. I am not quite sure right now whether I can
come up with a category that should not be suspended at all, but I will
think a little bit more about this.
> There is a principal issue. What is to be done with output requests?
> Starting the queue as soon as one arrives seems the safest thing to do,
> but it is fatal in a subset of situations, that is, it must not be done
> during snapshotting and when going for system suspend. The freezer is
> insufficient to prevent new requests from coming in.
Starting the queue immediately would be my preferred solution, as has been
already discussed previously in this thread.
Is anyone aware of any better solution for assuring whether it is possible
to restart the queue, apart from adding special hooks into usbcore? Don't
other USB drivers suffer from similar problems?
> --- linux-2.6.22-rc1/drivers/hid/usbhid/hiddev.c 2007-05-13
> 03:45:56.000000000 +0200
> +++ linux-2.6.22-rc1-autosuspend/drivers/hid/usbhid/hiddev.c 2007-05-22
> 09:51:08.000000000 +0200
> @@ -248,10 +248,12 @@ static int hiddev_release(struct inode *
> spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
>
> if (!--list->hiddev->open) {
> - if (list->hiddev->exist)
> + if (list->hiddev->exist) {
> usbhid_close(list->hiddev->hid);
> - else
> + usbhid_put_power(list->hiddev->hid);
> + } else {
> kfree(list->hiddev);
> + }
> }
What is the purpose of this hunk please?
It'd maybe be nice if you provide the cleanups (and this also includes
moving the comments around so that they are wrapped at 80 cols, etc) in a
separate patch. Thanks.
> +static int hidinput_has_ff(struct hid_device *hid)
> +{
> + struct hid_input *hidinput;
> +
> + list_for_each_entry(hidinput, &hid->inputs, list)
> + if (test_bit(EV_FF, hidinput->input->evbit))
> + return 1;
> + return 0;
> +}
This should better go into hid-input.c if possible.
> @@ -406,49 +497,53 @@ void usbhid_submit_report(struct hid_dev
>
> if (usbhid->urbout && dir == USB_DIR_OUT && report->type ==
> HID_OUTPUT_REPORT) {
>
> - spin_lock_irqsave(&usbhid->outlock, flags);
> -
> - if ((head = (usbhid->outhead + 1) & (HID_OUTPUT_FIFO_SIZE - 1))
> == usbhid->outtail) {
> - spin_unlock_irqrestore(&usbhid->outlock, flags);
> - warn("output queue full");
> - return;
> - }
> -
> - usbhid->out[usbhid->outhead] = report;
> - usbhid->outhead = head;
> + usbhid_queue_report_out(hid, report);
Is it OK that we don't check the return value of
usbhid_queue_report_out()? What if the queue becomes full, aren't we
losing the report? (applies also to usbhid_queue_report_ctrl() elsewhere).
Thanks,
--
Jiri Kosina
SUSE Labs
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
