[PATCH] Revert "HID: sony: Add force feedback support for Dualshock3 USB"

[PATCH] Revert "HID: sony: Add force feedback support for Dualshock3 USB"

[Sven Eckelmann]

The ff_memless has a timer running which gets run in an atomic context and
calls the play_effect callback. The callback function for sony uses the
hid_output_raw_report (overwritten by sixaxis_usb_output_raw_report) function
to handle differences in the control message format. It is not safe for an
atomic context because it may sleep later in usb_start_wait_urb.

[  619.529530] BUG: scheduling while atomic: swapper/5/0/0x00000100
[...]
[  619.529707] Call Trace:
[  619.529710]  <IRQ>  [<ffffffff81474a72>] ? dump_stack+0x41/0x51
[  619.529723]  [<ffffffff814723d8>] ? __schedule_bug+0x44/0x51
[  619.529731]  [<ffffffff814782fa>] ? __schedule+0x53a/0x720
[  619.529739]  [<ffffffff81476669>] ? schedule_timeout+0x1c9/0x2d0
[  619.529756]  [<ffffffffa00c0c6a>] ? ohci_urb_enqueue+0x1ea/0xa40 [ohci_hcd]
[  619.529763]  [<ffffffff810494ba>] ? gart_map_page+0x5a/0x80
[  619.529786]  [<ffffffffa000f7b5>] ? usb_hcd_map_urb_for_dma+0x425/0x4b0 [usbcore]
[  619.529794]  [<ffffffff81478c91>] ? wait_for_completion_timeout+0xa1/0x120
[  619.529801]  [<ffffffff81086090>] ? wake_up_state+0x10/0x10
[  619.529825]  [<ffffffffa0011d0d>] ? usb_start_wait_urb+0x6d/0xd0 [usbcore]
[  619.529848]  [<ffffffffa0011e2d>] ? usb_control_msg+0xbd/0x100 [usbcore]
[  619.529856]  [<ffffffffa1026620>] ? sony_play_effect+0x170/0x180 [hid_sony]
[  619.529864]  [<ffffffffa101c593>] ? ml_play_effects+0xf3/0x610 [ff_memless]
[  619.529872]  [<ffffffffa101cb90>] ? ml_ff_playback+0xa0/0xa0 [ff_memless]
[  619.529880]  [<ffffffffa101cbb9>] ? ml_effect_timer+0x29/0x470 [ff_memless]
[  619.529888]  [<ffffffff81063a51>] ? call_timer_fn+0x31/0x100
[  619.529895]  [<ffffffffa101cb90>] ? ml_ff_playback+0xa0/0xa0 [ff_memless]
[  619.529903]  [<ffffffff81064549>] ? run_timer_softirq+0x1c9/0x2b0
[  619.529910]  [<ffffffff8105d4e6>] ? __do_softirq+0xf6/0x240
[  619.529917]  [<ffffffff8148369c>] ? call_softirq+0x1c/0x30
[  619.529924]  [<ffffffff810143a5>] ? do_softirq+0x55/0x90
[  619.529930]  [<ffffffff8105d785>] ? irq_exit+0x95/0xa0
[  619.529937]  [<ffffffff81013ed9>] ? do_IRQ+0x49/0xb0
[  619.529944]  [<ffffffff8147a4ad>] ? common_interrupt+0x6d/0x6d
[  619.529947]  <EOI>  [<ffffffff810a2779>] ? ktime_get+0x39/0xc0
[  619.529959]  [<ffffffff810472a2>] ? native_safe_halt+0x2/0x10
[  619.529966]  [<ffffffff8101a349>] ? default_idle+0x19/0xa0
[  619.529973]  [<ffffffff8101a43b>] ? amd_e400_idle+0x6b/0xf0
[  619.529980]  [<ffffffff810a0ae8>] ? cpu_startup_entry+0xc8/0x270
[  619.529987]  [<ffffffff8103b962>] ? start_secondary+0x1d2/0x230

This reverts commit a08c22c0df0ad23d0df10ae1a9df26643589b3cc.

