Sleeping inside spinlock in force feedback input event code

Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Hi all!

It seems a new spinlock input_dev->event_lock has been added [1] to the
input subsystem since the force feedback support was reworked.

However, the force feedback subsystem sleeps on events in multiple
places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
io (otherwise commands were lost, IIRC; if necessary I'll test again).

ff_device->mutex is used to shield effects[], so it is locked when
handling EV_FF events, on flushes, and on effect upload and erase ioctls.

Maybe we should make EV_FF handling atomic? For effect uploading we
could either make it completely atomic, or lock only for reserving the
effect slot, then release the lock, and mark it as ready after upload is
complete.
Making even the upload completely atomic would mean that no force
feedback events/ioctl() would sleep, which AFAIK would be a plus for
userspace ff applications. On the other hand, hid-pidff (device managed
mode) driver doesn't know whether effect upload was successful until it
has received a report from the device, so it wouldn't be able to report
failure immediately. Other drivers would, though.

What do you think?

I just noticed this (due to James' freeze report below), so I haven't
yet put much thought into this.

[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8006479c9b75fb6594a7b746af3d7f1fbb68f18f

James Carthew wrote:
> ok I managed to get sysrq to print some stuff out by switching to
> console logging level 6:
> BUG: scheduling while atomic: ffcfstress/6888/0x00000003
> PID: 6888, comm: ffcfstress Tainted: P 2.6.26-rc5 #2
> [<c0757a2f>] schedule+0x9b/0x5f9
> [<c0226ccf>] lock_timer_base+0x19/0x35
> [<c0759431>] _spin_unlock_irqrestore+0xe/0x21]
> [<c0226de0>] __mod_timer+0x97/0xa1
> [<c075813f>] schedule_timeout+0x6b/0x86
> [<c0226bb5>] process_timeout+0x0/0x5
> [<c069ad55>] usbhid_wait_io+0x76/0xb9
> [<c022eebd>] autoremove_wake_function+0x0/0x2b
> [<c069c8c1>] pidff_playback_pid+0x3c/0x49
> [<c069c9ac>] pidff_playback+0x15/0x18
> [<c0665dd6>] input_ff_event+0x79/0x89
> [<c0665d5d>] input_ff_event+0x0/0x89
> [<c0664d5d>] input_handle_event+0x32a/0x362
> [<c0665728>] input_inject_event+0x59/0x92
> [<c0668bff>] evdev_write+0x77/0x84
> [<c0668b88>] evdev_write+0x0/0x84
> [<c025ed95>] vfs_write+0x83/0xf6
> [<c025f2a2>] sys_write+0x3c/0x63
> [<c02038dd>] sysenter_past_esp+0x6a/0x91


-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Dmitry Torokhov]

Hi Anssi,

On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
> Hi all!
> 
> It seems a new spinlock input_dev->event_lock has been added [1] to the
> input subsystem since the force feedback support was reworked.
> 
> However, the force feedback subsystem sleeps on events in multiple
> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
> io (otherwise commands were lost, IIRC; if necessary I'll test again).
> 
> ff_device->mutex is used to shield effects[], so it is locked when
> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
> 
> Maybe we should make EV_FF handling atomic? For effect uploading we
> could either make it completely atomic, or lock only for reserving the
> effect slot, then release the lock, and mark it as ready after upload is
> complete.
> Making even the upload completely atomic would mean that no force
> feedback events/ioctl() would sleep, which AFAIK would be a plus for
> userspace ff applications. On the other hand, hid-pidff (device managed
> mode) driver doesn't know whether effect upload was successful until it
> has received a report from the device, so it wouldn't be able to report
> failure immediately. Other drivers would, though.
> 
> What do you think?
> 

I think something the patch below is what is needed. EV_FF handling is
already atomic because of event_lock (and it is here to stay), but
uploading does not need to be atomic, only installing into effect
table needs the lock. Any change you could test the patch? I dont have
any FF devices.

Thanks!

-- 
Dmitry


Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/ff-core.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: linux/drivers/input/ff-core.c
===================================================================
--- linux.orig/drivers/input/ff-core.c
+++ linux/drivers/input/ff-core.c
@@ -166,8 +166,10 @@ int input_ff_upload(struct input_dev *de
 	if (ret)
 		goto out;
 
+	spin_lock_irq(&dev->event_lock);
 	ff->effects[id] = *effect;
 	ff->effect_owners[id] = file;
+	spin_unlock_irq(&dev->event_lock);
 
  out:
 	mutex_unlock(&ff->mutex);
@@ -189,16 +191,22 @@ static int erase_effect(struct input_dev
 	if (error)
 		return error;
 
+	spin_lock_irq(&dev->event_lock);
 	ff->playback(dev, effect_id, 0);
+	ff->effect_owners[effect_id] = NULL;
+	spin_unlock_irq(&dev->event_lock);
 
 	if (ff->erase) {
 		error = ff->erase(dev, effect_id);
-		if (error)
+		if (error) {
+			spin_lock_irq(&dev->event_lock);
+			ff->effect_owners[effect_id] = file;
+			spin_unlock_irq(&dev->event_lock);
+
 			return error;
+		}
 	}
 
-	ff->effect_owners[effect_id] = NULL;
-
 	return 0;
 }
 
@@ -263,8 +271,6 @@ int input_ff_event(struct input_dev *dev
 	if (type != EV_FF)
 		return 0;
 
-	mutex_lock(&ff->mutex);
-
 	switch (code) {
 	case FF_GAIN:
 		if (!test_bit(FF_GAIN, dev->ffbit) || value > 0xffff)
@@ -286,7 +292,6 @@ int input_ff_event(struct input_dev *dev
 		break;
 	}
 
-	mutex_unlock(&ff->mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(input_ff_event);

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

(Added Jiri Kosina due to the hid problem I describe near the end)

Dmitry Torokhov wrote:
> Hi Anssi,
> 
> On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
>> Hi all!
>>
>> It seems a new spinlock input_dev->event_lock has been added [1] to the
>> input subsystem since the force feedback support was reworked.
>>
>> However, the force feedback subsystem sleeps on events in multiple
>> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
>> io (otherwise commands were lost, IIRC; if necessary I'll test again).
>>
>> ff_device->mutex is used to shield effects[], so it is locked when
>> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
>>
>> Maybe we should make EV_FF handling atomic? For effect uploading we
>> could either make it completely atomic, or lock only for reserving the
>> effect slot, then release the lock, and mark it as ready after upload is
>> complete.
>> Making even the upload completely atomic would mean that no force
>> feedback events/ioctl() would sleep, which AFAIK would be a plus for
>> userspace ff applications. On the other hand, hid-pidff (device managed
>> mode) driver doesn't know whether effect upload was successful until it
>> has received a report from the device, so it wouldn't be able to report
>> failure immediately. Other drivers would, though.
>>
>> What do you think?
>>
> 
> I think something the patch below is what is needed. EV_FF handling is
> already atomic because of event_lock (and it is here to stay), but
> uploading does not need to be atomic, only installing into effect
> table needs the lock. Any change you could test the patch? I dont have
> any FF devices.

It seems to be ok, but not enough. The hid-pidff.c driver also waits on
pidff_playback_pid(). However, I now see that the wait is probably only
necessary because just the report pointer is passed to
usbhid_submit_report(). But fixing it properly seems non-trivial (to me).

E.g. the problem sequence is:

- playback_pid() gets called to stop effect 1.
- it sets control_report->field[X]->value[X] = 1;
- it submits control_report
- thus usbhid_submit_report() stores a pointer to the report
- playback_pid() gets immediately called again for effect 2.
- it sets control_report->field[X]->value[X] = 2;
- thus the previous report hasn't yet been submitted, but the report
content has already changed, thus effect 1 is never stopped.

Any idea how this should be solved properly?

-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Dmitry Torokhov]

On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
> (Added Jiri Kosina due to the hid problem I describe near the end)
> 
> Dmitry Torokhov wrote:
> > Hi Anssi,
> > 
> > On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
> >> Hi all!
> >>
> >> It seems a new spinlock input_dev->event_lock has been added [1] to the
> >> input subsystem since the force feedback support was reworked.
> >>
> >> However, the force feedback subsystem sleeps on events in multiple
> >> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
> >> io (otherwise commands were lost, IIRC; if necessary I'll test again).
> >>
> >> ff_device->mutex is used to shield effects[], so it is locked when
> >> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
> >>
> >> Maybe we should make EV_FF handling atomic? For effect uploading we
> >> could either make it completely atomic, or lock only for reserving the
> >> effect slot, then release the lock, and mark it as ready after upload is
> >> complete.
> >> Making even the upload completely atomic would mean that no force
> >> feedback events/ioctl() would sleep, which AFAIK would be a plus for
> >> userspace ff applications. On the other hand, hid-pidff (device managed
> >> mode) driver doesn't know whether effect upload was successful until it
> >> has received a report from the device, so it wouldn't be able to report
> >> failure immediately. Other drivers would, though.
> >>
> >> What do you think?
> >>
> > 
> > I think something the patch below is what is needed. EV_FF handling is
> > already atomic because of event_lock (and it is here to stay), but
> > uploading does not need to be atomic, only installing into effect
> > table needs the lock. Any change you could test the patch? I dont have
> > any FF devices.
> 
> It seems to be ok, but not enough. The hid-pidff.c driver also waits on
> pidff_playback_pid(). However, I now see that the wait is probably only
> necessary because just the report pointer is passed to
> usbhid_submit_report(). But fixing it properly seems non-trivial (to me).
> 
> E.g. the problem sequence is:
> 
> - playback_pid() gets called to stop effect 1.
> - it sets control_report->field[X]->value[X] = 1;
> - it submits control_report
> - thus usbhid_submit_report() stores a pointer to the report
> - playback_pid() gets immediately called again for effect 2.
> - it sets control_report->field[X]->value[X] = 2;
> - thus the previous report hasn't yet been submitted, but the report
> content has already changed, thus effect 1 is never stopped.
> 
> Any idea how this should be solved properly?
> 

It looks like there is a common issue with HID FF devices. Pid driver
tries to handle it by inserting waits till the control queue is
cleared, other drivers are completely ignorant of this problem...

I guess we need to implement a queue of events to be played and put it
in hid-ff.c so it is available for all hid ff drivers.

-- 
Dmitry

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Dmitry Torokhov wrote:
> On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
>> (Added Jiri Kosina due to the hid problem I describe near the end)
>>
>> Dmitry Torokhov wrote:
>>> Hi Anssi,
>>>
>>> On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
>>>> Hi all!
>>>>
>>>> It seems a new spinlock input_dev->event_lock has been added [1] to the
>>>> input subsystem since the force feedback support was reworked.
>>>>
>>>> However, the force feedback subsystem sleeps on events in multiple
>>>> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
>>>> io (otherwise commands were lost, IIRC; if necessary I'll test again).
>>>>
>>>> ff_device->mutex is used to shield effects[], so it is locked when
>>>> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
>>>>
>>>> Maybe we should make EV_FF handling atomic? For effect uploading we
>>>> could either make it completely atomic, or lock only for reserving the
>>>> effect slot, then release the lock, and mark it as ready after upload is
>>>> complete.
>>>> Making even the upload completely atomic would mean that no force
>>>> feedback events/ioctl() would sleep, which AFAIK would be a plus for
>>>> userspace ff applications. On the other hand, hid-pidff (device managed
>>>> mode) driver doesn't know whether effect upload was successful until it
>>>> has received a report from the device, so it wouldn't be able to report
>>>> failure immediately. Other drivers would, though.
>>>>
>>>> What do you think?
>>>>
>>> I think something the patch below is what is needed. EV_FF handling is
>>> already atomic because of event_lock (and it is here to stay), but
>>> uploading does not need to be atomic, only installing into effect
>>> table needs the lock. Any change you could test the patch? I dont have
>>> any FF devices.
>> It seems to be ok, but not enough. The hid-pidff.c driver also waits on
>> pidff_playback_pid(). However, I now see that the wait is probably only
>> necessary because just the report pointer is passed to
>> usbhid_submit_report(). But fixing it properly seems non-trivial (to me).
>>
>> E.g. the problem sequence is:
>>
>> - playback_pid() gets called to stop effect 1.
>> - it sets control_report->field[X]->value[X] = 1;
>> - it submits control_report
>> - thus usbhid_submit_report() stores a pointer to the report
>> - playback_pid() gets immediately called again for effect 2.
>> - it sets control_report->field[X]->value[X] = 2;
>> - thus the previous report hasn't yet been submitted, but the report
>> content has already changed, thus effect 1 is never stopped.
>>
>> Any idea how this should be solved properly?
>>
> 
> It looks like there is a common issue with HID FF devices. Pid driver
> tries to handle it by inserting waits till the control queue is
> cleared, other drivers are completely ignorant of this problem...
> I guess we need to implement a queue of events to be played and put it
> in hid-ff.c so it is available for all hid ff drivers.

With other HID FF drivers than hid-pidff we actually want to skip to the
last one, though (the report contains complete device state, so skipping
old ones does not matter). But indeed of course we still shouldn't be
modifying the just-submitted reports since hid_output_report() could be
reading them at the same time.

-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Anssi Hannula wrote:
> Dmitry Torokhov wrote:
>   
>> On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
>>     
>>> (Added Jiri Kosina due to the hid problem I describe near the end)
>>>
>>> Dmitry Torokhov wrote:
>>>       
>>>> Hi Anssi,
>>>>
>>>> On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
>>>>         
>>>>> Hi all!
>>>>>
>>>>> It seems a new spinlock input_dev->event_lock has been added [1] to the
>>>>> input subsystem since the force feedback support was reworked.
>>>>>
>>>>> However, the force feedback subsystem sleeps on events in multiple
>>>>> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
>>>>> io (otherwise commands were lost, IIRC; if necessary I'll test again).
>>>>>
>>>>> ff_device->mutex is used to shield effects[], so it is locked when
>>>>> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
>>>>>
>>>>> Maybe we should make EV_FF handling atomic? For effect uploading we
>>>>> could either make it completely atomic, or lock only for reserving the
>>>>> effect slot, then release the lock, and mark it as ready after upload is
>>>>> complete.
>>>>> Making even the upload completely atomic would mean that no force
>>>>> feedback events/ioctl() would sleep, which AFAIK would be a plus for
>>>>> userspace ff applications. On the other hand, hid-pidff (device managed
>>>>> mode) driver doesn't know whether effect upload was successful until it
>>>>> has received a report from the device, so it wouldn't be able to report
>>>>> failure immediately. Other drivers would, though.
>>>>>
>>>>> What do you think?
>>>>>
>>>>>           
>>>> I think something the patch below is what is needed. EV_FF handling is
>>>> already atomic because of event_lock (and it is here to stay), but
>>>> uploading does not need to be atomic, only installing into effect
>>>> table needs the lock. Any change you could test the patch? I dont have
>>>> any FF devices.
>>>>         
>>> It seems to be ok, but not enough. The hid-pidff.c driver also waits on
>>> pidff_playback_pid(). However, I now see that the wait is probably only
>>> necessary because just the report pointer is passed to
>>> usbhid_submit_report(). But fixing it properly seems non-trivial (to me).
>>>
>>> E.g. the problem sequence is:
>>>
>>> - playback_pid() gets called to stop effect 1.
>>> - it sets control_report->field[X]->value[X] = 1;
>>> - it submits control_report
>>> - thus usbhid_submit_report() stores a pointer to the report
>>> - playback_pid() gets immediately called again for effect 2.
>>> - it sets control_report->field[X]->value[X] = 2;
>>> - thus the previous report hasn't yet been submitted, but the report
>>> content has already changed, thus effect 1 is never stopped.
>>>
>>> Any idea how this should be solved properly?
>>>
>>>       
>> It looks like there is a common issue with HID FF devices. Pid driver
>> tries to handle it by inserting waits till the control queue is
>> cleared, other drivers are completely ignorant of this problem...
>> I guess we need to implement a queue of events to be played and put it
>> in hid-ff.c so it is available for all hid ff drivers.
>>     
>
> With other HID FF drivers than hid-pidff we actually want to skip to the
> last one, though (the report contains complete device state, so skipping
> old ones does not matter). But indeed of course we still shouldn't be
> modifying the just-submitted reports since hid_output_report() could be
> reading them at the same time.
>
>   
I tried to come up with something today.

Since the only place where the integrity of all reports (i.e. not just
the last one) is critical is hid-pidff, I implemented report submission
in workqueue for that. It seems to work.

As was said, though, there are conditions in other places which could
cause corrupted reports to be sent to devices. However, AFAICS it
doesn't seem to be a common issue with just HID FF devices, but all code
using usbhid_submit_report() with USB_DIR_OUT. Therefore I'm not sure
if any queue implementation in hid-ff.c would be the correct fix, but
instead it should be fixed so that non-FF users (leds) would be fixed
too. To achieve that, I modified usbhid_submit_report() to copy the
report contents before returning. This seems to work as well. I added
usbhid_wait_io() to effect removal in hid-pidff to prevent an event
flood from preventing the removal.

Note that even without a fix, any invalid reports would immediately
be resent with correct content (as any code modifying a report calls
usbhid_submit_report() again).

That said, I do prefer the usbhid_submit_report() change instead of
the workqueue one.
WDYT? Or do you have a better way in mind?

Below is the queue-raw-reports solution. I can post the alternative
hid-pidff-workqueue solution as well if you want me to.


Signed-off-by: Anssi Hannula <anssi.hannula@gmail.com>
---
Not tested extensively yet, just gathering comments.

--- linux-2.6.26-0.rc5.1mnb-fftest/include/linux/hid.h.orig	2008-06-12 15:39:31.000000000 +0300
+++ linux-2.6.26-0.rc5.1mnb-fftest/include/linux/hid.h	2008-06-29 03:02:30.000000000 +0300
@@ -411,6 +411,12 @@
 struct hid_control_fifo {
 	unsigned char dir;
 	struct hid_report *report;
+	char *raw_report;
+};
+
+struct hid_output_fifo {
+	struct hid_report *report;
+	char *raw_report;
 };
 
 #define HID_CLAIMED_INPUT	1
--- linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/usbhid.h.orig	2008-06-12 15:38:11.000000000 +0300
+++ linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/usbhid.h	2008-06-29 02:47:22.000000000 +0300
@@ -67,7 +67,7 @@
 	spinlock_t ctrllock;                                            /* Control fifo spinlock */
 
 	struct urb *urbout;                                             /* Output URB */
-	struct hid_report *out[HID_CONTROL_FIFO_SIZE];                  /* Output pipe fifo */
+	struct hid_output_fifo out[HID_CONTROL_FIFO_SIZE];              /* Output pipe fifo */
 	unsigned char outhead, outtail;                                 /* Output pipe fifo head & tail */
 	char *outbuf;                                                   /* Output buffer */
 	dma_addr_t outbuf_dma;                                          /* Output buffer dma */
--- linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-core.c.orig	2008-06-12 15:38:11.000000000 +0300
+++ linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-core.c	2008-06-29 03:01:42.000000000 +0300
@@ -240,13 +240,16 @@
 static int hid_submit_out(struct hid_device *hid)
 {
 	struct hid_report *report;
+	char *raw_report;
 	struct usbhid_device *usbhid = hid->driver_data;
 
-	report = usbhid->out[usbhid->outtail];
+	report = usbhid->out[usbhid->outtail].report;
+	raw_report = usbhid->out[usbhid->outtail].raw_report;
 
-	hid_output_report(report, usbhid->outbuf);
 	usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 	usbhid->urbout->dev = hid_to_usb_dev(hid);
+	memcpy(usbhid->outbuf, raw_report, usbhid->urbout->transfer_buffer_length);
+	kfree(raw_report);
 
 	dbg_hid("submitting out urb\n");
 
@@ -262,17 +265,20 @@
 {
 	struct hid_report *report;
 	unsigned char dir;
+	char *raw_report;
 	int len;
 	struct usbhid_device *usbhid = hid->driver_data;
 
 	report = usbhid->ctrl[usbhid->ctrltail].report;
+	raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report;
 	dir = usbhid->ctrl[usbhid->ctrltail].dir;
 
 	len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 	if (dir == USB_DIR_OUT) {
-		hid_output_report(report, usbhid->ctrlbuf);
 		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
 		usbhid->urbctrl->transfer_buffer_length = len;
+		memcpy(usbhid->ctrlbuf, raw_report, len);
+		kfree(raw_report);
 	} else {
 		int maxpacket, padlen;
 
@@ -408,6 +414,7 @@
 	int head;
 	unsigned long flags;
 	struct usbhid_device *usbhid = hid->driver_data;
+	int len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 
 	if ((hid->quirks & HID_QUIRK_NOGET) && dir == USB_DIR_IN)
 		return;
@@ -422,7 +429,14 @@
 			return;
 		}
 
-		usbhid->out[usbhid->outhead] = report;
+		usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		if (!usbhid->out[usbhid->outhead].raw_report) {
+			spin_unlock_irqrestore(&usbhid->outlock, flags);
+			warn("output queueing failed");
+			return;
+		}
+		hid_output_report(report, usbhid->out[usbhid->outhead].raw_report);
+		usbhid->out[usbhid->outhead].report = report;
 		usbhid->outhead = head;
 
 		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -441,6 +455,15 @@
 		return;
 	}
 
+	if (dir == USB_DIR_OUT) {
+		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
+			spin_unlock_irqrestore(&usbhid->ctrllock, flags);
+			warn("control queueing failed");
+			return;
+		}
+		hid_output_report(report, usbhid->ctrl[usbhid->ctrlhead].raw_report);
+	}
 	usbhid->ctrl[usbhid->ctrlhead].report = report;
 	usbhid->ctrl[usbhid->ctrlhead].dir = dir;
 	usbhid->ctrlhead = head;
--- linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-pidff.c.orig	2008-04-17 05:49:44.000000000 +0300
+++ linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-pidff.c	2008-06-29 04:06:50.000000000 +0300
@@ -397,7 +397,6 @@
 			  effect->u.condition[i].left_saturation);
 		pidff_set(&pidff->set_condition[PID_DEAD_BAND],
 			  effect->u.condition[i].deadband);
-		usbhid_wait_io(pidff->hid);
 		usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_CONDITION],
 				  USB_DIR_OUT);
 	}
@@ -512,7 +511,6 @@
 		pidff->effect_operation[PID_LOOP_COUNT].value[0] = n;
 	}
 
-	usbhid_wait_io(pidff->hid);
 	usbhid_submit_report(pidff->hid, pidff->reports[PID_EFFECT_OPERATION],
 			  USB_DIR_OUT);
 }
@@ -548,6 +546,9 @@
 	int pid_id = pidff->pid_id[effect_id];
 
 	debug("starting to erase %d/%d", effect_id, pidff->pid_id[effect_id]);
+	/* Wait for the queue to clear. We do not want a full fifo to
+	   prevent the effect removal. */
+	usbhid_wait_io(pidff->hid);
 	pidff_playback_pid(pidff, pid_id, 0);
 	pidff_erase_pid(pidff, pid_id);
 



-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Anssi Hannula wrote:
> Anssi Hannula wrote:
>   
>> Dmitry Torokhov wrote:
>>   
>>     
>>> On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
>>>     
>>>       
>>>> (Added Jiri Kosina due to the hid problem I describe near the end)
>>>>
>>>> Dmitry Torokhov wrote:
>>>>       
>>>>         
>>>>> Hi Anssi,
>>>>>
>>>>> On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
>>>>>         
>>>>>           
>>>>>> Hi all!
>>>>>>
>>>>>> It seems a new spinlock input_dev->event_lock has been added [1] to the
>>>>>> input subsystem since the force feedback support was reworked.
>>>>>>
>>>>>> However, the force feedback subsystem sleeps on events in multiple
>>>>>> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid
>>>>>> io (otherwise commands were lost, IIRC; if necessary I'll test again).
>>>>>>
>>>>>> ff_device->mutex is used to shield effects[], so it is locked when
>>>>>> handling EV_FF events, on flushes, and on effect upload and erase ioctls.
>>>>>>
>>>>>> Maybe we should make EV_FF handling atomic? For effect uploading we
>>>>>> could either make it completely atomic, or lock only for reserving the
>>>>>> effect slot, then release the lock, and mark it as ready after upload is
>>>>>> complete.
>>>>>> Making even the upload completely atomic would mean that no force
>>>>>> feedback events/ioctl() would sleep, which AFAIK would be a plus for
>>>>>> userspace ff applications. On the other hand, hid-pidff (device managed
>>>>>> mode) driver doesn't know whether effect upload was successful until it
>>>>>> has received a report from the device, so it wouldn't be able to report
>>>>>> failure immediately. Other drivers would, though.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>>           
>>>>>>             
>>>>> I think something the patch below is what is needed. EV_FF handling is
>>>>> already atomic because of event_lock (and it is here to stay), but
>>>>> uploading does not need to be atomic, only installing into effect
>>>>> table needs the lock. Any change you could test the patch? I dont have
>>>>> any FF devices.
>>>>>         
>>>>>           
>>>> It seems to be ok, but not enough. The hid-pidff.c driver also waits on
>>>> pidff_playback_pid(). However, I now see that the wait is probably only
>>>> necessary because just the report pointer is passed to
>>>> usbhid_submit_report(). But fixing it properly seems non-trivial (to me).
>>>>
>>>> E.g. the problem sequence is:
>>>>
>>>> - playback_pid() gets called to stop effect 1.
>>>> - it sets control_report->field[X]->value[X] = 1;
>>>> - it submits control_report
>>>> - thus usbhid_submit_report() stores a pointer to the report
>>>> - playback_pid() gets immediately called again for effect 2.
>>>> - it sets control_report->field[X]->value[X] = 2;
>>>> - thus the previous report hasn't yet been submitted, but the report
>>>> content has already changed, thus effect 1 is never stopped.
>>>>
>>>> Any idea how this should be solved properly?
>>>>
>>>>       
>>>>         
>>> It looks like there is a common issue with HID FF devices. Pid driver
>>> tries to handle it by inserting waits till the control queue is
>>> cleared, other drivers are completely ignorant of this problem...
>>> I guess we need to implement a queue of events to be played and put it
>>> in hid-ff.c so it is available for all hid ff drivers.
>>>     
>>>       
>> With other HID FF drivers than hid-pidff we actually want to skip to the
>> last one, though (the report contains complete device state, so skipping
>> old ones does not matter). But indeed of course we still shouldn't be
>> modifying the just-submitted reports since hid_output_report() could be
>> reading them at the same time.
>>
>>   
>>     
> I tried to come up with something today.
>
> Since the only place where the integrity of all reports (i.e. not just
> the last one) is critical is hid-pidff, I implemented report submission
> in workqueue for that. It seems to work.
>
> As was said, though, there are conditions in other places which could
> cause corrupted reports to be sent to devices. However, AFAICS it
> doesn't seem to be a common issue with just HID FF devices, but all code
> using usbhid_submit_report() with USB_DIR_OUT. Therefore I'm not sure
> if any queue implementation in hid-ff.c would be the correct fix, but
> instead it should be fixed so that non-FF users (leds) would be fixed
> too. To achieve that, I modified usbhid_submit_report() to copy the
> report contents before returning. This seems to work as well. I added
> usbhid_wait_io() to effect removal in hid-pidff to prevent an event
> flood from preventing the removal.
>
> Note that even without a fix, any invalid reports would immediately
> be resent with correct content (as any code modifying a report calls
> usbhid_submit_report() again).
>
> That said, I do prefer the usbhid_submit_report() change instead of
> the workqueue one.
> WDYT? Or do you have a better way in mind?
>
> Below is the queue-raw-reports solution. I can post the alternative
> hid-pidff-workqueue solution as well if you want me to.
>   
Ping? Please comment :) I'd really much like the hid-pidff driver
to not panic when used.

For the record, here is a workqueue solution. It is much less
intrusive than the previous patch, but IMO less correct.

Signed-off-by: Anssi Hannula <anssi.hannula@gmail.com>

---

--- linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-pidff.c.orig	2008-04-17 05:49:44.000000000 +0300
+++ linux-2.6.26-0.rc5.1mnb-fftest/drivers/hid/usbhid/hid-pidff.c	2008-06-29 01:45:32.000000000 +0300
@@ -20,6 +20,7 @@
 
 #include <linux/input.h>
 #include <linux/usb.h>
+#include <linux/workqueue.h>
 
 #include <linux/hid.h>
 
@@ -155,6 +156,7 @@
 struct pidff_device {
 	struct hid_device *hid;
 
+	struct workqueue_struct *report_queue;
 	struct hid_report *reports[sizeof(pidff_reports)];
 
 	struct pidff_usage set_effect[sizeof(pidff_set_effect)];
@@ -197,6 +199,62 @@
 	int pid_id[PID_EFFECTS_MAX];
 };
 
+struct pidff_queued_report {
+	struct work_struct work;
+	struct hid_report report;
+};
+
+static void pidff_submit_queued_report(struct work_struct *work)
+{
+	unsigned i;
+	struct pidff_queued_report *qreport = (struct pidff_queued_report *) work;
+	struct hid_report report = qreport->report;
+	usbhid_submit_report(report.device, &report, USB_DIR_OUT);
+	usbhid_wait_io(report.device);
+	for (i = 0; i < report.maxfield; i++)
+		kfree(report.field[i]);
+	kfree(qreport);
+}
+
+static void pidff_queue_report(struct pidff_device *pidff,
+			       struct hid_report *report)
+{
+	struct pidff_queued_report *qreport;
+	unsigned i;
+	
+	qreport = kzalloc(sizeof(*qreport), GFP_ATOMIC);
+	if (!qreport) {
+		printk(KERN_ERR "hid-piff: "
+			"not enough free pages for atomic report copy");
+		return;
+	}
+	qreport->report = *report;
+	for (i = 0; i < report->maxfield; i++) {
+		struct hid_field *field;
+		/* See hid_register_field() in hid-core.c */
+		size_t field_alloc_size = sizeof(struct hid_field)
+			+ report->field[i]->maxusage * sizeof(struct hid_usage)
+			+ report->field[i]->report_count * sizeof(unsigned);
+		field = kzalloc(field_alloc_size, GFP_ATOMIC);
+		if (!field) {
+			printk(KERN_ERR "hid-pidff: "
+				"not enough free pages for atomic field copies");
+			goto fail;
+		}
+		memcpy(field, report->field[i], field_alloc_size);
+		qreport->report.field[i] = field;
+	}
+
+	INIT_WORK(&qreport->work, pidff_submit_queued_report);
+	queue_work(pidff->report_queue, &qreport->work);
+	return;
+
+ fail:
+	while (i-- > 0)
+		 kfree(qreport->report.field[i]);
+	kfree(qreport);
+}
+
 /*
  * Scale an unsigned value with range 0..max for the given field
  */
@@ -261,8 +319,7 @@
 	debug("attack %u => %d", envelope->attack_level,
 	      pidff->set_envelope[PID_ATTACK_LEVEL].value[0]);
 
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_ENVELOPE],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_SET_ENVELOPE]);
 }
 
 /*
@@ -288,8 +345,7 @@
 	pidff_set_signed(&pidff->set_constant[PID_MAGNITUDE],
 			 effect->u.constant.level);
 
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_CONSTANT],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_SET_CONSTANT]);
 }
 
 /*
@@ -323,8 +379,7 @@
 				pidff->effect_direction);
 	pidff->set_effect[PID_START_DELAY].value[0] = effect->replay.delay;
 
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_EFFECT],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_SET_EFFECT]);
 }
 
 /*
@@ -355,9 +410,7 @@
 	pidff_set(&pidff->set_periodic[PID_PHASE], effect->u.periodic.phase);
 	pidff->set_periodic[PID_PERIOD].value[0] = effect->u.periodic.period;
 
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_PERIODIC],
-			  USB_DIR_OUT);
-
+	pidff_queue_report(pidff, pidff->reports[PID_SET_PERIODIC]);
 }
 
 /*
@@ -397,9 +450,7 @@
 			  effect->u.condition[i].left_saturation);
 		pidff_set(&pidff->set_condition[PID_DEAD_BAND],
 			  effect->u.condition[i].deadband);
-		usbhid_wait_io(pidff->hid);
-		usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_CONDITION],
-				  USB_DIR_OUT);
+		pidff_queue_report(pidff, pidff->reports[PID_SET_CONDITION]);
 	}
 }
 
@@ -439,8 +490,7 @@
 			 effect->u.ramp.start_level);
 	pidff_set_signed(&pidff->set_ramp[PID_RAMP_END],
 			 effect->u.ramp.end_level);
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_RAMP],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_SET_RAMP]);
 }
 
 /*
@@ -512,9 +562,7 @@
 		pidff->effect_operation[PID_LOOP_COUNT].value[0] = n;
 	}
 
-	usbhid_wait_io(pidff->hid);
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_EFFECT_OPERATION],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_EFFECT_OPERATION]);
 }
 
 /**
@@ -535,8 +583,7 @@
 static void pidff_erase_pid(struct pidff_device *pidff, int pid_id)
 {
 	pidff->block_free[PID_EFFECT_BLOCK_INDEX].value[0] = pid_id;
-	usbhid_submit_report(pidff->hid, pidff->reports[PID_BLOCK_FREE],
-			  USB_DIR_OUT);
+	pidff_queue_report(pidff, pidff->reports[PID_BLOCK_FREE]);
 }
 
 /*
@@ -550,6 +597,9 @@
 	debug("starting to erase %d/%d", effect_id, pidff->pid_id[effect_id]);
 	pidff_playback_pid(pidff, pid_id, 0);
 	pidff_erase_pid(pidff, pid_id);
+	/* wait until the effect has been erased to make sure the freed device
+	   memory is available for a possible immediate effect upload */
+	flush_workqueue(pidff->report_queue);
 
 	return 0;
 }
