Re: [PATCH v3 2/4] ptp: vmclock: support device notifications

[David Woodhouse]

On Wed, 2025-12-03 at 12:36 +0000, Chalios, Babis wrote:
> From: "Chalios, Babis" <bchalios@xxxxxxxxx>
> 
> Add optional support for device notifications in VMClock. When
> supported, the hypervisor will send a device notification every time it
> updates the seq_count to a new even value.
> 
> Moreover, add support for poll() in VMClock as a means to propagate this
> notification to user space. poll() will return a POLLIN event to
> listeners every time seq_count changes to a value different than the one
> last seen (since open() or last read()/pread()). This means that when
> poll() returns a POLLIN event, listeners need to use read() to observe
> what has changed and update the reader's view of seq_count. In other
> words, after a poll() returned, all subsequent calls to poll() will
> immediately return with a POLLIN event until the listener calls read().
> 
> The device advertises support for the notification mechanism by setting
> flag VMCLOCK_FLAG_NOTIFICATION_PRESENT in vmclock_abi flags field. If
> the flag is not present the driver won't setup the ACPI notification
> handler and poll() will always immediately return POLLHUP.
> 
> Signed-off-by: Babis Chalios <bchalios@xxxxxxxxx>

Mostly looks good to me; thanks. However... 

> +static __poll_t vmclock_miscdev_poll(struct file *fp, poll_table *wait)
> +{
> +	struct vmclock_file_state *fst = fp->private_data;
> +	struct vmclock_state *st = fst->st;
> +	uint32_t seq;
> +
> +	/*
> +	 * Hypervisor will not send us any notifications, so fail immediately
> +	 * to avoid having caller sleeping for ever.
> +	 */
> +	if (!(st->clk->flags & VMCLOCK_FLAG_NOTIFICATION_PRESENT))
> +		return POLLHUP;

Missing le64_to_cpu() there. And I guess *strictly* speaking we should
do the seq_lock dance whenever we read the dynamic fields, although
that only ever matters if a hypervisor were to bump seq_count to an odd
value, *then* set the VMCLOCK_FLAG_NOTIFICATION_PRESENT flag, then
clear the flag again before bumping seq_count to even, and blame the
*guest* for looking at the wrong time. Which is frankly insane, and I
don't think I care.

So with the le64_to_cpu() added, 

Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>

Don't post again for a few days though; three versions in 48 hours is
enough :)


> +static int vmclock_miscdev_release(struct inode *inode, struct file *fp)
> +{
> +	kfree(fp->private_data);
> +	return 0;
> +}

Still bugs me a little. I note simple_attr_release() is exported and
does the same, but I guess we'd want to move and rename that before we
try to use it from places like this.

Attachment: smime.p7s
Description: S/MIME cryptographic signature