Reported-by: simon@mungewell.org
Signed-of-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/hid/Kconfig    |  8 --------
 drivers/hid/hid-sony.c | 52 --------------------------------------------------
 2 files changed, 60 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 329fbb9..a27e531 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -618,14 +618,6 @@ config HID_SONY
 	  * Sony PS3 Blue-ray Disk Remote Control (Bluetooth)
 	  * Logitech Harmony adapter for Sony Playstation 3 (Bluetooth)
 
-config SONY_FF
-	bool "Sony PS2/3 accessories force feedback support"
-	depends on HID_SONY
-	select INPUT_FF_MEMLESS
-	---help---
-	Say Y here if you have a Sony PS2/3 accessory and want to enable force
-	feedback support for it.
-
 config HID_SPEEDLINK
 	tristate "Speedlink VAD Cezanne mouse support"
 	depends on HID
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index da551d1..bc37a18 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -614,54 +614,6 @@ static void buzz_remove(struct hid_device *hdev)
 	drv_data->extra = NULL;
 }
 
-#ifdef CONFIG_SONY_FF
-static int sony_play_effect(struct input_dev *dev, void *data,
-			    struct ff_effect *effect)
-{
-	unsigned char buf[] = {
-		0x01,
-		0x00, 0xff, 0x00, 0xff, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x03,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0xff, 0x27, 0x10, 0x00, 0x32,
-		0x00, 0x00, 0x00, 0x00, 0x00
-	};
-	__u8 left;
-	__u8 right;
-	struct hid_device *hid = input_get_drvdata(dev);
-
-	if (effect->type != FF_RUMBLE)
-		return 0;
-
-	left = effect->u.rumble.strong_magnitude / 256;
-	right = effect->u.rumble.weak_magnitude ? 1 : 0;
-
-	buf[3] = right;
-	buf[5] = left;
-
-	return hid->hid_output_raw_report(hid, buf, sizeof(buf),
-					  HID_OUTPUT_REPORT);
-}
-
-static int sony_init_ff(struct hid_device *hdev)
-{
-	struct hid_input *hidinput = list_entry(hdev->inputs.next,
-						struct hid_input, list);
-	struct input_dev *input_dev = hidinput->input;
-
-	input_set_capability(input_dev, EV_FF, FF_RUMBLE);
-	return input_ff_create_memless(input_dev, NULL, sony_play_effect);
-}
-
-#else
-static int sony_init_ff(struct hid_device *hdev)
-{
-	return 0;
-}
-#endif
-
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret;
@@ -711,10 +663,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret < 0)
 		goto err_stop;
 
-	ret = sony_init_ff(hdev);
-	if (ret < 0)
-		goto err_stop;
-
 	return 0;
 err_stop:
 	hid_hw_stop(hdev);
-- 
1.8.4.3

[PATCHv2] HID: sony: Send FF commands in non-atomic context

[Sven Eckelmann]

The ff_memless has a timer running which gets run in an atomic context and
calls the play_effect callback. The callback function for sony uses the
hid_output_raw_report (overwritten by sixaxis_usb_output_raw_report) function
to handle differences in the control message format. It is not safe for an
atomic context because it may sleep later in usb_start_wait_urb.

This "scheduling while atomic" can cause the system to lock up. A workaround is
to make the force feedback state update using work_queues and use the
play_effect function only to enqueue the work item.

Reported-by: Simon Wood <simon@mungewell.org>
Reported-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
This patch can replace
 'Revert "HID: sony: Add force feedback support for Dualshock3 USB"'

It doesn't contain the command changes from <2014555.nmU692BQMt@sven-desktop>.
It would be nice when Simon Wood could test it again with his Intec Wired
controller. When it doesn't work:

 * Try to change to usb_interrupt_msg instead of sc->hdev->hid_output_raw_report
 * Send a interrupt message using buf[10] = 0x02
   (only when buf[3] != 0 || buf[5] != 0) followed by a message with
   buf[10] = 0x1e (always)

 drivers/hid/hid-sony.c | 53 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index da551d1..098af2f8 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -225,6 +225,13 @@ static const unsigned int buzz_keymap[] = {
 struct sony_sc {
 	unsigned long quirks;
 
+#ifdef CONFIG_SONY_FF
+	struct work_struct rumble_worker;
+	struct hid_device *hdev;
+	__u8 left;
+	__u8 right;
+#endif
+
 	void *extra;
 };
 
@@ -615,9 +622,9 @@ static void buzz_remove(struct hid_device *hdev)
 }
 
 #ifdef CONFIG_SONY_FF
