| Message ID | 1308095157-4699-6-git-send-email-dh.herrmann@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Am Mittwoch, 15. Juni 2011, 01:45:50 schrieb David Herrmann: > The wiimote first starts HID hardware and then registers the input > device. We need to synchronize the startup so no event handler will > start parsing events when the wiimote device is not ready, yet. Hi, here the fun begins. Using atomic ops is not for the faint of heart. The problem is that atomic_t guarantees atomicity, not ordering. Tasks on on other CPUs reading wdata->ready==1 does not mean that they'll read anything else written before that as it was written or the old value. You need smp barriers. > > Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> > --- > 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() > 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. > + atomic_set(&wdata->ready, 1); > hid_info(hdev, "New device registered\n"); > return 0; Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 15 Jun 2011, Oliver Neukum wrote: > > Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> > > --- > > 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() > > > 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.
Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina: > > 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. Fine by me as long as the problem is understood. Regards Oliver
On Thu, Jun 16, 2011 at 12:18 PM, Oliver Neukum <oneukum@suse.de> wrote: > Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina: >> > 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. > > Fine by me as long as the problem is understood. Does that mean that I can omit the smp_rmb(), too? I do not like relying on input_register_device to use a spinlock. It makes reading the code hard, so I added both barriers. Should I remove them again or keep them? Thank you for reviewing. I have fixed all your concerns. I will resend the patches if there are no more comments on the code as [PATCH] in few days. Regards David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 16. Juni 2011, 12:29:40 schrieb David Herrmann: > On Thu, Jun 16, 2011 at 12:18 PM, Oliver Neukum <oneukum@suse.de> wrote: > > Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina: > >> > 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. > > > > Fine by me as long as the problem is understood. > > Does that mean that I can omit the smp_rmb(), too? I do not like > relying on input_register_device to use a spinlock. It makes reading > the code hard, so I added both barriers. Should I remove them again or > keep them? The full explanation is in Documentation/atomic-ops.txt Basically, whenever you do if (!descriptor->flag) return -ERR; descriptor->pointer->member = expression; //or anything else with the pointer you need smp_rmb() in between If you do if (!descriptor->flag) return -ERR; // no use of pointer here spin_lock(&lock); descriptor->pointer->member = expression; //or anything else with the pointer You are save and can do without the smp_rmb() But, you still need smp_rmb() if you do: if (!descriptor->flag) return -ERR; // no use of pointer here spin_lock(&descriptor->pointer->lock); All clear? I must say I stopped listing and checking all the places you use the flag, but I think some were buggy. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + 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; } + atomic_set(&wdata->ready, 1); hid_info(hdev, "New device registered\n"); return 0;
The wiimote first starts HID hardware and then registers the input device. We need to synchronize the startup so no event handler will start parsing events when the wiimote device is not ready, yet. Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> --- drivers/hid/hid-wiimote.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)