[PATCH net] net: ti: icss-iep: fix deadlock when disabling PPS

[PATCH net] net: ti: icss-iep: fix deadlock when disabling PPS

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

icss_iep_pps_enable() holds iep->ptp_clk_mutex across the whole function.
When PPS is being disabled it calls cancel_work_sync(&iep->work) while that
mutex is still held.

The work handler, icss_iep_cap_cmp_work(), acquires iep->ptp_clk_mutex at
its start. The capture-compare IRQ (icss_iep_cap_cmp_irq) schedules this
work on every CMP1 event while PPS is enabled, so a handler instance can be
running when userspace disables PPS. In that case cancel_work_sync() blocks
waiting for the handler to finish while holding ptp_clk_mutex, and the
handler blocks trying to acquire the same ptp_clk_mutex -> deadlock.
lockdep reports this as a possible circular locking dependency between
the work and ptp_clk_mutex.

Unlike a teardown path, the capture-compare IRQ is not disabled here, so
the race is reachable at runtime simply by toggling PPS off while the
hardware is generating CMP events.

Drain the work only after dropping ptp_clk_mutex. 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.

Fixes: f18ad402cd8b ("net: ti: icss-iep: Enable compare events")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
---
 drivers/net/ethernet/ti/icssg/icss_iep.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index ec08589..6264061 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -579,6 +579,7 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
 {
 	struct ptp_clock_request rq;
 	struct timespec64 ts;
+	bool drain_work = false;
 	int ret = 0;
 	u64 ns;
 
@@ -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);
 
+	/*
+	 * 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);
+
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH net] net: ti: icss-iep: fix deadlock when disabling PPS

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: ti: icss-iep: fix deadlock when disabling PPS

[netdev-bot+sashiko]

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.

> 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

[ ... ]

> @@ -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?

>  
> +	/*
> +	 * 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?

> +
>  	return ret;
>  }
>  

[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

Re: [PATCH net] net: ti: icss-iep: fix deadlock when disabling PPS

[khj]

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, <netdev-bot+sashiko@kernel.org>님이 작성:
>
> 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.
>
> > 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
>
> [ ... ]
>
> > @@ -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?
>
> >
> > +     /*
> > +      * 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?
>
> > +
> >       return ret;
> >  }
> >
>
> [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

[PATCH net v2] net: ti: icss-iep: fix deadlock when disabling PPS

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

icss_iep_pps_enable() holds iep->ptp_clk_mutex across the whole function.
When PPS is being disabled it calls cancel_work_sync(&iep->work) while that
mutex is still held.

The work handler, icss_iep_cap_cmp_work(), acquires iep->ptp_clk_mutex at
its start. The capture-compare IRQ (icss_iep_cap_cmp_irq) schedules this
work on every CMP1 event while PPS is enabled, so a handler instance can be
running when userspace disables PPS. In that case cancel_work_sync() blocks
waiting for the handler to finish while holding ptp_clk_mutex, and the
handler blocks trying to acquire the same ptp_clk_mutex -> deadlock.
lockdep would report this as a possible circular locking dependency
between the work and ptp_clk_mutex.

Unlike a teardown path, the capture-compare IRQ is not disabled here, so
the race is reachable at runtime simply by toggling PPS off while the
hardware is generating CMP events.

Drain the work only after dropping ptp_clk_mutex, so the disable path no
longer waits for the handler while holding the lock the handler needs.

Dropping the mutex before the drain means a work item that was already
queued (or an IRQ latched just before the compare was disabled) can still
run icss_iep_cap_cmp_work() after PPS/perout has been turned off. Guard the
handler so it returns early, under the mutex, when neither pps_enabled nor
perout_enabled is set, instead of re-arming CMP1 and emitting a stale PPS
event once the event source is gone.

This was found by a static analysis tool -- an in-house shape/pattern
checker that flags cancel_work_sync() calls made while holding a lock the
cancelled work's handler also acquires -- and confirmed by manual review
of the PPS enable/disable path. It was not tested on real hardware: I do
not have a TI ICSSG/PRU (AM65x-family) device, so the deadlock and the
late-work window are established by code inspection rather than an observed
lockdep or KASAN trace.

Fixes: f18ad402cd8b ("net: ti: icss-iep: Enable compare events")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2:
 - Guard icss_iep_cap_cmp_work() to return early, under ptp_clk_mutex, when
   PPS/perout is disabled, so a work item that runs after the mutex is
   dropped no longer re-arms the compare or emits a stale PPS event
   (addresses the late-work windows raised in review of v1).
 - Soften "lockdep reports" -> "lockdep would report" (not an observed
   trace).
 - Record discovery (static analysis + manual inspection) and that the
   change was not tested on real hardware in the commit message, as
   requested by the netdev commit-message bot.

v1: https://lore.kernel.org/netdev/20261107063726.3388730-1-hj351016@gmail.com/
 drivers/net/ethernet/ti/icssg/icss_iep.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index ec08589..3b2a88c 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -534,6 +534,17 @@ static void icss_iep_cap_cmp_work(struct work_struct *work)
 
 	mutex_lock(&iep->ptp_clk_mutex);
 
+	/*
+	 * PPS/perout may have been disabled (and the compare hardware
+	 * cleared) while this work was queued or waiting for the mutex.
+	 * Bail out rather than re-arm CMP1 and emit a stale PPS event once
+	 * the event source is gone.
+	 */
+	if (!iep->pps_enabled && !iep->perout_enabled) {
+		mutex_unlock(&iep->ptp_clk_mutex);
+		return;
+	}
+
 	ns = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG0]);
 	if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) {
 		val = readl(iep->base + reg_offs[ICSS_IEP_CMP1_REG1]);
@@ -579,6 +590,7 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
 {
 	struct ptp_clock_request rq;
 	struct timespec64 ts;
+	bool drain_work = false;
 	int ret = 0;
 	u64 ns;
 
@@ -607,7 +619,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 +628,15 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
 exit:
 	mutex_unlock(&iep->ptp_clk_mutex);
 
+	/*
+	 * Drain the capture-compare work only after dropping ptp_clk_mutex.
+	 * icss_iep_cap_cmp_work() takes ptp_clk_mutex itself, so cancelling it
+	 * under the mutex deadlocks against a handler instance already running
+	 * and waiting for that same mutex.
+	 */
+	if (drain_work)
+		cancel_work_sync(&iep->work);
+
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH net v2] net: ti: icss-iep: fix deadlock when disabling PPS

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · 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] This patch moves cancel_work_sync(&iep->work) in
  icss_iep_pps_enable() out from under ptp_clk_mutex.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a use-
  after-free / NULL dereference in the teardown path that the new guard…

--

net: ti: icss-iep: fix deadlock when disabling PPS

This patch changes icss_iep_pps_enable() so that cancel_work_sync(&iep->work)
runs after ptp_clk_mutex is dropped. This avoids a deadlock with
icss_iep_cap_cmp_work(), which takes the same mutex. It also makes
icss_iep_cap_cmp_work() return early when neither pps_enabled nor
perout_enabled is set.

> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index ec085897edf09..3b2a88c2dad88 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -534,6 +534,17 @@ static void icss_iep_cap_cmp_work(struct work_struct *work)
>  
>  	mutex_lock(&iep->ptp_clk_mutex);
>  
> +	/*
> +	 * PPS/perout may have been disabled (and the compare hardware
> +	 * cleared) while this work was queued or waiting for the mutex.
> +	 * Bail out rather than re-arm CMP1 and emit a stale PPS event once
> +	 * the event source is gone.
> +	 */
> +	if (!iep->pps_enabled && !iep->perout_enabled) {
> +		mutex_unlock(&iep->ptp_clk_mutex);
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but does this check cover teardown when perout
is enabled instead of PPS?

icss_iep_exit() unregisters the ptp clock before it disables perout:

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);
	...
}

Inside ptp_clock_unregister(), ptp_disable_all_events() only sends
PTP_CLK_REQ_PPS. icss_iep_pps_enable(off) then returns -EBUSY because
perout_enabled is set. So CMP1 stays armed and the work is never drained.

ptp_clock_unregister() then unregisters ptp->pps_source and calls
put_device() on the ptp device, which can free the ptp clock. After that,
iep->ptp_clock is set to NULL without holding ptp_clk_mutex.

Later, icss_iep_perout_enable(iep, NULL, false) clears perout_enabled. It
never calls cancel_work_sync().

During this window icss_iep_cap_cmp_irq() can still queue the work.
icss_iep_cap_cmp_work() gets past the new check because perout_enabled is
still 1, and then reaches:

	ptp_clock_event(iep->ptp_clock, &pevent);

For PTP_CLOCK_PPSUSR, ptp_clock_event() calls pps_event(ptp->pps_source, ...)
without a NULL check. Can this touch a freed pps_source or ptp clock, or
dereference a NULL iep->ptp_clock?

On am654 with icssg_prueth (clockops perout_enable), this looks reachable.
Enable perout with PTP_PEROUT_REQUEST, then bring the interface down, which
goes through prueth_emac_common_stop()->icss_iep_exit().

[ ... ]

> @@ -607,7 +619,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 +628,15 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
>  exit:
>  	mutex_unlock(&iep->ptp_clk_mutex);
>  
> +	/*
> +	 * Drain the capture-compare work only after dropping ptp_clk_mutex.
> +	 * icss_iep_cap_cmp_work() takes ptp_clk_mutex itself, so cancelling it
> +	 * under the mutex deadlocks against a handler instance already running
> +	 * and waiting for that same mutex.
> +	 */
> +	if (drain_work)
> +		cancel_work_sync(&iep->work);

