| 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
| ||
|
Date: Fri, 28 Feb 2020 00:18:36 +0800
From: Zhou Yanjie <zhouyanjie@...yeetech.com>
To: Paul Cercueil <paul@...pouillou.net>
Cc: linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
tglx@...utronix.de, ralf@...ux-mips.org, paulburton@...nel.org,
jiaxun.yang@...goat.com, chenhc@...ote.com, sboyd@...nel.org,
mturquette@...libre.com, mark.rutland@....com, robh+dt@...nel.org,
daniel.lezcano@...aro.org, geert+renesas@...der.be,
krzk@...nel.org, ebiederm@...ssion.com, miquel.raynal@...tlin.com,
keescook@...omium.org, sernia.zhou@...mail.com,
zhenwenjin@...il.com, dongsheng.qiu@...enic.com
Subject: Re: [PATCH v6 4/7] clocksource: Ingenic: Add high resolution timer
support for SMP.
Hi Paul,
On 2020年02月21日 02:33, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le ven., févr. 21, 2020 at 00:24, 周琰杰 (Zhou Yanjie)
> <zhouyanjie@...yeetech.com> a écrit :
>> Enable clock event handling on per CPU core basis.
>> Make sure that interrupts raised on the first core execute
>> event handlers on the correct CPU core.
>>
>> Tested-by: H. Nikolaus Schaller <hns@...delico.com>
>> Tested-by: Paul Boddie <paul@...die.org.uk>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.com>
>> ---
>>
>> Notes:
>> v1->v2:
>> 1.Adjust function naming to make it more reasonable.
>> 2.Replace function smp_call_function_single() with
>> smp_call_function_single_async() in order to resolve
>> the warning below:
>>
>> [ 0.350942] smp: Brought up 1 node, 2 CPUs
>> [ 0.365497] ------------[ cut here ]------------
>> [ 0.365522] WARNING: CPU: 0 PID: 1 at kernel/smp.c:300
>> smp_call_function_single+0x110/0x200
>> [ 0.365533] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.5.0-rc1+ #5
>> [ 0.365537] Stack : 00000000 59c73bcd 00000037 80074e80
>> 80000000 80670000 805a0000 80620590
>> [ 0.365557] 8065ce38 8fc0dc8c 806d0000 00000000
>> 80670000 00000001 8fc0dc20 59c73bcd
>> [ 0.365574] 00000000 00000000 806f0000 80670000
>> 00000000 806dab00 00000000 2d302e35
>> [ 0.365591] 203a6d6d 806e0000 806e0000 70617773
>> 80670000 00000000 00000000 00000009
>> [ 0.365610] 00000000 8fc94e20 8fc0de30 80690000
>> 00000018 803592dc 00000000 806d0000
>> [ 0.365627] ...
>> [ 0.365634] Call Trace:
>> [ 0.365647] [<8001b9a0>] show_stack+0x6c/0x12c
>> [ 0.365663] [<804aed20>] dump_stack+0x98/0xc8
>> [ 0.365673] [<8003044c>] __warn+0xc4/0xe8
>> [ 0.365682] [<800304f4>] warn_slowpath_fmt+0x84/0xb8
>> [ 0.365690] [<800a886c>] smp_call_function_single+0x110/0x200
>> [ 0.365703] ---[ end trace 5785856ca39c79d5 ]---
>> [ 0.365557] 8065ce38 8fc0dc8c 806d0000 00000000
>> 80670000 00000001 8fc0dc20 59c73bcd
>> [ 0.365574] 00000000 00000000 806f0000 80670000
>> 00000000 806dab00 00000000 2d302e35
>> [ 0.365591] 203a6d6d 806e0000 806e0000 70617773
>> 80670000 00000000 00000000 00000009
>> [ 0.365610] 00000000 8fc94e20 8fc0de30 80690000
>> 00000018 803592dc 00000000 806d0000
>> [ 0.365627] ...
>> [ 0.365634] Call Trace:
>> [ 0.365647] [<8001b9a0>] show_stack+0x6c/0x12c
>> [ 0.365663] [<804aed20>] dump_stack+0x98/0xc8
>> [ 0.365673] [<8003044c>] __warn+0xc4/0xe8
>> [ 0.365682] [<800304f4>] warn_slowpath_fmt+0x84/0xb8
>> [ 0.365690] [<800a886c>] smp_call_function_single+0x110/0x200
>> [ 0.365703] ---[ end trace 5785856ca39c79d5 ]---
>>
>> v2->v3:
>> No Change.
>>
>> v3->v4:
>> Rebase on top of kernel 5.6-rc1.
>>
>> v4->v5:
>> Move the check for (evt->event_handler) from
>> "ingenic_per_cpu_event_handler"
>> to "ingenic_tcu_cevt_cb".
>>
>> v5->v6:
>> No change.
>>
>> drivers/clocksource/ingenic-timer.c | 113
>> +++++++++++++++++++++++++++++-------
>> 1 file changed, 91 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clocksource/ingenic-timer.c
>> b/drivers/clocksource/ingenic-timer.c
>> index 4bbdb3d..e396326 100644
>> --- a/drivers/clocksource/ingenic-timer.c
>> +++ b/drivers/clocksource/ingenic-timer.c
>> @@ -1,7 +1,8 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> - * JZ47xx SoCs TCU IRQ driver
>> + * XBurst SoCs TCU IRQ driver
>> * Copyright (C) 2019 Paul Cercueil <paul@...pouillou.net>
>> + * Copyright (C) 2020 周琰杰 (Zhou Yanjie) <zhouyanjie@...yeetech.com>
>> */
>>
>> #include <linux/bitops.h>
>> @@ -21,18 +22,23 @@
>>
>> #include <dt-bindings/clock/ingenic,tcu.h>
>>
>> +static DEFINE_PER_CPU(call_single_data_t, ingenic_cevt_csd);
>> +
>> struct ingenic_soc_info {
>> unsigned int num_channels;
>> };
>>
>> struct ingenic_tcu {
>> struct regmap *map;
>> + struct device_node *np;
>> struct clk *timer_clk, *cs_clk;
>> + unsigned int timer_local[NR_CPUS];
>> unsigned int timer_channel, cs_channel;
>> struct clock_event_device cevt;
>> struct clocksource cs;
>> - char name[4];
>> + char name[8];
>> unsigned long pwm_channels_mask;
>> + int cpu;
>> };
>>
>> static struct ingenic_tcu *ingenic_tcu;
>> @@ -81,6 +87,24 @@ static int ingenic_tcu_cevt_set_next(unsigned long
>> next,
>> return 0;
>> }
>>
>> +static void ingenic_per_cpu_event_handler(void *info)
>> +{
>> + struct clock_event_device *cevt = (struct clock_event_device *)
>> info;
>> +
>> + cevt->event_handler(cevt);
>> +}
>> +
>> +static void ingenic_tcu_per_cpu_cb(struct clock_event_device *evt)
>> +{
>> + struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
>> + call_single_data_t *csd;
>> +
>> + csd = &per_cpu(ingenic_cevt_csd, tcu->cpu);
>> + csd->info = (void *) evt;
>> + csd->func = ingenic_per_cpu_event_handler;
>> + smp_call_function_single_async(tcu->cpu, csd);
>> +}
>> +
>> static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
>> {
>> struct clock_event_device *evt = dev_id;
>> @@ -89,7 +113,7 @@ static irqreturn_t ingenic_tcu_cevt_cb(int irq,
>> void *dev_id)
>> regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
>>
>> if (evt->event_handler)
>> - evt->event_handler(evt);
>> + ingenic_tcu_per_cpu_cb(evt);
>>
>> return IRQ_HANDLED;
>> }
>> @@ -105,14 +129,21 @@ static struct clk * __init
>> ingenic_tcu_get_clock(struct device_node *np, int id)
>> return of_clk_get_from_provider(&args);
>> }
>>
>> -static int __init ingenic_tcu_timer_init(struct device_node *np,
>> - struct ingenic_tcu *tcu)
>> +static int ingenic_tcu_setup_per_cpu_cevt(struct device_node *np,
>> + unsigned int channel)
>> {
>> - unsigned int timer_virq, channel = tcu->timer_channel;
>> + unsigned int timer_virq;
>> struct irq_domain *domain;
>> + struct ingenic_tcu *tcu;
>> unsigned long rate;
>> int err;
>>
>> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
>> + if (!tcu)
>> + return -ENOMEM;
>> +
>> + tcu->map = ingenic_tcu->map;
>> +
>> tcu->timer_clk = ingenic_tcu_get_clock(np, channel);
>> if (IS_ERR(tcu->timer_clk))
>> return PTR_ERR(tcu->timer_clk);
>> @@ -139,13 +170,15 @@ static int __init ingenic_tcu_timer_init(struct
>> device_node *np,
>> goto err_clk_disable;
>> }
>>
>> - snprintf(tcu->name, sizeof(tcu->name), "TCU");
>> + snprintf(tcu->name, sizeof(tcu->name), "TCU%u", channel);
>>
>> err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
>> tcu->name, &tcu->cevt);
>> if (err)
>> goto err_irq_dispose_mapping;
>>
>> + tcu->cpu = smp_processor_id();
>> + tcu->timer_channel = channel;
>> tcu->cevt.cpumask = cpumask_of(smp_processor_id());
>> tcu->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>> tcu->cevt.name = tcu->name;
>> @@ -166,6 +199,25 @@ static int __init ingenic_tcu_timer_init(struct
>> device_node *np,
>> return err;
>> }
>>
>> +static int ingenic_tcu_setup_cevt(unsigned int cpu)
>> +{
>> + int ret;
>> +
>> + ret = ingenic_tcu_setup_per_cpu_cevt(ingenic_tcu->np,
>> + ingenic_tcu->timer_local[cpu]);
>> + if (ret)
>> + goto err_tcu_clocksource_cleanup;
>> +
>> + return 0;
>> +
>> +err_tcu_clocksource_cleanup:
>> + clocksource_unregister(&ingenic_tcu->cs);
>> + clk_disable_unprepare(ingenic_tcu->cs_clk);
>> + clk_put(ingenic_tcu->cs_clk);
>> + kfree(ingenic_tcu);
>> + return ret;
>> +}
>> +
>> static int __init ingenic_tcu_clocksource_init(struct device_node *np,
>> struct ingenic_tcu *tcu)
>> {
>> @@ -239,6 +291,7 @@ static int __init ingenic_tcu_init(struct
>> device_node *np)
>> const struct ingenic_soc_info *soc_info = id->data;
>> struct ingenic_tcu *tcu;
>> struct regmap *map;
>> + unsigned cpu = 0;
>> long rate;
>> int ret;
>>
>> @@ -252,13 +305,18 @@ static int __init ingenic_tcu_init(struct
>> device_node *np)
>> if (!tcu)
>> return -ENOMEM;
>>
>> - /* Enable all TCU channels for PWM use by default except
>> channels 0/1 */
>> - tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
>> + /*
>> + * Enable all TCU channels for PWM use by default except
>> channels 0/1,
>> + * and channel 2 if target CPU is JZ4780 and SMP is selected.
>> + */
>> + tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1,
>> + NR_CPUS + 1);
>> of_property_read_u32(np, "ingenic,pwm-channels-mask",
>> (u32 *)&tcu->pwm_channels_mask);
>>
>> - /* Verify that we have at least two free channels */
>> - if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels -
>> 2) {
>> + /* Verify that we have at least NR_CPUS + 1 free channels */
>> + if (hweight8(tcu->pwm_channels_mask) >
>> + soc_info->num_channels - NR_CPUS + 1) {
>
> NR_CPUS can be up to 256. You want to use num_online_cpus() here, I
> believe.
Sure, will change in next version.
>
>> pr_crit("%s: Invalid PWM channel mask: 0x%02lx\n", __func__,
>> tcu->pwm_channels_mask);
>> ret = -EINVAL;
>> @@ -266,13 +324,27 @@ static int __init ingenic_tcu_init(struct
>> device_node *np)
>> }
>>
>> tcu->map = map;
>> + tcu->np = np;
>> ingenic_tcu = tcu;
>>
>> - tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
>> + tcu->timer_local[cpu] =
>> find_first_zero_bit(&tcu->pwm_channels_mask,
>> soc_info->num_channels);
>> - tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
>> - soc_info->num_channels,
>> - tcu->timer_channel + 1);
>> +
>> + if (NR_CPUS > 1) {
>> + for (cpu = 1; cpu < NR_CPUS; cpu++)
>> + tcu->timer_local[cpu] = find_next_zero_bit(
>> + &tcu->pwm_channels_mask,
>> + soc_info->num_channels,
>> + tcu->timer_local[cpu - 1] + 1);
>> +
>> + tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
>> + soc_info->num_channels,
>> + tcu->timer_local[cpu-1] + 1);
>> + } else {
>> + tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
>> + soc_info->num_channels,
>> + tcu->timer_local[cpu] + 1);
>> + }
>
> I believe you can factorize the code a bit here - don't check for
> NR_CPUS > 1, and have a for (cpu = 0; cpu < num_online_cpus(); cpu++)
> loop. You could use a temporary variable to store the 'next bit' value.
Sure.
>
> -Paul
>
>>
>> ret = ingenic_tcu_clocksource_init(np, tcu);
>> if (ret) {
>> @@ -280,9 +352,10 @@ static int __init ingenic_tcu_init(struct
>> device_node *np)
>> goto err_free_ingenic_tcu;
>> }
>>
>> - ret = ingenic_tcu_timer_init(np, tcu);
>> - if (ret)
>> - goto err_tcu_clocksource_cleanup;
>> + /* Setup clock events on each CPU core */
>> + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "Ingenic XBurst:
>> online",
>> + ingenic_tcu_setup_cevt, NULL);
>> + WARN_ON(ret < 0);
>>
>> /* Register the sched_clock at the end as there's no way to undo
>> it */
>> rate = clk_get_rate(tcu->cs_clk);
>> @@ -290,10 +363,6 @@ static int __init ingenic_tcu_init(struct
>> device_node *np)
>>
>> return 0;
>>
>> -err_tcu_clocksource_cleanup:
>> - clocksource_unregister(&tcu->cs);
>> - clk_disable_unprepare(tcu->cs_clk);
>> - clk_put(tcu->cs_clk);
>> err_free_ingenic_tcu:
>> kfree(tcu);
>> return ret;
>> --
>> 2.7.4
>>
>
Powered by blists - more mailing lists