@@ -1234,6 +1284,12 @@
 
 }
 
+static void pidff_destroy(struct ff_device *ff)
+{
+	struct pidff_device *pidff = ff->private;
+	destroy_workqueue(pidff->report_queue);
+}
+
 /*
  * Check if the device is PID and initialize it
  */
@@ -1266,12 +1322,18 @@
 	if (!pidff_reports_ok(pidff)) {
 		debug("reports not ok, aborting");
 		error = -ENODEV;
-		goto fail;
+		goto fail1;
 	}
 
 	error = pidff_init_fields(pidff, dev);
 	if (error)
-		goto fail;
+		goto fail1;
+
+	pidff->report_queue = create_singlethread_workqueue("hid-pidff");
+	if (!pidff->report_queue) {
+		error = -ENOMEM;
+		goto fail1;
+	}
 
 	pidff_reset(pidff);
 
@@ -1283,7 +1345,7 @@
 
 	error = pidff_check_autocenter(pidff, dev);
 	if (error)
-		goto fail;
+		goto fail2;
 
 	max_effects =
 	    pidff->block_load[PID_EFFECT_BLOCK_INDEX].field->logical_maximum -
@@ -1306,12 +1368,12 @@
 	    pidff->pool[PID_DEVICE_MANAGED_POOL].value[0] == 0) {
 		printk(KERN_NOTICE "hid-pidff: "
 		       "device does not support device managed pool\n");
-		goto fail;
+		goto fail2;
 	}
 
 	error = input_ff_create(dev, max_effects);
 	if (error)