[Severity: Medium]
Now that the drain runs outside ptp_clk_mutex, is the disable still ordered
before a following PPS or perout enable?

The chardev ioctls hold ptp->pincfg_mux around ops->enable(). In
drivers/ptp/ptp_sysfs.c, pps_enable_store() and period_store() call
ops->enable() without it. That means a second writer can take ptp_clk_mutex
between the mutex_unlock() and the cancel_work_sync() above:

CPU1                                  CPU2
icss_iep_pps_enable(iep, 0)
  icss_iep_perout_enable_hw(off)
  iep->pps_enabled = 0
  mutex_unlock()
                                      icss_iep_pps_enable(iep, 1)
                                        icss_iep_perout_enable_hw(on)
                                        iep->pps_enabled = 1
                                        mutex_unlock()
  cancel_work_sync(&iep->work)

Two outcomes look possible.

In the first, a stale work item from the old session gets the mutex after
CPU2. It may have been queued by a CMP1 IRQ just before the disable, or it
may already be waiting in mutex_lock(). The new check in
icss_iep_cap_cmp_work() only looks at the flags, so the item gets through.
It reads the new start value that icss_iep_update_to_next_boundary() wrote to
CMP1. It writes start + iep->period back, then reports a PPSUSR event for an
edge that has not happened yet.

Would the first pulse of the new session be skipped, with a bogus timestamp
sent to userspace?

In the second, CPU1 is delayed until the new session's first CMP1 match.
icss_iep_cap_cmp_irq() queues the new session's work, and CPU1's late
cancel_work_sync() removes it before it runs.

In the non-clockops path, this work is the only code that advances CMP1 by
iep->period. icssg_prueth_sr1 is one example, since it calls
icss_iep_init(iep, NULL, NULL, 0).

Could PPS stop for good here while pps_enabled is still true?

Before this patch, ptp_clk_mutex serialized the disable, the drain and the
next enable.

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108074733.248784-1-hj351016%40gmail.com

Re: [PATCH net v2] net: ti: icss-iep: fix deadlock when disabling PPS

[khj]

Thanks for the careful review — both are fair.

On the [Medium]: agreed. Draining outside ptp_clk_mutex is what breaks
the deadlock with icss_iep_cap_cmp_work(), but it does weaken the
enable<->disable serialization that the mutex used to provide, so a
concurrent sysfs enable can now race the disable's drain as you
describe. The deadlock and the ordering are in tension and a clean fix
needs a small restructure rather than just relocating the
cancel_work_sync().

On the pre-existing [High]: agreed, and thanks for spotting it — it is
a separate bug from the deadlock this patch targets. icss_iep_exit()
unregisters (and can free) the PTP clock before the perout path is
torn down, and icss_iep_perout_enable(off) never drains iep->work, so
a late icss_iep_cap_cmp_work() can reach ptp_clock_event()/pps_event()
on a freed or NULL iep->ptp_clock. That deserves its own fix (drain
the work in the perout teardown path / order the unregister after the
disable), which I'll send separately rather than fold into this
deadlock fix.

Since this is hardware I cannot test (am654 icssg PRU) and the clean
fix touches the PPS/perout teardown and the ptp_clk_mutex discipline,
I would appreciate the maintainers' preferred direction before I
respin — in particular whether to re-serialize the
enable/disable/drain differently or to move the drain into
icss_iep_exit()/perout teardown.

pw-bot: cr

