On Wed, Dec 20, 2017 at 7:20 PM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > On Wed, Dec 20, 2017 at 9:11 AM, Benjamin Tissoires > <benjamin.tissoires@xxxxxxxxxx> wrote: >> On Sat, Dec 16, 2017 at 1:48 AM, Dmitry Torokhov >> <dmitry.torokhov@xxxxxxxxx> wrote: >>> Hi Benjamin, >>> >>> On Thu, Dec 14, 2017 at 02:25:22PM +0100, Benjamin Tissoires wrote: >>>> Before unregistering the led classes, we have to be sure there is no >>>> more events in the input pipeline. >>>> Closing the input node before removing the led classes flushes the >>>> pipeline and this prevents segfaults. >>> >>> I do not think this actually the issue. input_leds_event() is an empty >>> stub, it does not really do anything with events it receives form input >>> core, and it does not reference the led structures. The way input leds >> >> Right. Actually, we could simply drop the stub as input_to_handler() >> checks for the validity of .event(). >> >>> work is that input driver owns the hardware led and is responsible for >>> communicating with it. The LED subsystem is a secondary interface, >>> requests coming from /sys/class/led/... are being forwarded to the input >>> core and then to the input hardware driver by the way of: >>> >>> input_leds_brightness_set() -> >>> input_inject_event() -> >>> input_handle_event()-> >>> disposition & INPUT_PASS_TO_DEVICE >>> dev->event() (in atkbd or hid-input) >>> actual control of LED >>> >>> I do not see how stopping event flow from input core to input-leds would >>> help here. >> >> I wonder if this solution in this patch is not just masking the race >> condition by forcing the sync. >> >> After further researches, I realized that the patch is actually not >> needed. In the upstream repo of Peter, the code inject events and >> closes the device ASAP, triggering races with udev. >> Adding the proper udev stubs seem to solve the issues. >> I still have other random crashes, but I can't seem to reproduce the >> error of https://bugzilla.kernel.org/show_bug.cgi?id=197679 now. >> >> Anyway, I can't explain the backtrace of the kernel bug, it is as if >> we are calling the unregister function without having properly >> registered the device. But this can not happen because of the mutexes >> in place. >> The race seems to be udev and probably the led class accesses... > > I wonder if the crash was observed only with your first patch adding > the delay in initializing the leds? Nah, the bug was initially found by Peter on a plain Fedora system without my crap :) > >> >> BTW, if the handler doesn't use the events coming in from the input >> subsystem, is there any benefits of opening the device from the >> handler? >> I tried without, and it seemed to be working fine, but I wonder if >> there is no hidden dependency I haven't see. > > You want to open the device as it is what essentially powers it up, so > that it can react to input_inject_event() sent via LED subsystem. In > case of atkbd input_open_device() will result in call to serio_open() > which enables the KBD port of i8042. You probably do not see any > difference because the VT subsystem already attached the keyboard > handler to the device and it already "opened" the device in question. > Right. So I guess there is not much to do then :) Cheers, Benjamin