-		goto fail;
+		goto fail2;
 
 	ff = dev->ff;
 	ff->private = pidff;
@@ -1320,13 +1382,16 @@
 	ff->set_gain = pidff_set_gain;
 	ff->set_autocenter = pidff_set_autocenter;
 	ff->playback = pidff_playback;
+	ff->destroy = pidff_destroy;
 
 	printk(KERN_INFO "Force feedback for USB HID PID devices by "
 	       "Anssi Hannula <anssi.hannula@gmail.com>\n");
 
 	return 0;
 
- fail:
+ fail2:
+	destroy_workqueue(pidff->report_queue);
+ fail1:
 	kfree(pidff);
 	return error;
 }



-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Dmitry Torokhov]

On Sun, Jul 20, 2008 at 05:10:21PM +0300, Anssi Hannula wrote:
> Ping? Please comment :) I'd really much like the hid-pidff driver
> to not panic when used.
> 
> For the record, here is a workqueue solution. It is much less
> intrusive than the previous patch, but IMO less correct.
> 

I was deferring the final judgement to Jiri but I like the generic
solution with com plete copy of the report best of all. I also wonder if
we could pre-allocate the buffer for the report queue so we dont need to
allocate it separately for each request.

-- 
Dmitry

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Dmitry Torokhov wrote:
> On Sun, Jul 20, 2008 at 05:10:21PM +0300, Anssi Hannula wrote:
>> Ping? Please comment :) I'd really much like the hid-pidff driver
>> to not panic when used.
>>
>> For the record, here is a workqueue solution. It is much less
>> intrusive than the previous patch, but IMO less correct.
>>
> 
> I was deferring the final judgement to Jiri but I like the generic
> solution with com plete copy of the report best of all. I also wonder if
> we could pre-allocate the buffer for the report queue so we dont need to
> allocate it separately for each request.

