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

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1038 bytes --]

On Sunday 17 November 2013 17:30:51 David Herrmann wrote:
> Yeah, the input-ff callbacks cannot be handled inline. You also get
> deadlocks with the input-spinlock. In the wiimote driver I simply
> dispatch the ff-events to a workqueue. You can have a look at
> drivers/hid/hid-wiimote-modules.c. You can get some ordering-problems
> then, but these can usually be ignored as they just collapse events.
> 
> The related commit was:
> 
> commit f50f9aabf32db7414551ffdfdccc71be5f3d6e7d
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date:   Wed Oct 2 13:47:28 2013 +0200
> 
>     HID: wiimote: fix FF deadlock

Yes, I've played around with the linux kernel usb message api and came to the 
same conclusion (for now). I've only tested it with a small proof of concept 
patch and it didn't hang anymore with testhaptic.

Maybe Simon Wood can test his devices because I am unsure whether PS3 
dualshock clones will work with the interrupt or control urbs. For example the 
script from Simon didn't work at all for me.

Kind regards,
	Sven

[-- Attachment #1.2: proof_of_concept_wq_rumble.patch --]
[-- Type: text/x-patch, Size: 3182 bytes --]

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index da551d1..d509447 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -224,6 +224,10 @@ static const unsigned int buzz_keymap[] = {
 
 struct sony_sc {
 	unsigned long quirks;
+	struct work_struct rumble_worker;
+	struct hid_device *hdev;
+	__u8 left;
+	__u8 right;
 
 	void *extra;
 };
@@ -614,35 +618,53 @@ 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)
+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,
-		0x00, 0x00, 0x00, 0x00, 0x03,
+		0x00, 0x00, 0x00, 0x00, 0x02,
 		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
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00
 	};
-	__u8 left;
-	__u8 right;
+	struct usb_interface *intf = to_usb_interface(sc->hdev->dev.parent);
+	struct usb_device *usbdev = interface_to_usbdev(intf);
+
+	buf[3] = sc->right;
+	buf[5] = sc->left;
+
+	if (sc->right || sc->left)
+		usb_interrupt_msg(usbdev, usb_sndctrlpipe(usbdev, 2), buf,
+				  sizeof(buf), NULL, USB_CTRL_SET_TIMEOUT);
+
+	buf[10] = 0x1e;
+	usb_interrupt_msg(usbdev, usb_sndctrlpipe(usbdev, 2), buf, sizeof(buf),
+			  NULL, USB_CTRL_SET_TIMEOUT);
+}
+
+#ifdef CONFIG_SONY_FF
+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,6 +672,9 @@ 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);
+
+	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);
@@ -711,6 +736,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret < 0)
 		goto err_stop;
 
+	sc->hdev = hdev;
 	ret = sony_init_ff(hdev);
 	if (ret < 0)
 		goto err_stop;
@@ -728,6 +754,8 @@ static void sony_remove(struct hid_device *hdev)
 	if (sc->quirks & BUZZ_CONTROLLER)
 		buzz_remove(hdev);
 
+	cancel_work_sync(&sc->rumble_worker);
+
 	hid_hw_stop(hdev);
 }
 

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