Re: [PATCH v2 2/2] alarmtimer: Modify alarmtimer suspend callback to check for imminent alarm using PM notifier

[Thomas Gleixner]

On Thu, Feb 08 2024 at 19:56, Pranav Prasad wrote:

The subject line wants some trimming. It's supposed to be a concise
short summary and not a novel.

Aside of that it's blantantly wrong. It does not modify the suspend
callback to check with a PM notifier. It adds a PM notifier to check
early before the suspend callback runs.

> +/**
> + * alarmtimer_get_soonest - Finds the soonest alarm to expire among the
> + * alarm bases.
> + * @rtc: ptr to rtc_device struct
> + * @min: ptr to relative time to the soonest alarm to expire
> + * @expires: ptr to absolute time of the soonest alarm to expire
> + * @type: ptr to alarm type
> + *
> + * Returns 1 if soonest alarm was found, returns 0 if don't care.
> + */
> +static int alarmtimer_get_soonest(struct rtc_device *rtc, ktime_t *min,
> +					ktime_t *expires, int *type)

Please align the second argument with the first argument of the
function.

Also the return value wants to be bool.

> +{
> +	int i;
> +	unsigned long flags;

Please see:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

> +	spin_lock_irqsave(&freezer_delta_lock, flags);
> +	*min = freezer_delta;
> +	*expires = freezer_expires;
> +	*type = freezer_alarmtype;
> +	freezer_delta = 0;
> +	spin_unlock_irqrestore(&freezer_delta_lock, flags);

This only makes sense for the actual suspend operation because freezing
of processes happens after the notifier callback runs.

> +	rtc = alarmtimer_get_rtcdev();
> +	/* If we have no rtcdev, just return */
> +	if (!rtc)
> +		return 0;
> +
> +	/* Find the soonest timer to expire */
> +	for (i = 0; i < ALARM_NUMTYPE; i++) {
> +		struct alarm_base *base = &alarm_bases[i];
> +		struct timerqueue_node *next;
> +		ktime_t delta;
> +
> +		spin_lock_irqsave(&base->lock, flags);
> +		next = timerqueue_getnext(&base->timerqueue);
> +		spin_unlock_irqrestore(&base->lock, flags);
> +		if (!next)
> +			continue;
> +		delta = ktime_sub(next->expires, base->get_ktime());
> +		if (!*min || delta < *min) {

You can spare the !*min if you initialize min to KTIME_MAX.

> +			*expires = next->expires;
> +			*min = delta;
> +			*type = i;
> +		}
> +	}
> +
> +	if (*min == 0)

!*min above and *min == 0 here. Can we have consistency please?

> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int alarmtimer_pm_callback(struct notifier_block *nb,
> +			    unsigned long mode, void *_unused)
> +{
> +	ktime_t min, expires;
> +	struct rtc_device *rtc = NULL;
> +	int type;

Same as above vs. ordering.

> +	switch (mode) {
> +	case PM_SUSPEND_PREPARE:
> +		/* Find the soonest timer to expire */
> +		if (!alarmtimer_get_soonest(rtc, &min, &expires, &type))
> +			return NOTIFY_DONE;
> +
> +		if (ktime_to_ns(min) <
> +			suspend_check_duration_ms * NSEC_PER_MSEC) {

No need for the line break. 80 character limit is gone.

> +			pr_warn("[%s] Suspend abort due to imminent alarm\n", __func__);

Why is this a warning? If at all it wants to be pr_debug() and the
__func_ is pretty useless as grep is able to find the string, no?

> +			pm_wakeup_event(&rtc->dev, suspend_check_duration_ms);

How is this supposed to work? rtc is NULL.

> +			return notifier_from_errno(-ETIME);
> +		}
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block alarmtimer_pm_notifier = {
> +	.notifier_call = alarmtimer_pm_callback,
> +};
> +
>  /**
>   * alarmtimer_get_rtcdev - Return selected rtcdevice
>   *
> @@ -181,6 +263,7 @@ static int alarmtimer_rtc_add_device(struct device *dev)
>  static inline void alarmtimer_rtc_timer_init(void)
>  {
>  	rtc_timer_init(&rtctimer, NULL, NULL);
> +	register_pm_notifier(&alarmtimer_pm_notifier);
>  }
>  
>  static struct class_interface alarmtimer_rtc_interface = {
> @@ -296,49 +379,14 @@ EXPORT_SYMBOL_GPL(alarm_expires_remaining);
>  static int alarmtimer_suspend(struct device *dev)
>  {
>  	ktime_t min, now, expires;
> -	int i, ret, type;
> -	struct rtc_device *rtc;
> -	unsigned long flags;
> +	struct rtc_device *rtc = NULL;
>  	struct rtc_time tm;
> +	int ret, type;

See above.

SNIP

>  	/* Setup an rtc timer to fire that far in the future */

And another NULL pointer dereference follows suit.

How was this ever tested?

You need:

	rtc = alarmtimer_get_rtcdev();
	if (!rtc)
		return [0|NOTIFY_DONE];

in both functions and then hand in rtc to alarmtimer_get_soonest(), no?

Thanks,

        tglx