It would take (biggest report of the hid device)*64 bytes for output
fifo and (biggest report of the hid device)*256 bytes for control fifo.
Maximum report size is 4096 bytes, though I guess most devices' biggest
report is far shorter than that.

I'll leave it up to you people. I'll be away (military service) for the
next 2 weeks, but after that I can do some quick testing on any
suggested patch on my next short vacation.

-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Jiri Kosina]

On Sun, 20 Jul 2008, Anssi Hannula wrote:

> Ping? Please comment :) I'd really much like the hid-pidff driver
> to not panic when used.

Hi,

sorry for not commeting on this for quite some time, I have been quite 
loaded during past few days.

> For the record, here is a workqueue solution. It is much less
> intrusive than the previous patch, but IMO less correct.

I like the previous generic approach more. It looks correct to me, but you 
marked the patch as untested when you were sending it. Is this still the 
case?

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Jiri Kosina wrote:
> On Sun, 20 Jul 2008, Anssi Hannula wrote:
> 
>> Ping? Please comment :) I'd really much like the hid-pidff driver
>> to not panic when used.
> 
> Hi,

Hi!

> sorry for not commeting on this for quite some time, I have been quite
> loaded during past few days.

Sorry for replying so late, I haven't had too much free time during the
last few months.

>> For the record, here is a workqueue solution. It is much less
>> intrusive than the previous patch, but IMO less correct.
> 
> I like the previous generic approach more. It looks correct to me, but
> you marked the patch as untested when you were sending it. Is this still
> the case?

