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

[Dmitry Torokhov]

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?

>
> 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.

Thanks.

-- 
Dmitry