Re: [PATCH v8 2/3] clocksource: Add JH7110 timer driver

[Thomas Gleixner]

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