Am Dienstag, 22. Mai 2007 13:28 schrieb Jiri Kosina:
Hi,
> > 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.
I agree that starting the queue as soon as possible is the sensible
approach. The question is what as soon as possible means.
> 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?
No, HID is pretty unique in causing output from its interrupt handlers.
> > --- 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?
To make sure devices opened through hiddev are bot suspended.
> 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.
I did that and Pavel complained about it. I'll work on the
comments.
> > +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.
Ok.
> > @@ -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).
We are. But we are already. What would you do if we overflow the queue?
At present this can only happen if the device is very slow processing
requests, but it is possible. Flow control would be a good idea, but it is
too late at that stage. The prototypes would have to be changed to report
back errors, or the space left.
Regards
Oliver
-------------------------------------------------------------------------
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
