Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jindong Yue <jindong.yue@nxp.com>
Cc:     linux-kernel@vger.kernel.org, fweisbec@gmail.com, mingo@kernel.org
Subject: Re: [PATCH] tick/broadcast: Allow later probed device enter oneshot mode
In-Reply-To: <20210329052555.24487-1-jindong.yue@nxp.com>
References: <20210329052555.24487-1-jindong.yue@nxp.com>
Date:   Mon, 29 Mar 2021 23:17:46 +0200
Message-ID: <878s657j7p.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Mon, Mar 29 2021 at 13:25, Jindong Yue wrote:
 
>  /*
>   * Debugging: see timer_list.c
> @@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
>  	 * notification the systems stays stuck in periodic mode
>  	 * forever.
>  	 */
> -	if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
> +	if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
>  		tick_clock_notify();

If the kernel runs in oneshot mode already, then calling
tick_clock_notify() does not make sense.

> +		/*
> +		 * If new broadcast device is installed after high resolution
> +		 * timers enabled, it can not switch to oneshot mode anymore.

This is not restricted to high resolution timers. The point is that
the mode is ONESHOT which might be either HIGHRES or NOHZ or both.

> +		 */
> +		if (tick_broadcast_oneshot_active() &&
> +		    dev->event_handler != tick_handle_oneshot_broadcast) {

How would that condition ever be false for a newly installed device?

> +			tick_broadcast_switch_to_oneshot();
> +		}

So what you really want is:

        if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
        	return;

        if (tick_broadcast_oneshot_active()) {
		tick_broadcast_switch_to_oneshot();
                return;
        }

        tick_clock_notify();

Thanks,

        tglx