linux-kernel - Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <10e6ee08-4f5e-7220-045d-be7ca74b4345@linaro.org>
Date:   Tue, 10 Dec 2019 10:19:23 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Paul Cercueil <paul@...pouillou.net>, od@...c.me,
        linux-kernel@...r.kernel.org,
        Maarten ter Huurne <maarten@...ewalker.org>,
        Mathieu Malaterre <malat@...ian.org>,
        Artur Rojek <contact@...ur-rojek.eu>
Subject: Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST

On 07/11/2019 21:34, Paul Cercueil wrote:
> 
> 
> Le jeu., nov. 7, 2019 at 21:22, Daniel Lezcano
> <daniel.lezcano@...aro.org> a écrit :
>> On 07/11/2019 20:57, Paul Cercueil wrote:
>>>
>>>
>>>  Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano
>>>  <daniel.lezcano@...aro.org> a écrit :
>>>>  On 07/11/2019 16:56, Paul Cercueil wrote:
>>>>>   Hi Daniel,
>>>>>
>>>>>   On 2019-08-16 16:54, Daniel Lezcano wrote:
>>>>>>   On 09/08/2019 14:38, Paul Cercueil wrote:
>>>>>>>   From: Maarten ter Huurne <maarten@...ewalker.org>
>>>>>>>
>>>>>>>   OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>>>>>
>>>>>>>   SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>>>>>>   JZ4780 have a 64-bit OST.
>>>>>>>
>>>>>>>   This driver will register both a clocksource and a sched_clock
>>>>>>> to the
>>>>>>>   system.
>>>>>>>
>>>>>>>   Signed-off-by: Maarten ter Huurne <maarten@...ewalker.org>
>>>>>>>   Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>>>>>>>   Tested-by: Mathieu Malaterre <malat@...ian.org>
>>>>>>>   Tested-by: Artur Rojek <contact@...ur-rojek.eu>
>>>>>>>   ---
>>>>>>>
>>>>>>
>>>>>>   [ ... ]
>>>>>>
>>>>>>>   +    err = clocksource_register_hz(cs, rate);
>>>>>>>   +    if (err) {
>>>>>>>   +        dev_err(dev, "clocksource registration failed: %d\n",
>>>>>>> err);
>>>>>>>   +        clk_disable_unprepare(ost->clk);
>>>>>>>   +        return err;
>>>>>>>   +    }
>>>>>>>   +
>>>>>>>   +    /* Cannot register a sched_clock with interrupts on */
>>>>>>
>>>>>>   Aren't they already disabled?
>>>>>
>>>>>   Sorry for the late reply.
>>>>>
>>>>>   No, they are not already disabled; this is what I get if I
>>>>> comment out
>>>>>   the local_irq_save()/local_irq_restore():
>>>>>
>>>>>   [    0.361014] clocksource: ingenic-ost: mask: 0xffffffff
>>>>> max_cycles:
>>>>>   0xffffffff, max_idle_ns: 159271703898 ns
>>>>>   [    0.361515] clocksource: Switched to clocksource ingenic-ost
>>>>>   [    0.361686] ------------[ cut here ]------------
>>>>>   [    0.361893] WARNING: CPU: 0 PID: 1 at
>>>>> kernel/time/sched_clock.c:179
>>>>>   sched_clock_register+0x7c/0x2e4
>>>>>   [    0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+
>>>>> #461
>>>>>   [    0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
>>>>>   00dfd7a7 806e6db4 8106bb74
>>>>>   [    0.362619]         806f0000 81067ca4 806f31c7 80769478 00000020
>>>>>   10000400 8106bb20 00dfd7a7
>>>>>   [    0.362906]         00000000 00000000 80780000 00000000 00000007
>>>>>   00000001 00000049 3563722d
>>>>>   [    0.363191]         8106ba61 00000000 ffffffff 00000010 806f0000
>>>>>   00000000 00000000 806f0000
>>>>>   [    0.363477]         00000020 00000000 80714534 80770000 00000002
>>>>>   80319154 00000000 80770000
>>>>>   [    0.363762]         ...
>>>>>   [    0.363906] Call Trace:
>>>>>   [    0.364087] [<8001af14>] show_stack+0x40/0x128
>>>>>   [    0.364289] [<8002fd88>] __warn+0xb8/0xe0
>>>>>   [    0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
>>>>>   [    0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
>>>>>   [    0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
>>>>>   [    0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
>>>>>   [    0.365526] [<803d362c>] really_probe+0x104/0x374
>>>>>   [    0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
>>>>>   [    0.365938] [<803d4070>] __driver_attach+0x78/0x118
>>>>>   [    0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
>>>>>   [    0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
>>>>>   [    0.366513] [<803d4878>] driver_register+0x84/0x14c
>>>>>   [    0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
>>>>>   [    0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
>>>>>   [    0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
>>>>>   [    0.367318] [<805df75c>] kernel_init+0x10/0xf8
>>>>>   [    0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
>>>>>   [    0.367722] ---[ end trace 7fedf00408fa3bed ]---
>>>>>   [    0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
>>>>>   every 178956970966ns
>>>>>
>>>>>   At kernel/time/sched_clock.c:179 there is:
>>>>>   WARN_ON(!irqs_disabled());
>>>>
>>>>  That is strange, no drivers is doing that and no warning is appearing.
>>>>
>>>>  Isn't missing a local_irq_disable in the code path in the stack above?
>>>
>>>  I think it comes to the fact that the other drivers are probed much
>>>  earlier in the boot process, while this one is probed as a regular
>>>  platform device driver.
>>
>>
>> There are other drivers doing and nobody is disabling the interrupt:
>>
>> em_sti.c:static struct platform_driver em_sti_device_driver = {
>> ingenic-timer.c:static struct platform_driver ingenic_tcu_driver = {
>> sh_cmt.c:static struct platform_driver sh_cmt_device_driver = {
>> sh_mtu2.c:static struct platform_driver sh_mtu2_device_driver = {
>> sh_tmu.c:static struct platform_driver sh_tmu_device_driver = {
>> timer-ti-dm.c:static struct platform_driver omap_dm_timer_driver = {
> 
> Yes, but they don't register a sched_clock at all (except for
> ingenic-timer, but it does probe using TIMER_OF_DECLARE), and this
> warning is in sched_clock_register().
> 
>>
>>>>>>>   +    local_irq_save(flags);
>>>>>>>   +    if (soc_info->is64bit)
>>>>>>>   +        sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>>>>>>   +    else
>>>>>>>   +        sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>>>>>>   +    local_irq_restore(flags);
>>>>>>>   +
>>>>>>>   +    return 0;
>>>>>>>   +}
>>>>>>>   +

Thomas,

the local_irq_save/restore calls shouldn't be in the
sched_clock_register() function instead of showing a warning?


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog