Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula <anssi.hannula@gmail.com>]

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