On Tue, Dec 19 2023 at 22:54, Xingyu Wu wrote:
> +
> +struct jh7110_clkevt {
> + struct clk *clk;
> + struct reset_control *rst;
> + void __iomem *base;
> + u32 reload_val;
> + int irq;
> + char name[sizeof("jh7110-timer.chX")];
> +};
> +
> +struct jh7110_timer_priv {
> + struct reset_control *prst;
> + struct device *dev;
> + struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
> +};
Please format your data structures according to documentation:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
> +/* IRQ handler for the timer */
> +static irqreturn_t jh7110_timer_interrupt(int irq, void *data)
> +{
> + struct clock_event_device *evt = data;
> + struct jh7110_clkevt *clkevt = &jh7110_timer_info.clkevt[0];
> + u8 cpu_id = smp_processor_id();
> + u32 reg = readl(clkevt->base + JH7110_TIMER_INT_STATUS);
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
> + /* Check interrupt status and channel(cpu) ID */
> + if (!(reg & BIT(cpu_id)))
> + return IRQ_NONE;
> +
> + clkevt = &jh7110_timer_info.clkevt[cpu_id];
> + writel(JH7110_TIMER_INT_CLR_ENA, (clkevt->base + JH7110_TIMER_INT_CLR));
> +
> + if (evt->event_handler)
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int jh7110_timer_set_periodic(struct clock_event_device *evt)
> +{
> + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT;
> +
> + writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL);
> + return 0;
> +}
> +
> +static int jh7110_timer_set_oneshot(struct clock_event_device *evt)
> +{
> + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT;
> +
> + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
> + return 0;
> +}
> +
> +static int jh7110_timer_set_next_event(unsigned long next,
> + struct clock_event_device *evt)
> +{
> + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT;
> +
> + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
> + writel(next, clkevt->base + JH7110_TIMER_LOAD);
> +
> + return jh7110_timer_start(clkevt);
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, jh7110_clock_event) = {
> + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> + .rating = JH7110_CLOCKEVENT_RATING,
> + .set_state_shutdown = jh7110_timer_shutdown,
> + .set_state_periodic = jh7110_timer_set_periodic,
> + .set_state_oneshot = jh7110_timer_set_oneshot,
> + .set_state_oneshot_stopped = jh7110_timer_shutdown,
> + .tick_resume = jh7110_timer_tick_resume,
> + .set_next_event = jh7110_timer_set_next_event,
> + .suspend = jh7110_timer_suspend,
> + .resume = jh7110_timer_resume,
> +};
See formatting rules.
> +static int jh7110_timer_starting_cpu(unsigned int cpu)
> +{
> + struct clock_event_device *evt = per_cpu_ptr(&jh7110_clock_event, cpu);
> + struct jh7110_timer_priv *priv = &jh7110_timer_info;
> + int ret;
> + u32 rate;
> +
> + if (cpu >= JH7110_TIMER_CH_MAX)
> + return -ENOMEM;
> +
> + ret = clk_prepare_enable(priv->clkevt[cpu].clk);
> + if (ret)
> + return ret;
> +
> + ret = reset_control_deassert(priv->clkevt[cpu].rst);
> + if (ret)
> + return ret;
> +
> + rate = clk_get_rate(priv->clkevt[cpu].clk);
> + evt->cpumask = cpumask_of(cpu);
> + evt->irq = priv->clkevt[cpu].irq;
> + evt->name = priv->clkevt[cpu].name;
> + clockevents_config_and_register(evt, rate, JH7110_TIMER_MIN_TICKS,
> + JH7110_TIMER_MAX_TICKS);
> +
> + ret = devm_request_irq(priv->dev, evt->irq, jh7110_timer_interrupt,
> + IRQF_TIMER | IRQF_IRQPOLL,
> + evt->name, evt);
How is this all supposed to work from a CPU hotplug state callback which
runs in the early bringup phase with interrupts disabled? clk_prepare()
which is invoked from clk_prepare_enable() can sleep and
devm_request_irq() can sleep too.
Did you ever test this with the required debug config options enabled?
https://www.kernel.org/doc/html/latest/process/submit-checklist.html
Obviously not.
> + if (ret)
> + return ret;
> +
> + ret = irq_set_affinity(evt->irq, cpumask_of(cpu));
> + if (ret)
> + return ret;
> + /* Use one-shot mode */
> + writel(JH7110_TIMER_MODE_SINGLE, (priv->clkevt[cpu].base + JH7110_TIMER_CTL));
> +
> + return jh7110_timer_start(&priv->clkevt[cpu]);
> +}
> +
> +static void jh7110_timer_release(void *data)
> +{
> + struct jh7110_timer_priv *priv = data;
> + int i;
> +
> + for (i = 0; i < JH7110_TIMER_CH_MAX; i++) {
> + /* Disable each channel of timer */
> + if (priv->clkevt[i].base)
> + writel(JH7110_TIMER_DIS, priv->clkevt[i].base + JH7110_TIMER_ENABLE);
> +
> + /* Avoid no initialization in the loop of the probe */
> + if (!IS_ERR_OR_NULL(priv->clkevt[i].rst))
> + reset_control_assert(priv->clkevt[i].rst);
> +
> + if (!IS_ERR_OR_NULL(priv->clkevt[i].clk))
> + clk_disable_unprepare(priv->clkevt[i].clk);
Same problem. And of course this does not undo the other steps of
jh7110_timer_starting_cpu() so if you offline a CPU and then online it
again the callback will fail because the clockevent is already
registered and the interrupt requested. Clearly untested too.
Thanks,
tglx
