Re: [PATCH net] ptp: Report clock removal to poll waiters

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102122340.1160733-1-thnkslprpt@xxxxxxxxx?part=1
---
ptp: Report clock removal to poll waiters

The patch adds a check of ptp->defunct to ptp_poll(), placed after
poll_wait() has registered on tsev_wq. ptp_poll() now returns EPOLLERR
when the clock is being removed, so a poll waiter that consumes the
removal wakeup no longer stays blocked until timeout.

> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index f63b45f5ee6ae..01adc70ad97b4 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -593,6 +593,9 @@ __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
>
>  	poll_wait(fp, &ptp->tsev_wq, wait);
>
> +	if (READ_ONCE(ptp->defunct))
> +		return EPOLLERR;
> +

[Severity: Low]
This isn't a bug, but should the other accesses to ptp->defunct be
annotated to match this new READ_ONCE()?

The only writer still uses a plain store, so this load has no
WRITE_ONCE() to pair with:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
    ...
	ptp->defunct = 1;
	wake_up_interruptible(&ptp->tsev_wq);
    ...
}

The other lockless reader in this file, ptp_read(), still uses plain
loads:

drivers/ptp/ptp_chardev.c:ptp_read() {
    ...
	if (wait_event_interruptible(ptp->tsev_wq, ptp->defunct || queue_cnt(queue)))
		return -ERESTARTSYS;

	if (ptp->defunct)
		return -ENODEV;
    ...
}

After this patch, ptp_poll() and ptp_read() read the same flag in two
different ways. Other code in this subsystem pairs its annotations
explicitly. For example, queue_cnt() in ptp_private.h notes that its
READ_ONCE() is "Paired with WRITE_ONCE() in
enqueue_external_timestamp(), ptp_read(), extts_fifo_show()".

Would it be more consistent to use WRITE_ONCE(ptp->defunct, 1) in
ptp_clock_unregister() and READ_ONCE() in ptp_read()? The other option
is to leave the new check unannotated like the existing ones.

>  	return queue_cnt(queue) ? EPOLLIN : 0;
>  }
-- 
pw-bot: changes-requested