[PATCH v2 24/33] HID: logitech-hidpp: execute G920 effect upload and erase synchronously

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

The input force-feedback subsystem expects upload and erase operations
to execute synchronously in process context, returning errors if the
device rejects the effect or communication fails so that callers can
react accordingly. The G920 driver previously queued these requests onto
an asynchronous workqueue and immediately returned success to the input
core, hiding transfer errors and allowing user space to race ahead
before the hardware effect was actually uploaded or removed.

Convert hidpp_ff_upload_effect() and hidpp_ff_erase_effect() to execute
synchronously using hidpp_send_fap_command_sync(). Before issuing the
hardware command, flush any in-flight playback commands on the workqueue
to ensure ordering. Open and close the underlying transport around the
transfer, update the slot table directly upon completion, and report
errors back to the caller on upload.

Because upload and erase no longer go through the workqueue, drop
effect ID tracking from the asynchronous work item and queue playback,
autocenter, and gain commands with their slot parameters already
populated by the caller.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 126 +++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 66 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 9526286ba406..20f7e8367ee1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2332,8 +2332,6 @@ static void hidpp_touchpad_raw_xy_event(struct hidpp_device *hidpp_dev,
 
 #define HIDPP_FF_EFFECT_AUTOSTART	0x80
 
-#define HIDPP_FF_EFFECTID_NONE		-1
-#define HIDPP_FF_EFFECTID_AUTOCENTER	-2
 #define HIDPP_AUTOCENTER_PARAMS_LENGTH	18
 
 #define HIDPP_FF_MAX_PARAMS	20
@@ -2358,7 +2356,6 @@ struct g920_data {
 struct hidpp_ff_work_data {
 	struct work_struct work;
 	struct hidpp_ff_private_data *data;
-	int effect_id;
 	u8 command;
 	u8 params[HIDPP_FF_MAX_PARAMS];
 	u8 size;
@@ -2407,71 +2404,23 @@ static void hidpp_ff_work_handler(struct work_struct *w)
 	struct hidpp_ff_private_data *data = wd->data;
 	struct g920_data *g920 = data->hidpp->private_data;
 	struct hidpp_report response;
-	u8 slot;
 	int ret;
 
-	/* add slot number if needed */
-	switch (wd->effect_id) {
-	case HIDPP_FF_EFFECTID_AUTOCENTER:
-		wd->params[0] = g920->slot_autocenter;
-		break;
-	case HIDPP_FF_EFFECTID_NONE:
-		/* leave slot as zero */
-		break;
-	default:
-		/* find current slot for effect */
-		wd->params[0] = data->slots[wd->effect_id];
-		break;
-	}
-
 	/* send command and wait for reply */
 	ret = hidpp_send_fap_command_sync(data->hidpp, g920->feature_index,
 					  wd->command, wd->params, wd->size,
 					  &response);
-
-	if (ret) {
+	if (ret)
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
-		goto out;
-	}
-
-	/* parse return data */
-	switch (wd->command) {
-	case HIDPP_FF_DOWNLOAD_EFFECT:
-		slot = response.fap.params[0];
-		if (slot > 0) {
-			if (wd->effect_id >= 0)
-				/* regular effect uploaded */
-				data->slots[wd->effect_id] = slot;
-			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
-				/* autocenter spring uploaded */
-				g920->slot_autocenter = slot;
-		}
-		break;
-	case HIDPP_FF_DESTROY_EFFECT:
-		slot = wd->params[0];
-		if (slot > 0) {
-			if (wd->effect_id >= 0)
-				/* regular effect destroyed */
-				data->slots[wd->effect_id] = 0;
-			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
-				/* autocenter spring destroyed */
-				g920->slot_autocenter = 0;
-		}
-		break;
-	case HIDPP_FF_SET_GLOBAL_GAINS:
+	else if (wd->command == HIDPP_FF_SET_GLOBAL_GAINS)
 		g920->gain = (wd->params[0] << 8) + wd->params[1];
-		break;
-	default:
-		/* no action needed */
-		break;
-	}
 
-out:
 	atomic_dec(&data->workqueue_size);
 	kfree(wd);
 }
 
-static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id, u8 command, u8 *params, u8 size)
+static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, u8 command,
+			       const u8 *params, u8 size)
 {
 	struct hidpp_ff_work_data *wd;
 	int s;
@@ -2483,7 +2432,6 @@ static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id
 	INIT_WORK(&wd->work, hidpp_ff_work_handler);
 
 	wd->data = data;
-	wd->effect_id = effect_id;
 	wd->command = command;
 	wd->size = size;
 	memcpy(wd->params, params, size);
@@ -2501,11 +2449,16 @@ static int hidpp_ff_queue_work(struct hidpp_ff_private_data *data, int effect_id
 static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effect, struct ff_effect *old)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
+	struct hidpp_device *hidpp = data->hidpp;
+	struct hid_device *hid = hidpp->hid_dev;
+	struct g920_data *g920 = hidpp->private_data;
+	struct hidpp_report response;
 	u8 params[20];
 	u8 size;
-	int force;
+	int force, error;
 
 	/* set common parameters */
+	params[0] = old ? data->slots[effect->id] : 0;
 	params[2] = effect->replay.length >> 8;
 	params[3] = effect->replay.length & 255;
 	params[4] = effect->replay.delay >> 8;
@@ -2552,7 +2505,9 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 			params[1] = HIDPP_FF_EFFECT_PERIODIC_TRIANGLE;
 			break;
 		default:
-			hid_err(data->hidpp->hid_dev, "Unexpected periodic waveform type %i!\n", effect->u.periodic.waveform);
+			hid_err(hidpp->hid_dev,
+				"Unexpected periodic waveform type %i!\n",
+				effect->u.periodic.waveform);
 			return -EINVAL;
 		}
 		force = (effect->u.periodic.magnitude * fixp_sin16((effect->direction * 360) >> 16)) >> 15;
