Re: [PATCH 07/12] HID: hid-lg4ff: Protect concurrent access to the output HID report values with a spinlock.

[Jiri Kosina]

On Sat, 21 Mar 2015, Michal Malý wrote:

> Since all functions that need to send some data to the device they
> manage share the same HID report some synchronization is needed to
> prevent sending bogus data to the device.

This patch is doing much more than just adding a mutual exclusion 
mechanisms (for example the wdata indirection), so more descriptive 
changelog is needed.

Looking at the patch it's actually not clear at all what are all the 
things it's trying to do, so please resend v2 of this patch with much more 
verbose changelog, so that I am able to review it properly. Thanks.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 07/12] HID: hid-lg4ff: Protect concurrent access to the output HID report values with a spinlock.

[Michal Malý]

Since all functions that need to send some data to the device they
manage share the same HID report some synchronization is needed to
prevent sending bogus data to the device.

Signed-off-by: Michal Malý 
---
 drivers/hid/hid-lg.c|   4 +-
 drivers/hid/hid-lg4ff.c | 293 ++--
 2 files changed, 213 insertions(+), 84 deletions(-)

diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index c797781..c3981da 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -734,8 +734,8 @@ static void lg_remove(struct hid_device *hdev)
struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
if (drv_data->quirks & LG_FF4)
lg4ff_deinit(hdev);
-
-   hid_hw_stop(hdev);
+   else
+   hid_hw_stop(hdev);
kfree(drv_data);
 }
 
diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index a2f47ee..0bbdeea 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -71,7 +71,7 @@
 static void lg4ff_set_range_dfp(struct hid_device *hid, u16 range);
 static void lg4ff_set_range_g25(struct hid_device *hid, u16 range);
 
-struct lg4ff_device_entry {
+struct lg4ff_wheel_data {
u32 product_id;
u16 range;
u16 min_range;
@@ -84,9 +84,15 @@ struct lg4ff_device_entry {
const char *real_tag;
const char *real_name;
u16 real_product_id;
+
void (*set_range)(struct hid_device *hid, u16 range);
 };
 
+struct lg4ff_device_entry {
+   spinlock_t report_lock;
+   struct lg4ff_wheel_data wdata;
+};
+
 static const signed short lg4ff_wheel_effects[] = {
FF_CONSTANT,
FF_AUTOCENTER,
@@ -278,11 +284,11 @@ int lg4ff_adjust_input_event(struct hid_device *hid, 
struct hid_field *field,
return 0;
}
 
-   switch (entry->product_id) {
+   switch (entry->wdata.product_id) {
case USB_DEVICE_ID_LOGITECH_DFP_WHEEL:
switch (usage->code) {
case ABS_X:
-   new_value = lg4ff_adjust_dfp_x_axis(value, 
entry->range);
+   new_value = lg4ff_adjust_dfp_x_axis(value, 
entry->wdata.range);
input_event(field->hidinput->input, usage->type, 
usage->code, new_value);
return 1;
default:
@@ -298,8 +304,23 @@ static int lg4ff_play(struct input_dev *dev, void *data, 
struct ff_effect *effec
struct hid_device *hid = input_get_drvdata(dev);
struct list_head *report_list = 
&hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct hid_report *report = list_entry(report_list->next, struct 
hid_report, list);
+   struct lg4ff_device_entry *entry;
+   struct lg_drv_data *drv_data;
s32 *value = report->field[0]->value;
int x;
+   unsigned long flags;
+
+   drv_data = hid_get_drvdata(hid);
+   if (!drv_data) {
+   hid_err(hid, "Private driver data not found!\n");
+   return -EINVAL;
+   }
+
+   entry = drv_data->device_props;
+   if (!entry) {
+   hid_err(hid, "Device properties not found!\n");
+   return -EINVAL;
+   }
 
 #define CLAMP(x) do { if (x < 0) x = 0; else if (x > 0xff) x = 0xff; } while 
(0)
 
@@ -308,6 +329,7 @@ static int lg4ff_play(struct input_dev *dev, void *data, 
struct ff_effect *effec
x = effect->u.ramp.start_level + 0x80;  /* 0x80 is no force */
CLAMP(x);
 
+   spin_lock_irqsave(&entry->report_lock, flags);
if (x == 0x80) {
/* De-activate force in slot-1*/
value[0] = 0x13;
@@ -319,6 +341,7 @@ static int lg4ff_play(struct input_dev *dev, void *data, 
struct ff_effect *effec
value[6] = 0x00;
 
hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+   spin_unlock_irqrestore(&entry->report_lock, flags);
return 0;
}
 
@@ -331,6 +354,7 @@ static int lg4ff_play(struct input_dev *dev, void *data, 
struct ff_effect *effec
value[6] = 0x00;
 
hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+   spin_unlock_irqrestore(&entry->report_lock, flags);
break;
}
return 0;
@@ -343,10 +367,11 @@ static void lg4ff_set_autocenter_default(struct input_dev 
*dev, u16 magnitude)
struct hid_device *hid = input_get_drvdata(dev);
struct list_head *report_list = 
&hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct hid_report *report = list_entry(report_list->next, struct 
hid_report, list);
-   s32 *value = report->field[0]->value;
-   u32 expand_a, expand_b;
struct lg4ff_device_entry *entry;
struct lg_drv_data *drv_data;
+   s32 *value = report->field[0]->value;
+   u32 expand_a, expand_b;
+   unsigned long flags;
 
drv_data = hid_get_drvdata(