Re: [RFC 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

On Wed, 15 Jun 2011, Oliver Neukum wrote:

quoted

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 9fa4fe1..4dbb2c1 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -10,6 +10,7 @@
  * any later version.
  */
 
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input.h>

@@ -20,6 +21,7 @@
 #define WIIMOTE_NAME "Nintendo Wii Remote"
 
 struct wiimote_data {
+	atomic_t ready;
 	struct hid_device *hdev;
 	struct input_dev *input;
 };

@@ -27,12 +29,22 @@ struct wiimote_data {
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {
+	struct wiimote_data *wdata = input_get_drvdata(dev);
+
+	if (!atomic_read(&wdata->ready))
+		return -EBUSY;
+

Here an smp_rmb()

quoted

 	return 0;
 }
 
 static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 							u8 *raw_data, int size)
 {
+	struct wiimote_data *wdata = hid_get_drvdata(hdev);
+
+	if (!atomic_read(&wdata->ready))
+		return -EBUSY;
+
 	if (size < 1)
 		return -EINVAL;
 

@@ -98,6 +110,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		goto err_stop;
 	}

Here an smp_wmb()
It is redundant because input_register_device takes a spinlock which
implies a barrier, but you'll make the automated checking tools happier.  

Yeah, spinlock implies SMP barrier implicitly.

I don't think adding barriers just to make some buggy automatic checker 
happy is worth it.

Thanks for the review, Oliver.

-- 
Jiri Kosina
SUSE Labs