@@ -2637,11 +2592,26 @@ static int hidpp_ff_upload_effect(struct input_dev *dev, struct ff_effect *effec
 				effect->u.condition[0].center);
 		break;
 	default:
-		hid_err(data->hidpp->hid_dev, "Unexpected force type %i!\n", effect->type);
+		hid_err(hidpp->hid_dev, "Unexpected force type %i!\n", effect->type);
 		return -EINVAL;
 	}
 
-	return hidpp_ff_queue_work(data, effect->id, HIDPP_FF_DOWNLOAD_EFFECT, params, size);
+	flush_workqueue(data->wq);
+
+	error = hid_hw_open(hid);
+	if (error)
+		return error;
+
+	error = hidpp_send_fap_command_sync(hidpp, g920->feature_index,
+					    HIDPP_FF_DOWNLOAD_EFFECT,
+					    params, size, &response);
+	hid_hw_close(hid);
+
+	if (error)
+		return error;
+
+	data->slots[effect->id] = response.fap.params[0];
+	return 0;
 }
 
 static int hidpp_ff_playback(struct input_dev *dev, int effect_id, int value)
@@ -2649,31 +2619,55 @@ static int hidpp_ff_playback(struct input_dev *dev, int effect_id, int value)
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 params[2];
 
+	params[0] = data->slots[effect_id];
+	if (!dev->ready || !params[0])
+		return 0;
+
 	params[1] = value ? HIDPP_FF_EFFECT_STATE_PLAY : HIDPP_FF_EFFECT_STATE_STOP;
 
-	dbg_hid("St%sing playback of effect %d.\n", value?"art":"opp", effect_id);
+	dbg_hid("St%sing playback of effect %d.\n", value ? "art" : "opp", effect_id);
 
-	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
+	return hidpp_ff_queue_work(data, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
 }
 
 static int hidpp_ff_erase_effect(struct input_dev *dev, int effect_id)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
-	u8 slot = 0;
+	struct hidpp_device *hidpp = data->hidpp;
+	struct hid_device *hid = hidpp->hid_dev;
+	struct g920_data *g920 = hidpp->private_data;
+	struct hidpp_report response;
+	u8 slot;
 
 	dbg_hid("Erasing effect %d.\n", effect_id);
 
-	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_DESTROY_EFFECT, &slot, 1);
+	flush_workqueue(data->wq);
+
+	slot = data->slots[effect_id];
+	data->slots[effect_id] = 0;
+
+	if (slot) {
+		if (hid_hw_open(hid) == 0) {
+			hidpp_send_fap_command_sync(hidpp, g920->feature_index,
+						    HIDPP_FF_DESTROY_EFFECT,
+						    &slot, 1, &response);
+			hid_hw_close(hid);
+		}
+	}
+
+	return 0;
 }
 
 static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
+	struct g920_data *g920 = data->hidpp->private_data;
 	u8 params[HIDPP_AUTOCENTER_PARAMS_LENGTH];
 
 	dbg_hid("Setting autocenter to %d.\n", magnitude);
 
 	/* start a standard spring effect */
+	params[0] = g920->slot_autocenter;
 	params[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART;
 	/* zero delay and duration */
 	params[2] = params[3] = params[4] = params[5] = 0;
@@ -2685,7 +2679,7 @@ static void hidpp_ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 	/* zero deadband and center */
 	params[10] = params[11] = params[12] = params[13] = 0;
 
-	hidpp_ff_queue_work(data, HIDPP_FF_EFFECTID_AUTOCENTER, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
+	hidpp_ff_queue_work(data, HIDPP_FF_DOWNLOAD_EFFECT, params, ARRAY_SIZE(params));
 }
 
 static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
@@ -2700,7 +2694,7 @@ static void hidpp_ff_set_gain(struct input_dev *dev, u16 gain)
 	params[2] = 0; /* no boost */
 	params[3] = 0;
 
-	hidpp_ff_queue_work(data, HIDPP_FF_EFFECTID_NONE, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
+	hidpp_ff_queue_work(data, HIDPP_FF_SET_GLOBAL_GAINS, params, ARRAY_SIZE(params));
 }
 
 static ssize_t hidpp_ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)

-- 
2.56.0.542.g3ef89b7020-goog