I've done some more testing and it seems to be working fine.

> Thanks a lot,
> 


-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Jiri Kosina]

On Sat, 20 Sep 2008, Anssi Hannula wrote:

> > I like the previous generic approach more. It looks correct to me, but 
> > you marked the patch as untested when you were sending it. Is this 
> > still the case?
> I've done some more testing and it seems to be working fine.

Great, thanks! Could you please rebase the patch against current state of 
my HID tree (as in linux-next, -mm, or just the 'mm' branch of hid.git on 
kernel.org), and send it to me, I will merge it right away?

There were some changes in the code due to hidbus getting finally merged, 
but it should be rather trivial to solve.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Jiri Kosina wrote:
> On Sat, 20 Sep 2008, Anssi Hannula wrote:
> 
>>> I like the previous generic approach more. It looks correct to me, but 
>>> you marked the patch as untested when you were sending it. Is this 
>>> still the case?
>> I've done some more testing and it seems to be working fine.
> 
> Great, thanks! Could you please rebase the patch against current state of 
> my HID tree (as in linux-next, -mm, or just the 'mm' branch of hid.git on 
> kernel.org), and send it to me, I will merge it right away?
> 
> There were some changes in the code due to hidbus getting finally merged, 
> but it should be rather trivial to solve.

AFAICS no changes are needed, it applies to hid.git#mm fine.

