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
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 <redacted>
---
drivers/input/ff-core.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
Index: linux/drivers/input/ff-core.c
===================================================================
(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:
quoted
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
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:
quoted
Hi Anssi,
On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
quoted
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
On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
quoted
(Added Jiri Kosina due to the hid problem I describe near the end)
Dmitry Torokhov wrote:
quoted
Hi Anssi,
On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
quoted
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
On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
quoted
(Added Jiri Kosina due to the hid problem I describe near the end)
Dmitry Torokhov wrote:
quoted
Hi Anssi,
On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
quoted
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 <redacted>
---
Not tested extensively yet, just gathering comments.
@@ -548,6 +546,9 @@intpid_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+preventtheeffectremoval.*/+usbhid_wait_io(pidff->hid);pidff_playback_pid(pidff,pid_id,0);pidff_erase_pid(pidff,pid_id);
On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote:
quoted
(Added Jiri Kosina due to the hid problem I describe near the end)
Dmitry Torokhov wrote:
quoted
Hi Anssi,
On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:
quoted
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 <redacted>
---
@@ -197,6 +199,62 @@intpid_id[PID_EFFECTS_MAX];};+structpidff_queued_report{+structwork_structwork;+structhid_reportreport;+};++staticvoidpidff_submit_queued_report(structwork_struct*work)+{+unsignedi;+structpidff_queued_report*qreport=(structpidff_queued_report*)work;+structhid_reportreport=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);+}++staticvoidpidff_queue_report(structpidff_device*pidff,+structhid_report*report)+{+structpidff_queued_report*qreport;+unsignedi;++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++){+structhid_field*field;+/* See hid_register_field() in hid-core.c */+size_tfield_alloc_size=sizeof(structhid_field)++report->field[i]->maxusage*sizeof(structhid_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");+gotofail;+}+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);+}+/**Scaleanunsignedvaluewithrange0..maxforthegivenfield*/
@@ -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+memoryisavailableforapossibleimmediateeffectupload*/+flush_workqueue(pidff->report_queue);return0;}
@@ -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");-gotofail;+gotofail2;}error=input_ff_create(dev,max_effects);if(error)-gotofail;+gotofail2;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");return0;-fail:+fail2:+destroy_workqueue(pidff->report_queue);+fail1:kfree(pidff);returnerror;}
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
On Sun, Jul 20, 2008 at 05:10:21PM +0300, Anssi Hannula wrote:
quoted
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
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
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.
quoted
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.
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
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
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 <redacted>
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 <redacted>
---
@@ -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)intpid_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+preventtheeffectremoval.*/+usbhid_wait_io(pidff->hid);pidff_playback_pid(pidff,pid_id,0);pidff_erase_pid(pidff,pid_id);