-static int sony_play_effect(struct input_dev *dev, void *data,
-			    struct ff_effect *effect)
+static void sony_rumble_worker(struct work_struct *work)
 {
+	struct sony_sc *sc = container_of(work, struct sony_sc, rumble_worker);
 	unsigned char buf[] = {
 		0x01,
 		0x00, 0xff, 0x00, 0xff, 0x00,
@@ -628,21 +635,28 @@ static int sony_play_effect(struct input_dev *dev, void *data,
 		0xff, 0x27, 0x10, 0x00, 0x32,
 		0x00, 0x00, 0x00, 0x00, 0x00
 	};
-	__u8 left;
-	__u8 right;
+
+	buf[3] = sc->right;
+	buf[5] = sc->left;
+
+	sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
+					HID_OUTPUT_REPORT);
+}
+
+static int sony_play_effect(struct input_dev *dev, void *data,
+			    struct ff_effect *effect)
+{
 	struct hid_device *hid = input_get_drvdata(dev);
+	struct sony_sc *sc = hid_get_drvdata(hid);
 
 	if (effect->type != FF_RUMBLE)
 		return 0;
 
-	left = effect->u.rumble.strong_magnitude / 256;
-	right = effect->u.rumble.weak_magnitude ? 1 : 0;
+	sc->left = effect->u.rumble.strong_magnitude / 256;
+	sc->right = effect->u.rumble.weak_magnitude ? 1 : 0;
 
-	buf[3] = right;
-	buf[5] = left;
-
-	return hid->hid_output_raw_report(hid, buf, sizeof(buf),
-					  HID_OUTPUT_REPORT);
+	schedule_work(&sc->rumble_worker);
+	return 0;
 }
 
 static int sony_init_ff(struct hid_device *hdev)
@@ -650,16 +664,31 @@ static int sony_init_ff(struct hid_device *hdev)
 	struct hid_input *hidinput = list_entry(hdev->inputs.next,
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
+	struct sony_sc *sc = hid_get_drvdata(hdev);
+
+	sc->hdev = hdev;
+	INIT_WORK(&sc->rumble_worker, sony_rumble_worker);
 
 	input_set_capability(input_dev, EV_FF, FF_RUMBLE);
 	return input_ff_create_memless(input_dev, NULL, sony_play_effect);
 }
 
+static void sony_destroy_ff(struct hid_device *hdev)
+{
+	struct sony_sc *sc = hid_get_drvdata(hdev);
+
+	cancel_work_sync(&sc->rumble_worker);
+}
+
 #else
 static int sony_init_ff(struct hid_device *hdev)
 {
 	return 0;
 }
+
+static void sony_destroy_ff(struct hid_device *hdev)
+{
+}
 #endif
 
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
@@ -728,6 +757,8 @@ static void sony_remove(struct hid_device *hdev)
 	if (sc->quirks & BUZZ_CONTROLLER)
 		buzz_remove(hdev);
 
+	sony_destroy_ff(hdev);
+
 	hid_hw_stop(hdev);
 }
 
-- 
1.8.4.3

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[simon]

>  drivers/hid/hid-sony.c | 53
> +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 11 deletions(-)

Doesn't this miss patching Kconfig (previous changes were reverted).
Simon

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[Sven Eckelmann]

On Sunday 17 November 2013 14:46:32 simon@mungewell.org wrote:
> >  drivers/hid/hid-sony.c | 53
> > 
> > +++++++++++++++++++++++++++++++++++++++-----------
> > 
> >  1 file changed, 42 insertions(+), 11 deletions(-)
> 
> Doesn't this miss patching Kconfig (previous changes were reverted).
> Simon

