Re: [PATCH v13 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Hi Baolin,

On 10/01/2018 03:18 PM, Baolin Wang wrote:
> Hi Jacek,
> 
> On 30 September 2018 at 23:33, Jacek Anaszewski
> <jacek.anaszewski@xxxxxxxxx> wrote:
>> Hi Baolin,
>>
>> Thank you for adding the dimming support.
>>
>> I've tested it and detected severe problem when
>> delta_t is lower than 50, i.e. UPDATE_INTERVAL.
>>
>> echo "10 49 20 49" > pattern
>>
>> results after a while in a system wide freeze, see the following
>> kernel log:
>>
>> [  210.593592] rcu: INFO: rcu_sched self-detected stall on CPU
>> [  210.593601] rcu:     4-....: (21134 ticks this GP) idle=5b6/0/0x3
>> softirq=4843/4843 fqs=5250
>> [  210.593602] rcu:      (t=21000 jiffies g=25697 q=79)
>> [  210.593605] NMI backtrace for cpu 4
>> [  210.593608] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 4.19.0-rc1+ #156
>> [  210.593609] Hardware name: Gigabyte Technology Co., Ltd.
>> Z97-HD3/Z97-HD3, BIOS F6 06/11/2014
>> [  210.593609] Call Trace:
>> [  210.593612]  <IRQ>
>> [  210.593618]  dump_stack+0x46/0x5b
>> [  210.593621]  nmi_cpu_backtrace+0x90/0xa0
>> [  210.593625]  ? lapic_can_unplug_cpu+0x90/0x90
>> [  210.593627]  nmi_trigger_cpumask_backtrace+0x8d/0xc0
>> [  210.593630]  rcu_dump_cpu_stacks+0x83/0xb3
>> [  210.593632]  rcu_check_callbacks+0x5aa/0x710
>> [  210.593636]  ? tick_sched_do_timer+0x50/0x50
>> [  210.593638]  update_process_times+0x23/0x50
>> [  210.593640]  tick_sched_handle+0x2f/0x40
>> [  210.593642]  tick_sched_timer+0x32/0x70
>> [  210.593644]  __hrtimer_run_queues+0xf7/0x270
>> [  210.593646]  hrtimer_interrupt+0x11d/0x270
>> [  210.593649]  smp_apic_timer_interrupt+0x5d/0x130
>> [  210.593651]  apic_timer_interrupt+0xf/0x20
>> [  210.593655] RIP: 0010:pattern_trig_timer_function+0x23/0x100
>> [  210.593657] Code: ff ff eb f6 0f 1f 00 41 54 4c 8d a7 b0 df ff ff 55
>> 53 48 89 fb 49 8d ac 24 18 20 00 00 48 89 ef e8 f2 d3 29 00 eb 16 8b 53
>> f4 <8b> 08 39 ca 77 05 83 f9 31 77 6d 4c 89 e7 e8 aa f9 ff ff 80 7b f8
>> [  210.593658] RSP: 0018:ffff8d1017903eb8 EFLAGS: 00000216 ORIG_RAX:
>> ffffffffffffff13
>> [  210.593659] RAX: ffff8d0fe7330010 RBX: ffff8d0fe7332050 RCX:
>> 0000000000000031
>> [  210.593660] RDX: 0000000000000000 RSI: 0000000000000014 RDI:
>> 000000000000000a
>> [  210.593661] RBP: ffff8d0fe7332018 R08: ffff8d1017903f08 R09:
>> ffff8d1017903f08
>> [  210.593662] R10: 0000002c1cd5e92f R11: 0000000000000000 R12:
>> ffff8d0fe7330000
>> [  210.593663] R13: ffffffffaa738a70 R14: 0000000000000000 R15:
>> 0000000000000001
>> [  210.593665]  ? apic_timer_interrupt+0xa/0x20
>> [  210.593666]  ? pattern_trig_activate+0xc0/0xc0
>> [  210.593669]  ? pattern_trig_timer_function+0x36/0x100
>> [  210.593671]  call_timer_fn+0x26/0x130
>> [  210.593672]  run_timer_softirq+0x1cc/0x400
>> [  210.593674]  ? enqueue_hrtimer+0x35/0x90
>> [  210.593676]  ? __hrtimer_run_queues+0x127/0x270
>> [  210.593679]  ? recalibrate_cpu_khz+0x10/0x10
>> [  210.593681]  __do_softirq+0x104/0x2a5
>> [  210.593685]  irq_exit+0x9d/0xa0
>> [  210.593687]  smp_apic_timer_interrupt+0x67/0x130
>> [  210.593688]  apic_timer_interrupt+0xf/0x20
>> [  210.593689]  </IRQ>
>> [  210.593693] RIP: 0010:cpuidle_enter_state+0xf8/0x2a0
>> [  210.593694] Code: c7 0f 1f 44 00 00 31 ff e8 65 69 95 ff 80 7c 24 03
>> 00 74 12 9c 58 f6 c4 02 0f 85 8c 01 00 00 31 ff e8 7c 41 9a ff fb 4d 29
>> e7 <48> ba cf f7 53 e3 a5 9b c4 20 4c 89 f8 4c 89 fd 48 f7 ea 48 c1 fd
>> [  210.593695] RSP: 0018:ffffb0fc00cf7e88 EFLAGS: 00000206 ORIG_RAX:
>> ffffffffffffff13
>> [  210.593696] RAX: ffff8d1017920dc0 RBX: ffff8d1014b49c00 RCX:
>> 000000000000001f
>> [  210.593697] RDX: 0000000000000000 RSI: 000000c1514cc3a0 RDI:
>> 0000000000000000
>> [  210.593698] RBP: 0000000000000001 R08: fffffffbcf10adc2 R09:
>> 000000000000afc7
>> [  210.593699] R10: 000000000000003c R11: ffff8d101791fe68 R12:
>> 0000002c1d11a32d
>> [  210.593700] R13: 0000000000000001 R14: 0000000000000004 R15:
>> 0000000000011a82
>> [  210.593703]  ? cpuidle_enter_state+0xdb/0x2a0
>> [  210.593705]  do_idle+0x1f5/0x250
>> [  210.593707]  cpu_startup_entry+0x6a/0x70
>> [  210.593709]  start_secondary+0x183/0x1b0
>> [  210.593711]  secondary_startup_64+0xa4/0xb0
>>
>> Probably the best solution would be to avoid the dimming if
>> delta_t is lower than UPDATE_INTERVAL.
> 
> Thanks for your testing. Yes, I will valid the delta_t value when user
> use the gradual dimming, and add some comments in ABI to make it
> clear, that we should not set delta_t lower than UPDATE_INTERVAL for
> gradual dimming.

Thanks.

[...]
>>> +static ssize_t repeat_show(struct device *dev, struct device_attribute *attr,
>>> +                        char *buf)
>>> +{
>>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +     struct pattern_trig_data *data = led_cdev->trigger_data;
>>> +     int repeat;
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     repeat = data->last_repeat;
>>
>> I'm not sure if we discussed it earlier, but currently repeat_show
>> always reports initially requested repeat number, instead of the
>> number of remaining iterations. IMHO the latter approach would be
>> more useful.
> 
> That's what we discussed before and had a consensus about this. Please see:
> https://lore.kernel.org/patchwork/patch/971746/

Ah right. I had a vague reminiscence of this discussion, but failed to
track it back.

OK, so no action is required here, let's keep the things as we agreed.

-- 
Best regards,
Jacek Anaszewski