-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Jiri Kosina]

On Sat, 4 Oct 2008, Anssi Hannula wrote:

> > There were some changes in the code due to hidbus getting finally 
> > merged, but it should be rather trivial to solve.
> AFAICS no changes are needed, it applies to hid.git#mm fine.

You are right.

Could you please send me the patch with the changelog? I will then merge 
it for 2.6.28.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula]

Jiri Kosina wrote:
> On Sat, 4 Oct 2008, Anssi Hannula wrote:
>
>   
>>> There were some changes in the code due to hidbus getting finally 
>>> merged, but it should be rather trivial to solve.
>>>       
>> AFAICS no changes are needed, it applies to hid.git#mm fine.
>>     
>
> You are right.
>
> Could you please send me the patch with the changelog? I will then merge 
> it for 2.6.28.
>
> Thanks,
>
>   


From: Anssi Hannula <anssi.hannula@gmail.com>
Subject: HID: fix a lockup regression when using force feedback on a PID device

Commit 8006479c9b75fb6594a7b746af3d7f1fbb68f18f introduced a spinlock in
input_dev->event_lock, which is locked when handling input events.
However, the hid-pidff driver sleeps when handling events as it waits for
reports being sent to the device before changing the report contents
again.
This causes a system lockup when trying to use force feedback with a PID
device, a regression introduced in 2.6.24 and 2.6.23.15.