This was meant as a "replacement"/v2 of the revert patch as said earlier 
(directly bellow the commit message):

> > This patch can replace
> > 'Revert "HID: sony: Add force feedback support for Dualshock3 USB"'

And if the maintainer doesn't like the work_queue workaround then he can still 
apply the revert patch or discuss/propose a different approach.

Kind regards,
	Sven

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[simon]

> On Sunday 17 November 2013 14:46:32 simon@mungewell.org wrote:
>> >  drivers/hid/hid-sony.c | 53
>> >
>> > +++++++++++++++++++++++++++++++++++++++-----------
>> >
>> >  1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> Doesn't this miss patching Kconfig (previous changes were reverted).
>> Simon
>
> This was meant as a "replacement"/v2 of the revert patch as said earlier
> (directly bellow the commit message):
>
>> > This patch can replace
>> > 'Revert "HID: sony: Add force feedback support for Dualshock3 USB"'
>
> And if the maintainer doesn't like the work_queue workaround then he can
> still
> apply the revert patch or discuss/propose a different approach.

Patching the original Nov9 patch ([PATCH] HID: sony: Add force feedback
support for Dualshock3 USB) and then the 2nd revert ([PATCHv2] HID: sony:
Send FF commands in non-atomic context) works for me.

Both Sony DualShock2/SixAxis and my wired Itec controller work as expected
with 'testrumble' and 'testhaptic'.

Thanks for getting this fixed.
Simon

Signed-off-by: Simon Wood <simon@mungewell.org>

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[Jiri Kosina]

On Sun, 17 Nov 2013, Sven Eckelmann wrote:

> The ff_memless has a timer running which gets run in an atomic context and
> calls the play_effect callback. The callback function for sony uses the
> hid_output_raw_report (overwritten by sixaxis_usb_output_raw_report) function
> to handle differences in the control message format. It is not safe for an
> atomic context because it may sleep later in usb_start_wait_urb.
> 
> This "scheduling while atomic" can cause the system to lock up. A workaround is
> to make the force feedback state update using work_queues and use the
> play_effect function only to enqueue the work item.
> 
> Reported-by: Simon Wood <simon@mungewell.org>
> Reported-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> This patch can replace
>  'Revert "HID: sony: Add force feedback support for Dualshock3 USB"'
> 
> It doesn't contain the command changes from <2014555.nmU692BQMt@sven-desktop>.
> It would be nice when Simon Wood could test it again with his Intec Wired
> controller. When it doesn't work:
> 
>  * Try to change to usb_interrupt_msg instead of sc->hdev->hid_output_raw_report
>  * Send a interrupt message using buf[10] = 0x02
>    (only when buf[3] != 0 || buf[5] != 0) followed by a message with
>    buf[10] = 0x1e (always)

Now applied for 3.13. Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

On Tuesday 19 November 2013 09:38:21 Jiri Kosina wrote:
[...]
> Now applied for 3.13. Thanks.

Ok, but would have been nice when the v3 version have been applied. Now I have 
to resubmit the LED patchset to include the function/variable rename. I will 
do this tonight or tomorrow.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[Jiri Kosina]

On Tue, 19 Nov 2013, Sven Eckelmann wrote:

> > Now applied for 3.13. Thanks.
> 
> Ok, but would have been nice when the v3 version have been applied. 

I apparently got confused by this thread, sorry.

> Now I have to resubmit the LED patchset to include the function/variable 
> rename. I will do this tonight or tomorrow.

That'd be nice, thanks. Also I think really only the first patch (atomic 
context fix) is really applicable for this (3.13) merge window.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCHv2] HID: sony: Send FF commands in non-atomic context

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Tuesday 19 November 2013 11:35:27 Jiri Kosina wrote:
> > Now I have to resubmit the LED patchset to include the function/variable
> > rename. I will do this tonight or tomorrow.
> 
> That'd be nice, thanks. Also I think really only the first patch (atomic
> context fix) is really applicable for this (3.13) merge window.

Yes, only the first one was the fix and only the rest is for later because it 
is a feature.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]