Re: [PATCH 2/2] input - leds: fix input_led_disconnect path

[Benjamin Tissoires]

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