How to properly unregister LED class devices?

[Pranay Srivastava]

On Mon, Sep 7, 2015 at 4:58 PM, Jacek Anaszewski
<j.anaszewski at samsung.com> wrote:
> Hi Clément,
>
>
> On 09/07/2015 11:05 AM, Clément Vuchener wrote:
>>
>> On Mon, Sep 07, 2015 at 12:08:17PM +0530, Pranay Srivastava wrote:
>>>
>>> On Sun, Sep 6, 2015 at 11:18 PM, Clément Vuchener
>>> <clement.vuchener at gmail.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> I am trying to write a driver that uses LED class devices using works
>>>> for setting the LED brightness but I am not sure of how to unregister the
>>>> devices.
>>>>
>>>> I have been using code like this:
>>>>          led_classdev_unregister(&drvdata->backlight.cdev);
>>>>          cancel_work_sync(&drvdata->backlight.work);
>>>> trying with both flush_work or cancel_work_sync as I have seen it in
>>>> other drivers.
>>>>
>>>> Using flush_work, the kernel oops in my work function when I unplug the
>>>> device. cancel_work_sync seems to fix that, but I am not sure it will work
>>>> every time. I would like to understand what happens and if I am doing
>>>> something wrong, to be sure it will not break in some different setup.
>>>
>>>
>>> Can you post the backtrace?
>>>
>>
>> I could not get it with my patched kernel (I must be missing some config
>> option) so I used the code as a module on my fedora 22 (4.1.6) kernel.
>>
>> general protection fault: 0000 [#1] SMP
>> Modules linked in: hid_corsair_k90(OE) bnep bluetooth nf_nat_h323
>> nf_conntrack_h323 nf_nat_pptp nf_nat_proto_gre nf_conntrack_pptp
>> nf_conntrack_proto_g
>>   snd_hda_codec_hdmi coretemp arc4 kvm_intel snd_hda_codec_realtek iwldvm
>> kvm snd_hda_codec_generic mac80211 snd_hda_intel snd_hda_controller
>> snd_hda_co
>> CPU: 2 PID: 491 Comm: kworker/2:3 Tainted: G           OE
>> 4.1.6-200.fc22.x86_64 #1
>> Hardware name: CLEVO CO.                        W350ET/W350ET, BIOS
>> 1.02.21PM v3 07/01/2013
>> Workqueue: events k90_record_led_work [hid_corsair_k90]
>> task: ffff880223bd4f00 ti: ffff8800c92a0000 task.ti: ffff8800c92a0000
>> RIP: 0010:[<ffffffff814e8816>]  [<ffffffff814e8816>]
>> __dev_printk+0x26/0x90
>> RSP: 0018:ffff8800c92a3d48  EFLAGS: 00010202
>> RAX: 657079740000009d RBX: ffff8801fcee7800 RCX: 000000000001a2e1
>> RDX: ffff8800c92a3d58 RSI: ffff8801fcee7800 RDI: ffffffff81a2673f
>> RBP: ffff8800c92a3d48 R08: 0000001400730102 R09: ffff8800c92a3d58
>> R10: ffffffff81578c4b R11: 0000000000000000 R12: ffff88022f317000
>> R13: ffff88022f31b900 R14: 0000000000000080 R15: ffff8801fcc7d320
>> FS:  0000000000000000(0000) GS:ffff88022f300000(0000)
>> knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000002eee850 CR3: 0000000002c0b000 CR4: 00000000001406e0
>> Stack:
>>   ffff8800c92a3db8 ffffffff814e8bd2 ffffffffa09701f8 ffff8800c92a3d68
>>   ffff880100000010 ffff8800c92a3dc8 ffff8800c92a3d88 000000002440e468
>>   0000000000000000 ffff8801fcee7800 00000000ffffffed 000000000001a2e1
>> Call Trace:
>>   [<ffffffff814e8bd2>] dev_warn+0x62/0x80
>>   [<ffffffffa096f44c>] k90_record_led_work+0x8c/0xa0 [hid_corsair_k90]
>>   [<ffffffff8179da31>] ? __schedule+0x241/0x720
>>   [<ffffffff810baadb>] process_one_work+0x1bb/0x410
>>   [<ffffffff810baeec>] worker_thread+0x1bc/0x480
>>   [<ffffffff810bad30>] ? process_one_work+0x410/0x410
>>   [<ffffffff810bad30>] ? process_one_work+0x410/0x410
>>   [<ffffffff810c0bf8>] kthread+0xd8/0xf0
>>   [<ffffffff810c0b20>] ? kthread_worker_fn+0x180/0x180
>>   [<ffffffff817a2322>] ret_from_fork+0x42/0x70
>>   [<ffffffff810c0b20>] ? kthread_worker_fn+0x180/0x180
>> Code: 00 00 00 00 00 0f 1f 44 00 00 55 48 85 f6 49 89 d1 48 89 e5 74 60 4c
>> 8b 46 50 4d 85 c0 74 26 48 8b 86 90 00 00 00 48 85 c0 74 2a <48> 8b 08 0f be
>> RIP  [<ffffffff814e8816>] __dev_printk+0x26/0x90
>
>
> As your backtrace shows, the problem originates from dev_warn call from
> k90_record_led_work. dev_warn takes dev in its first argument, which
> is already released at the time when it is called as a result of the
> call to flush_work. In order to work this around you could set some
> flag on remove to indicate that LED class device has been released
> and return from k90_record_led_work immediately.

You can also try doing get_device to have > 1 ref count so it won't go
away. But I still feel you shouldn't have to if you re-order your
work_fn and unregister calls. That is flush/cancel work before
unregistering and then at the end remove sysfs group.

> The same applies to your question regarding retaining the LED state
> on remove. Hope this helps.
>
> Is the backtrace always the same? It could likely happen also earlier,
> during dereference of dev in the following line:
>
>
> struct device *dev = led->cdev.dev->parent;
>
> --
> Best Regards,
> Jacek Anaszewski



-- 
        ---P.K.S