On Mon, 14 Mar 2016, Oliver Neukum wrote:
> On Mon, 2016-03-14 at 11:03 -0400, Alan Stern wrote:
> > On Mon, 14 Mar 2016, Oliver Neukum wrote:
>
> > > It seems to me that hid_resume_common() needs to be split into three
> > > parts
> > >
> > > a) doing the stuff for pending resets
> > > b) conditionally restarting IO
> > > c) reset the flag
> >
> > Daniel's problem involved hid_post_reset(), not hid_resume_common().
> > Is that what you meant? Maybe I'm wrong, but it looks like
> > hid_post_reset() just needs to check whether I/O should be started
> > before trying to restart it.
>
> Yes. That means that all that hid_post_reset() does before calling
> hid_start_in() must be left unchanged and we can put the change into
> a common code path.
>
> > (It also looks like hid_resume() needs to clear the HID_SUSPENDED flag
> > even when HID_STARTED isn't set.)
>
> Exactly. Thus hid_resume_common() needs to be split. The code paths
> for reset_resume(), post_reset() and resume() diverge too much. You will
> see that they can be easily be reused if you split them into
> those three parts.
All right, I have taken Oliver's suggestion. The patch below refactors
the code to consolidate the common activities in a new function,
hid_restart_io().
Daniel, can you please test this patch?
Alan Stern
Index: usb-4.4/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-4.4.orig/drivers/hid/usbhid/hid-core.c
+++ usb-4.4/drivers/hid/usbhid/hid-core.c
@@ -951,14 +951,6 @@ static int usbhid_output_report(struct h
return ret;
}
-static void usbhid_restart_queues(struct usbhid_device *usbhid)
-{
- if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
- usbhid_restart_out_queue(usbhid);
- if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
- usbhid_restart_ctrl_queue(usbhid);
-}
-
static void hid_free_buffers(struct usb_device *dev, struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
@@ -1404,6 +1396,33 @@ static void hid_cease_io(struct usbhid_d
usb_kill_urb(usbhid->urbout);
}
+static void hid_restart_io(struct hid_device *hid)
+{
+ struct usbhid_device *usbhid = hid->driver_data;
+
+ spin_lock_irq(&usbhid->lock);
+ clear_bit(HID_SUSPENDED, &usbhid->iofl);
+ usbhid_mark_busy(usbhid);
+
+ if (test_bit(HID_CLEAR_HALT, &usbhid->iofl) ||
+ test_bit(HID_RESET_PENDING, &usbhid->iofl))
+ schedule_work(&usbhid->reset_work);
+ usbhid->retry_delay = 0;
+ spin_unlock_irq(&usbhid->lock);
+
+ if (test_bit(HID_STARTED, &usbhid->iofl)) {
+ if (hid_start_in(hid) < 0)
+ hid_io_error(hid);
+
+ spin_lock_irq(&usbhid->lock);
+ if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
+ usbhid_restart_out_queue(usbhid);
+ if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+ usbhid_restart_ctrl_queue(usbhid);
+ spin_unlock_irq(&usbhid->lock);
+ }
+}
+
/* Treat USB reset pretty much the same as suspend/resume */
static int hid_pre_reset(struct usb_interface *intf)
{
@@ -1457,10 +1476,8 @@ static int hid_post_reset(struct usb_int
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
hid_set_idle(dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0);
- status = hid_start_in(hid);
- if (status < 0)
- hid_io_error(hid);
- usbhid_restart_queues(usbhid);
+
+ hid_restart_io(hid);
return 0;
}
@@ -1483,25 +1500,9 @@ void usbhid_put_power(struct hid_device
#ifdef CONFIG_PM
static int hid_resume_common(struct hid_device *hid, bool driver_suspended)
{
- struct usbhid_device *usbhid = hid->driver_data;
- int status;
-
- spin_lock_irq(&usbhid->lock);
- clear_bit(HID_SUSPENDED, &usbhid->iofl);
- usbhid_mark_busy(usbhid);
-
- if (test_bit(HID_CLEAR_HALT, &usbhid->iofl) ||
- test_bit(HID_RESET_PENDING, &usbhid->iofl))
- schedule_work(&usbhid->reset_work);
- usbhid->retry_delay = 0;
-
- usbhid_restart_queues(usbhid);
- spin_unlock_irq(&usbhid->lock);
-
- status = hid_start_in(hid);
- if (status < 0)
- hid_io_error(hid);
+ int status = 0;
+ hid_restart_io(hid);
if (driver_suspended && hid->driver && hid->driver->resume)
status = hid->driver->resume(hid);
return status;
@@ -1570,12 +1571,8 @@ static int hid_suspend(struct usb_interf
static int hid_resume(struct usb_interface *intf)
{
struct hid_device *hid = usb_get_intfdata (intf);
- struct usbhid_device *usbhid = hid->driver_data;
int status;
- if (!test_bit(HID_STARTED, &usbhid->iofl))
- return 0;
-
status = hid_resume_common(hid, true);
dev_dbg(&intf->dev, "resume status %d\n", status);
return 0;
@@ -1584,10 +1581,8 @@ static int hid_resume(struct usb_interfa
static int hid_reset_resume(struct usb_interface *intf)
{
struct hid_device *hid = usb_get_intfdata(intf);
- struct usbhid_device *usbhid = hid->driver_data;
int status;
- clear_bit(HID_SUSPENDED, &usbhid->iofl);
status = hid_post_reset(intf);
if (status >= 0 && hid->driver && hid->driver->reset_resume) {
int ret = hid->driver->reset_resume(hid);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
