Thanks, this is a good catch. All three are the same underlying point:
dropping ptp_clk_mutex before the drain lets a work item that was already
queued (or latched by the IRQ just before the compare was disabled) run
icss_iep_cap_cmp_work() after PPS has been turned off, where it re-arms
CMP1 and emits a stale PPS event.
v2 fixes that by guarding the handler: it now returns early, under
ptp_clk_mutex, when neither pps_enabled nor perout_enabled is set, so a
late or stale work does nothing instead of touching the (just-disabled)
compare hardware or emitting an event. That covers the "handler already
blocked on the mutex" case and the "pending work starts between unlock and
cancel" case.
On the concurrent-sysfs window (pps_enable_store / period_store not
serialized against the ioctl path): with the guard, a work cancelled by a
disable that races a fresh enable simply bails if the enable has not yet
set the flag; I don't think this patch can fully close the ptp_sysfs
serialization gap, which predates it.
On the pre-existing perout/teardown issue you flagged (High): you're
right that icss_iep_perout_enable(.., 0) never drains iep->work, and that
icss_iep_exit() clears iep->ptp_clock before the perout disable runs, so a
work queued during perout can still reach ptp_clock_event() on teardown.
The handler guard reduces but does not fully close that, because on the
teardown path perout_enabled can still be set when the work runs. I'd
prefer to fix the perout-disable drain and the exit ordering as a separate
change rather than fold a teardown rework into this deadlock fix — happy to
send it as a follow-up unless you'd rather have it here.
Not tested on real hardware (no TI ICSSG/PRU device); the above is from
code inspection.
pw-bot: cr
2026년 11월 8일 (일) 오후 3:59, [off-list ref]님이 작성:
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 3 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The patch moves cancel_work_sync(&iep->work) out of the
ptp_clk_mutex critical section in icss_iep_pps_enable()…
- [Medium] After the patch, an icss_iep_cap_cmp_work() instance can run
after icss_iep_pps_enable(0) has disabled and cleared the compare…
- [Medium] The commit message says that because the compare is disabled
before the drain, 'no new work can be scheduled once the mutex is…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the shared
iep->work is never drained when periodic output (perout) is disabled,…
--
net: ti: icss-iep: fix deadlock when disabling PPS
This commit moves cancel_work_sync(&iep->work) in icss_iep_pps_enable()
out of the ptp_clk_mutex critical section. The PPS disable path then no
longer waits for icss_iep_cap_cmp_work() while it holds the mutex that the
work handler needs.
quoted
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index ec085897edf09..6264061a4eab3 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
[ ... ]
quoted
@@ -607,7 +608,7 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
} else {
ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
if (iep->cap_cmp_irq)
- cancel_work_sync(&iep->work);
+ drain_work = true;
}
if (!ret)@@ -616,6 +617,15 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
exit:
mutex_unlock(&iep->ptp_clk_mutex);
[Severity: Medium]
Once the mutex is dropped here, can icss_iep_cap_cmp_work() run against
compare hardware that has just been disabled and cleared?
There seem to be two ways for this to happen:
- A handler already blocked in mutex_lock(&iep->ptp_clk_mutex) now
continues. This is the case the patch fixes.
- A work item that was pending but not yet started begins running
between mutex_unlock() and cancel_work_sync(). In that case
cancel_work_sync() only waits for it to finish.
The handler doesn't check pps_enabled or perout_enabled after it takes the
mutex:
icss_iep_cap_cmp_work() {
mutex_lock(&iep->ptp_clk_mutex);
ns = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
...
ns_next = ns + iep->period;
writel(lower_32_bits(ns_next),
iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
...
pevent.pps_times.ts_real = ns_to_timespec64(ns);
pevent.type = PTP_CLOCK_PPSUSR;
pevent.index = 0;
ptp_clock_event(iep->ptp_clock, &pevent);
...
}
icss_iep_perout_enable_hw(on=0) has just written 0 to CMP1_REG0/REG1. The
late handler would then set CMP1 to 0 + iep->period, which undoes the
"clear CMP regs" step. It would also send a PTP_CLOCK_PPSUSR event with a
zero (1970) timestamp.
pps_event() doesn't check whether PPS is enabled. It records the assert,
increments assert_sequence, wakes /dev/ppsN readers and calls
pps_kc_event(), all after userspace has turned PPS off.
Before this patch, the blocked case deadlocked and the pending case was
cancelled under the mutex, so this behaviour is new.
Should icss_iep_cap_cmp_work() return early under the mutex when neither
pps_enabled nor perout_enabled is set?
quoted
+ /*
+ * Drain the capture-compare work only after dropping ptp_clk_mutex.
+ * icss_iep_cap_cmp_work() acquires ptp_clk_mutex itself, so calling
+ * cancel_work_sync() while the mutex is held deadlocks when a handler
+ * instance is already running and waiting for that same mutex.
+ */
+ if (drain_work)
+ cancel_work_sync(&iep->work);
[Severity: Medium]
The drain now runs outside ptp_clk_mutex and doesn't re-check
pps_enabled or perout_enabled. Could it cancel work that belongs to a
later enable?
pps_enable_store() and period_store() in drivers/ptp/ptp_sysfs.c call
ops->enable() without ptp->pincfg_mux. That means they aren't serialized
against the PTP_ENABLE_PPS ioctl path or against each other.
One possible sequence:
CPU0 (disable) CPU1 (sysfs enable)
icss_iep_pps_enable(iep, 0)
pps_enabled = false
mutex_unlock()
icss_iep_pps_enable(iep, 1)
program CMP1, set CMP_EN
pps_enabled = true
first CMP1 hit:
icss_iep_cap_cmp_irq()
schedule_work(&iep->work)
cancel_work_sync(&iep->work)
removes the new session's work
icss_iep_cap_cmp_work() is the only code that moves CMP1 forward. Once
its work is cancelled, no further compare events or PPS pulses are
produced, while the driver still reports pps_enabled == true.
The other order is possible as well. Suppose an old-session work instance
is blocked in mutex_lock() and gets the mutex after CPU1's enable. It
would read the new boundary B from CMP1, write B + period and send a
PPSUSR event stamped B before time B arrives. The first real edge of the
new session would be skipped.
Before this patch the cancel ran under ptp_clk_mutex, so a concurrent
enable had to wait for it.
[Severity: Medium]
The commit message says:
The hardware compare is
already disabled by icss_iep_perout_enable_hw() before the drain, so no new
work can be scheduled once the mutex is released.
Is that guaranteed? icss_iep_cap_cmp_irq() reads pps_enabled with no lock
and no ordering against this path. Nothing calls
synchronize_irq(iep->cap_cmp_irq) before the drain:
icss_iep_cap_cmp_irq() {
val = readl(iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
if (val & BIT(1)) {
writel(BIT(1), iep->base + reg_offs[ICSS_IEP_CMP_STAT_REG]);
if (iep->pps_enabled || iep->perout_enabled)
schedule_work(&iep->work);
...
}
Suppose a CMP1 hit was latched just before CMP_EN was cleared, and its
handler on CPU1 has already read pps_enabled == true.
CPU0 can then set pps_enabled = false, drop the mutex and call
cancel_work_sync(). That call returns immediately because nothing is
pending.
CPU1 then calls schedule_work(), and the work runs after PPS has been
disabled.
On teardown, ptp_clock_unregister() -> ptp_disable_all_events() ->
icss_iep_pps_enable(0) runs first. After that the pps source and
ptp_clock are released, and icss_iep_exit() sets iep->ptp_clock to NULL.
A work item queued this late could call ptp_clock_event() on a freed or
NULL ptp_clock.
On PREEMPT_RT the handler is force-threaded and can be preempted between
the flag read and schedule_work(), so the window is wider there.
The old code had the same race with a larger window. However, the commit
message's argument depends on this window being closed.
Would a synchronize_irq() after clearing pps_enabled, together with a
state check in the work handler, close it?
quoted
[Severity: High]
This isn't a bug introduced by this patch, but the perout disable path has
a related problem. icss_iep_cap_cmp_irq() also queues iep->work when
perout_enabled is set, but icss_iep_perout_enable(..., 0) never drains it:
icss_iep_perout_enable() {
...
ret = icss_iep_perout_enable_hw(iep, req, on);
if (!ret)
iep->perout_enabled = !!on;
exit:
mutex_unlock(&iep->ptp_clk_mutex);
return ret;
}
On teardown, icss_iep_exit() unregisters the clock and sets it to NULL
before perout is turned off:
icss_iep_exit() {
if (iep->ptp_clock) {
ptp_clock_unregister(iep->ptp_clock);
iep->ptp_clock = NULL;
}
icss_iep_disable(iep);
if (iep->pps_enabled)
icss_iep_pps_enable(iep, false);
else if (iep->perout_enabled)
icss_iep_perout_enable(iep, NULL, false);
...
}
The ptp_disable_all_events() call inside ptp_clock_unregister() only
disables EXTTS and PPS. The PPS disable returns -EBUSY while perout is
enabled, so the work isn't drained there either.
Can a work item queued while perout is active run icss_iep_cap_cmp_work()
after this point? It could be queued before teardown, or by an IRQ that
arrives before CMP_EN is cleared. If it runs, ptp_clock_event() would use
a NULL or freed iep->ptp_clock and pps_device.
This can be reached from ndo_stop via prueth_emac_common_stop(), or from
prueth_remove(). It needs an IEP with an iep_cap_cmp interrupt and the
icssg_prueth clockops, which set n_per_out = 1.
Also, iep->ptp_clock = NULL is assigned without holding ptp_clk_mutex.
Should icss_iep_perout_enable() also call cancel_work_sync() on disable?
And should icss_iep_exit() drain iep->work before it unregisters the
clock?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107063726.3388730-1-hj351016%40gmail.com