On Tue, 16 Jul 2013, Benjamin Tissoires wrote: > > Report fields can be updated from HID drivers unlocked via > > hid_set_field(). It is protected by input_lock in HID core so only a > > single input event is handled at a time. USBHID can thus update the field > > unlocked and doesn't conflict with any HID vendor/device drivers. Note, > > many HID drivers make heavy use of hid_set_field() in that way. > > > > But usbhid also schedules a work to gather multiple LED changes in a > > single report. Hence, we used to lock the LED field update so the work can > > read a consistent state. However, hid_set_field() only writes a single > > integer field, which is guaranteed to be allocated all the time. So the > > worst possible race-condition is a garbage read on the LED field. > > > > Therefore, there is no need to protect the update. In fact, the only thing > > that is prevented by locking hid_set_field(), is an LED update while the > > scheduled work currently writes an older LED update out. However, this > > means, a new work is scheduled directly when the old one is done writing > > the new state to the device. So we actually _win_ by not protecting the > > write and allowing the write to be combined with the current write. A new > > worker is still scheduled, but will not write any new state. So the LED > > will not blink unnecessarily on the device. > > > > Assume we have the LED set to 0. Two request come in which enable the LED > > and immediately disable it. The current situation with two CPUs would be: > > > > usb_hidinput_input_event() | hid_led() > > ---------------------------------+---------------------------------- > > spin_lock(&usbhid->lock); > > hid_set_field(1); > > spin_unlock(&usbhid->lock); > > schedule_work(...); > > spin_lock(&usbhid->lock); > > __usbhid_submit_report(..1..); > > spin_unlock(&usbhid->lock); > > spin_lock(&usbhid->lock); > > hid_set_field(0); > > spin_unlock(&usbhid->lock); > > schedule_work(...); > > spin_lock(&usbhid->lock); > > __usbhid_submit_report(..0..); > > spin_unlock(&usbhid->lock); > > > > With the locking removed, we _might_ end up with (look at the changed > > __usbhid_submit_report() parameters in the first try!): > > > > usb_hidinput_input_event() | hid_led() > > ---------------------------------+---------------------------------- > > hid_set_field(1); > > schedule_work(...); > > spin_lock(&usbhid->lock); > > hid_set_field(0); > > schedule_work(...); > > __usbhid_submit_report(..0..); > > spin_unlock(&usbhid->lock); > > > > ... next work ... > > > > spin_lock(&usbhid->lock); > > __usbhid_submit_report(..0..); > > spin_unlock(&usbhid->lock); > > > > As one can see, we no longer send the "LED ON" signal as it is disabled > > immediately afterwards and the following "LED OFF" request overwrites the > > pending "LED ON". > > > > It is important to note that hid_set_field() is not atomic, so we might > > also end up with any other value. But that doesn't matter either as we > > _always_ schedule the next work with a correct value and schedule_work() > > acts as memory barrier, anyways. So in the worst case, we run > > __usbhid_submit_report(..<garbage>..) in the first case and the following > > __usbhid_submit_report() will write the correct value. But LED states are > > booleans so any garbage will be converted to either 0 or 1 and the remote > > device will never see invalid requests. > > > > Why all this? It avoids any custom locking around hid_set_field() in > > usbhid and finally allows us to provide a generic hidinput_input_event() > > handler for all HID transport drivers. > > > > Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx> > > --- > > Hi David, > > that was a very good commit message! > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> I love this patch :) Thanks David, thanks Benjamin. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
