Re: [PATCH v4 2/5] clockevents: Add timed freeze

[Thomas Gleixner]

On Thu, 9 Jun 2016, dbasehore@xxxxxxxxxxxx wrote:
>  
> +/*
> + * Clockevent device may run during freeze
> + */
> +# define CLOCK_EVT_FEAT_FREEZE		0x000100

This is a bad name and a horrible comment. The device does not freeze. It is
able to run during suspend. 

Hint: We have CLOCK_SOURCE_SUSPEND_NONSTOP which is self explanatory.

>  /**
>   * struct clock_event_device - clock event device descriptor
>   * @event_handler:	Assigned by the framework to be called by the low
>   *			level handler of the event source
>   * @set_next_event:	set next event function using a clocksource delta
>   * @set_next_ktime:	set next event function using a direct ktime value
> + * @event_pending:	check if the programmed event is still pending. Used
> + *			for freeze events when timekeeping is suspended and
> + *			irqs are disabled.
>   * @next_event:		local storage for the next event in oneshot mode
>   * @max_delta_ns:	maximum delta value in ns
>   * @min_delta_ns:	minimum delta value in ns
> @@ -100,7 +108,9 @@ struct clock_event_device {
>  	void			(*event_handler)(struct clock_event_device *);
>  	int			(*set_next_event)(unsigned long evt, struct clock_event_device *);
>  	int			(*set_next_ktime)(ktime_t expires, struct clock_event_device *);
> +	bool			(*event_expired)(struct clock_event_device *);
>  	ktime_t			next_event;
> +	bool			freeze_event_programmed;

I really don't like that flag. Why do you need it at all?

>  	u64			max_delta_ns;
>  	u64			min_delta_ns;
  
> +static int clockevents_program_freeze_event(struct clock_event_device *dev,
> +					    ktime_t delta)
> +{
> +	int64_t delta_ns = ktime_to_ns(delta);

Why int? Please use u64 and spare all the silly type casts you have in your
code.

> +	unsigned long long clc;

What's wrong with u64?

> +	int ret;
> +
> +	if (delta_ns > (int64_t) dev->max_delta_ns) {
> +		printk_deferred(KERN_WARNING
> +				"Freeze event time longer than max delta\n");
> +		delta_ns = (int64_t) dev->max_delta_ns;

What's the point of this? Tell the caller that it does not work and be done
with it. -ERANGE or something like that.

> +	}

> +	clockevents_tick_resume(dev);
> +	clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT);
> +	delta_ns = max_t(int64_t, delta_ns, dev->min_delta_ns);

You're comparing signed and insigned. Use u64 and max()... Also you really
should tell the caller, that providing a timeout that small is silly.

> +	clc = ((unsigned long long) delta_ns * dev->mult) >> dev->shift;

Sigh.

> +	ret = dev->set_next_event((unsigned long) clc, dev);
> +	if (ret < 0) {
> +		printk_deferred(KERN_WARNING
> +				"Failed to program freeze event\n");
> +		clockevents_shutdown(dev);
> +	} else {
> +		dev->freeze_event_programmed = true;

I'm still not seing why you need that flag.

> +	}
> +
> +	return ret;
> +}
> +
> +static bool clockevents_freeze_event_expired(struct clock_event_device *dev)
> +{
> +	if (dev->freeze_event_programmed)
> +		return dev->event_expired(dev);

So this will oops from deep inside suspend when the clock event does not have
the callback ....

> +	return false;
> +}
> +
> +static void clockevents_cleanup_freeze_event(struct clock_event_device *dev)
> +{
> +	if (!(dev->features & CLOCK_EVT_FEAT_FREEZE))
> +		return;

What's that check for? This is only called from the code below in a section
which cannot be reached when the flag is not set.

> +	clockevents_shutdown(dev);

You can open code this line at the call site because that's all you need.

> +	dev->freeze_event_programmed = false;
> +}

> +/**
> + * timed_freeze - Enter freeze on a CPU for a timed duration
> + * @ops:	Pointers for enter freeze and callback functions.
> + * @data:	Pointer to pass arguments to the function pointers.
> + * @delta:	Time to freeze for. If this amount of time passes in freeze, the
> + *		callback in ops will be called.
> + *
> + * Returns the value from ops->enter_freeze or ops->callback on success, -EERROR
> + * otherwise. If an error is encountered while setting up the clock event,
> + * freeze with still be entered, but it will not be timed nor will the callback
> + * function be run.

That logic makes no sense at all.

> +int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta)
> +{
> +	int cpu = smp_processor_id();
> +	struct tick_device *td = tick_get_device(cpu);
> +	struct clock_event_device *dev;
> +	int ret;
> +
> +	if (!ops || !ops->enter_freeze) {
> +		printk_deferred(KERN_ERR
> +				"[%s] called with invalid ops\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!td || !td->evtdev ||

td is always valid, because it's a pointer to a per cpu variable.

> +	    !(td->evtdev->features & CLOCK_EVT_FEAT_FREEZE)) {
> +		printk_deferred(KERN_WARNING
> +				"[%s] called with invalid clock event device\n",

The function is not called with an invalid clock event device. There is either
no clock event device or the device does not support this.

> +				__func__);
> +		ret = -ENOSYS;
> +		goto freeze_no_check;
> +	}
> +
> +	dev = td->evtdev;
> +	if (!clockevent_state_shutdown(dev)) {
> +		printk_deferred(KERN_WARNING
> +				"[%s] called while clock event device in use\n",
> +				__func__);
> +		ret = -EBUSY;
> +		goto freeze_no_check;
> +	}
> +
> +	ret = clockevents_program_freeze_event(dev, delta);
> +	if (ret < 0)
> +		goto freeze_no_check;
> +
> +	ret = ops->enter_freeze(data);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (ops->callback && clockevents_freeze_event_expired(dev))
> +		ret = ops->callback(data);

This callback thing is just wrong here. If that fails then how is the call
site supposed to figure out where the error came from? From your printks?

The correct way to do this is:

int tick_set_frozen_event(ktime_t delta)
{
    	if (....)
		return -EINVAL;
	if (....)
		return -ENOSYS;

	return program_event(dev, delta);
}

and:

int tick_clear_frozen_event()
{
	ret = event_expired(dev);
	clockevents_shutdown(dev);
	return ret;
}

So no ops, nothing nada.

And at the call site you do:

    ret = tick_set_frozen_event(delta);
    if (ret)
		goto deal_with_ret;

    ret = freeze();
    if (ret) {
       	     tick_clear_frozen_event();
	     goto deal_with_freeze_abort;
    }
    
    ret = tick_clear_frozen_event();

    do_something_sensible(ret);

That's actually understandable and debugable code.

Thanks,

	tglx