On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@xxxxxxxxx> 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> Cheers, Benjamin -- 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