Fix it by extracting the raw report data from struct hid_report
immediately when hid_submit_report() is called, therefore allowing
drivers to change the contents of struct hid_report immediately without
affecting the already-queued transfer.

In hid-pidff, re-add the removed usbhid_wait_io() to
pidff_erase_effect() instead, to prevent a full report queue from causing
the submission to fail, thus not freeing up device memory.
pidff_erase_effect() is not called while dev->event_lock is held.

Signed-off-by: Anssi Hannula <anssi.hannula@gmail.com>

---

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 07840df..1ae047c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -232,13 +232,16 @@ static void hid_irq_in(struct urb *urb)
 static int hid_submit_out(struct hid_device *hid)
 {
 	struct hid_report *report;
+	char *raw_report;
 	struct usbhid_device *usbhid = hid->driver_data;
 
-	report = usbhid->out[usbhid->outtail];
+	report = usbhid->out[usbhid->outtail].report;
+	raw_report = usbhid->out[usbhid->outtail].raw_report;
 
-	hid_output_report(report, usbhid->outbuf);
 	usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 	usbhid->urbout->dev = hid_to_usb_dev(hid);
+	memcpy(usbhid->outbuf, raw_report, usbhid->urbout->transfer_buffer_length);
+	kfree(raw_report);
 
 	dbg_hid("submitting out urb\n");
 
@@ -254,17 +257,20 @@ static int hid_submit_ctrl(struct hid_device *hid)
 {
 	struct hid_report *report;
 	unsigned char dir;
+	char *raw_report;
 	int len;
 	struct usbhid_device *usbhid = hid->driver_data;
 
 	report = usbhid->ctrl[usbhid->ctrltail].report;
+	raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report;
 	dir = usbhid->ctrl[usbhid->ctrltail].dir;
 
 	len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 	if (dir == USB_DIR_OUT) {
-		hid_output_report(report, usbhid->ctrlbuf);
 		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
 		usbhid->urbctrl->transfer_buffer_length = len;
+		memcpy(usbhid->ctrlbuf, raw_report, len);
+		kfree(raw_report);
 	} else {
 		int maxpacket, padlen;
 
@@ -401,6 +407,7 @@ void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, uns
 	int head;
 	unsigned long flags;
 	struct usbhid_device *usbhid = hid->driver_data;
+	int len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
 
 	if ((hid->quirks & HID_QUIRK_NOGET) && dir == USB_DIR_IN)
 		return;
@@ -415,7 +422,14 @@ void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, uns
 			return;
 		}
 
-		usbhid->out[usbhid->outhead] = report;
+		usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		if (!usbhid->out[usbhid->outhead].raw_report) {
+			spin_unlock_irqrestore(&usbhid->outlock, flags);
+			warn("output queueing failed");
+			return;
+		}
+		hid_output_report(report, usbhid->out[usbhid->outhead].raw_report);
+		usbhid->out[usbhid->outhead].report = report;
 		usbhid->outhead = head;
 
 		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -434,6 +448,15 @@ void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, uns
 		return;
 	}
 
+	if (dir == USB_DIR_OUT) {
+		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
+			spin_unlock_irqrestore(&usbhid->ctrllock, flags);
+			warn("control queueing failed");
+			return;
+		}
+		hid_output_report(report, usbhid->ctrl[usbhid->ctrlhead].raw_report);
+	}
 	usbhid->ctrl[usbhid->ctrlhead].report = report;
 	usbhid->ctrl[usbhid->ctrlhead].dir = dir;
 	usbhid->ctrlhead = head;
diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 0113261..484e3ee 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -397,7 +397,6 @@ static void pidff_set_condition_report(struct pidff_device *pidff,
 			  effect->u.condition[i].left_saturation);
 		pidff_set(&pidff->set_condition[PID_DEAD_BAND],
 			  effect->u.condition[i].deadband);
-		usbhid_wait_io(pidff->hid);
 		usbhid_submit_report(pidff->hid, pidff->reports[PID_SET_CONDITION],
 				  USB_DIR_OUT);
 	}
@@ -512,7 +511,6 @@ static void pidff_playback_pid(struct pidff_device *pidff, int pid_id, int n)
 		pidff->effect_operation[PID_LOOP_COUNT].value[0] = n;
 	}
 
-	usbhid_wait_io(pidff->hid);
 	usbhid_submit_report(pidff->hid, pidff->reports[PID_EFFECT_OPERATION],
 			  USB_DIR_OUT);
 }
@@ -548,6 +546,9 @@ static int pidff_erase_effect(struct input_dev *dev, int effect_id)
 	int pid_id = pidff->pid_id[effect_id];
 
 	debug("starting to erase %d/%d", effect_id, pidff->pid_id[effect_id]);
+	/* Wait for the queue to clear. We do not want a full fifo to
+	   prevent the effect removal. */
+	usbhid_wait_io(pidff->hid);
 	pidff_playback_pid(pidff, pid_id, 0);
 	pidff_erase_pid(pidff, pid_id);
 
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index b47f991..abedb13 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -67,7 +67,7 @@ struct usbhid_device {
 	spinlock_t ctrllock;                                            /* Control fifo spinlock */
 
 	struct urb *urbout;                                             /* Output URB */
-	struct hid_report *out[HID_CONTROL_FIFO_SIZE];                  /* Output pipe fifo */
+	struct hid_output_fifo out[HID_CONTROL_FIFO_SIZE];              /* Output pipe fifo */
 	unsigned char outhead, outtail;                                 /* Output pipe fifo head & tail */
 	char *outbuf;                                                   /* Output buffer */
 	dma_addr_t outbuf_dma;                                          /* Output buffer dma */
diff --git a/include/linux/hid.h b/include/linux/hid.h
index eb1844c..3e2726a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -388,6 +388,12 @@ struct hid_report_enum {
 struct hid_control_fifo {
 	unsigned char dir;
 	struct hid_report *report;
+	char *raw_report;
+};
+
+struct hid_output_fifo {
+	struct hid_report *report;
+	char *raw_report;
 };
 
 #define HID_CLAIMED_INPUT	1



-- 
Anssi Hannula

Re: Sleeping inside spinlock in force feedback input event code

[Jiri Kosina]

On Sat, 4 Oct 2008, Anssi Hannula wrote:

> From: Anssi Hannula <anssi.hannula@gmail.com>
> Subject: HID: fix a lockup regression when using force feedback on a PID device

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs