[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