2026년 11월 9일 (월) 오후 5:21, <netdev-bot+sashiko@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (1) · Medium: 1 · 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] This patch moves cancel_work_sync(&iep->work) in
>   icss_iep_pps_enable() out from under ptp_clk_mutex.
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it is a use-
>   after-free / NULL dereference in the teardown path that the new guard…
>
> --
>
> net: ti: icss-iep: fix deadlock when disabling PPS
>
> This patch changes icss_iep_pps_enable() so that cancel_work_sync(&iep->work)
> runs after ptp_clk_mutex is dropped. This avoids a deadlock with
> icss_iep_cap_cmp_work(), which takes the same mutex. It also makes
> icss_iep_cap_cmp_work() return early when neither pps_enabled nor
> perout_enabled is set.
>
> > diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> > index ec085897edf09..3b2a88c2dad88 100644
> > --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> > +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> > @@ -534,6 +534,17 @@ static void icss_iep_cap_cmp_work(struct work_struct *work)
> >
> >       mutex_lock(&iep->ptp_clk_mutex);
> >
> > +     /*
> > +      * PPS/perout may have been disabled (and the compare hardware
> > +      * cleared) while this work was queued or waiting for the mutex.
> > +      * Bail out rather than re-arm CMP1 and emit a stale PPS event once
> > +      * the event source is gone.
> > +      */
> > +     if (!iep->pps_enabled && !iep->perout_enabled) {
> > +             mutex_unlock(&iep->ptp_clk_mutex);
> > +             return;
> > +     }
>
> [Severity: High]
> This is a pre-existing issue, but does this check cover teardown when perout
> is enabled instead of PPS?
>
> icss_iep_exit() unregisters the ptp clock before it disables perout:
>
> 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);
>         ...
> }
>
> Inside ptp_clock_unregister(), ptp_disable_all_events() only sends
> PTP_CLK_REQ_PPS. icss_iep_pps_enable(off) then returns -EBUSY because
> perout_enabled is set. So CMP1 stays armed and the work is never drained.
>
> ptp_clock_unregister() then unregisters ptp->pps_source and calls
> put_device() on the ptp device, which can free the ptp clock. After that,
> iep->ptp_clock is set to NULL without holding ptp_clk_mutex.
>
> Later, icss_iep_perout_enable(iep, NULL, false) clears perout_enabled. It
> never calls cancel_work_sync().
>
> During this window icss_iep_cap_cmp_irq() can still queue the work.
> icss_iep_cap_cmp_work() gets past the new check because perout_enabled is
> still 1, and then reaches:
>
>         ptp_clock_event(iep->ptp_clock, &pevent);
>
> For PTP_CLOCK_PPSUSR, ptp_clock_event() calls pps_event(ptp->pps_source, ...)
> without a NULL check. Can this touch a freed pps_source or ptp clock, or
> dereference a NULL iep->ptp_clock?
>
> On am654 with icssg_prueth (clockops perout_enable), this looks reachable.
> Enable perout with PTP_PEROUT_REQUEST, then bring the interface down, which
> goes through prueth_emac_common_stop()->icss_iep_exit().
>
> [ ... ]
>
> > @@ -607,7 +619,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 +628,15 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
> >  exit:
> >       mutex_unlock(&iep->ptp_clk_mutex);
> >
> > +     /*
> > +      * Drain the capture-compare work only after dropping ptp_clk_mutex.
> > +      * icss_iep_cap_cmp_work() takes ptp_clk_mutex itself, so cancelling it
> > +      * under the mutex deadlocks against a handler instance already running
> > +      * and waiting for that same mutex.
> > +      */
> > +     if (drain_work)
> > +             cancel_work_sync(&iep->work);
>
> [Severity: Medium]
> Now that the drain runs outside ptp_clk_mutex, is the disable still ordered
> before a following PPS or perout enable?
>
> The chardev ioctls hold ptp->pincfg_mux around ops->enable(). In
> drivers/ptp/ptp_sysfs.c, pps_enable_store() and period_store() call
> ops->enable() without it. That means a second writer can take ptp_clk_mutex
> between the mutex_unlock() and the cancel_work_sync() above:
>
> CPU1                                  CPU2
> icss_iep_pps_enable(iep, 0)
>   icss_iep_perout_enable_hw(off)
>   iep->pps_enabled = 0
>   mutex_unlock()
>                                       icss_iep_pps_enable(iep, 1)
>                                         icss_iep_perout_enable_hw(on)
>                                         iep->pps_enabled = 1
>                                         mutex_unlock()
>   cancel_work_sync(&iep->work)
>
> Two outcomes look possible.
>
> In the first, a stale work item from the old session gets the mutex after
> CPU2. It may have been queued by a CMP1 IRQ just before the disable, or it
> may already be waiting in mutex_lock(). The new check in
> icss_iep_cap_cmp_work() only looks at the flags, so the item gets through.
> It reads the new start value that icss_iep_update_to_next_boundary() wrote to
> CMP1. It writes start + iep->period back, then reports a PPSUSR event for an
> edge that has not happened yet.
>
> Would the first pulse of the new session be skipped, with a bogus timestamp
> sent to userspace?
>
> In the second, CPU1 is delayed until the new session's first CMP1 match.
> icss_iep_cap_cmp_irq() queues the new session's work, and CPU1's late
> cancel_work_sync() removes it before it runs.
>
> In the non-clockops path, this work is the only code that advances CMP1 by
> iep->period. icssg_prueth_sr1 is one example, since it calls
> icss_iep_init(iep, NULL, NULL, 0).
>
> Could PPS stop for good here while pps_enabled is still true?
>
> Before this patch, ptp_clk_mutex serialized the disable, the drain and the
> next enable.
>
> > +
> >       return ret;
> >  }
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108074733.248784-1-hj351016%40gmail.com