Re: [PATCH v8 3/4] virtio-rtc: Add alarm feature

On Thu, Apr 17, 2025 at 05:05:00PM +0200, Peter Hilber wrote:
> On Wed, Apr 16, 2025 at 10:17:33AM +0200, Matias Ezequiel Vara Larsen wrote:
> > On Thu, Mar 06, 2025 at 10:51:11AM +0100, Peter Hilber wrote:
> > > Add the VIRTIO_RTC_F_ALARM feature (without normative statements).
> > > 
> > > The intended use case is: A driver needs to react when an alarm time has
> > > been reached, but at alarm time, the driver may be in a sleep state or
> > > powered off. The alarm feature can resume and notify the driver in this
> > > case. Alarms may be retained across device resets (including reset on
> > > boot).
> > > 
> > > Peculiarities
> > > -------------
> > > 
> > > Unlike usual alarm clocks, a virtio-rtc alarm-capable clock may step
> > > autonomously at any time: An alarm may change back from "expired" to
> > > "not expired" before the driver has started processing an alarm
> > > notification.
> > > 
> > > To address the above, and the device resets, define "alarm expiration"
> > > in such a way that the driver always has a chance to react to an alarm,
> > > and make the device always responsible for notifying the driver about an
> > > alarm expiration.
> > > 
> > > The VIRTIO_RTC_REQ_SET_ALARM_ENABLED request is there so that the Linux
> > > ioctls RTC_AIE_ON and RTC_AIE_OFF only need to emit one request.
> > > 
> > > Signed-off-by: Peter Hilber <[email protected]>
> > > ---
> > > 

[...]

> > > +An alarm \emph{expiration} becomes obsolete
> > > +
> > > +\begin{itemize}
> > > +\item when the driver disables the alarm, or
> > > +
> > > +\item when the driver sets an alarm time, or
> > > +
> > > +\item when the clock jumps backwards, before the alarm time, or
> > > +
> > > +\item when another alarm expiration event happens.
> > > +\end{itemize}
> > > +
> > > +If an alarm expiration becomes obsolete, it is unspecified which alarm
> > > +actions the device executes for this alarm expiration, and the device
> > > +stops executing these alarm actions after a grace period.
> > 
> > Can't you leave this paragraph implementation-specific and thus
> > removing it completely?
> 
> OK.

It seems complicated to me. If the device does not stop alarm actions
when the driver disables the alarm, it becomes impossible to reliably
clear the device state ("start with a clean slate", as per the
discussion in [2]). For this, v8 has the requirement

	If the driver successfully requests VIRTIO_RTC_REQ_SET_ALARM, or
	VIRTIO_RTC_REQ_SET_ALARM_ENABLED, for clock C, the device MUST
	stop serving any previous alarm expiration event for C before
	the device marks the message as used.

This is the only hard requirement which restricts a v8 implementation,
every other alarm action could still be done within an indefinite "grace
period".

I think the "grace period" related requirements should be dropped, so
that the behavior is essentially implementation-specific apart from
stopping the alarm action when the driver disables the alarm. Maybe I
can change the last paragraph cited from the patch like the following:

	If an alarm expiration becomes obsolete, it is unspecified which
	alarm actions the device executes for this alarm expiration.
	When the driver disables an alarm, the device stops any alarm
	action for this alarm before using the buffer.

Best regards,

Peter

[2] https://lore.kernel.org/virtio-comment/[email protected]/