Re: [PATCH v2] coresight: Fix scheduling while atomic in coresight_put_percpu_source_ref()

[MOHAMED AYMAN]

Hi Sebastian,

First Thing WOOOOOOOW, This is a brilliant observation and entirely
changes the approach.

To answer your question regarding how the reference drops to 0 in the
PM notifier:
You are exactly right that the device should have a base reference.
The only way `put_device()` inside the CPU_PM notifier drops the lastt
reference (reaching 0) is if there is a race condition. If another CPU
concurrently calls `coresight_unregister()`, it drops the base
reference. That leaves our CPU's PM notifier holding the absolute last
reference. When our PM notifier calls `put_device()`, it inadvertently
triggers the release cascade from an atomic context.

As you correctly pointed out, `coresight_cpu_get_active_path()` is the
root cause. It lazily reuses `coresight_get_percpu_source_ref()`,
which takes a kobject reference (`get_device()`) intended for path
building.

Since `coresight_cpu_pm_notify()` runs with local IRQs disabled, we do
not need to manipulate the kobject refcount at all. We can simply read
the per-cpu pointer under `coresight_dev_lock`, check the mode, and
return the path.

If we avoid the `get_device()` / `put_device()` entirely in the PM
path, we completely eliminate the bug at its source. We can throw away
all the complex workqueue and pending counter logic from v1/v2.

The fix would simply be rewriting `coresight_cpu_get_active_path()` like this:

static struct coresight_path *coresight_cpu_get_active_path(enum cs_mode mode)
{
struct coresight_device *source;
struct coresight_path *path = NULL;

guard(raw_spinlock_irqsave)(&coresight_dev_lock);

source = per_cpu(csdev_source, smp_processor_id());
if (source && (coresight_get_mode(source) & mode))
path = source->path;

return path;
}

This means we don't need `coresight_put_percpu_source_ref()` inside
the PM path at all.

Does this align with your suggestion !??
If so, I will prepare a v3 patch that drops the workqueue architecture
entirely and just applies this much simpler fix..

Best regards,
Mohamed Ayman

On Wed, Jul 15, 2026 at 9:58 AM Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> On 2026-07-14 22:42:12 [+0300], MOHAMED AYMAN wrote:
> > Hi Sebastian,
> Hi Mohamed,
>
> > Thank you for the review and the feedback.
> >
> > Regarding the commit message:
> > You are completely right. I will update the wording in the v4 patch to
> > explicitly state that it "uses a spinlock_t for locking which becomes
> > a sleeping lock on PREEMPT_RT" instead of calling it an rt_mutex
> > directly.
> >
> > Regarding the module-ref counter:
> > We don't explicitly bump the module reference count for each new
> > device. Instead, we rely on `destroy_workqueue(coresight_wq)` inside
> > `coresight_exit()`. `destroy_workqueue()` synchronously drains all
> > pending work items before returning, which ensures no deferred puts
> > are executed after the module is unmapped.
>
> But what stops the module unload before all devices are released?
>
> > Regarding deferring coresight_device_release() vs put_device():
> > My initial v1 patch did exactly what you suggested, it only deferred
> > the body of `coresight_device_release()`. However, it was pointed out
> > that `put_device()` synchronously recurses into `kobject_cleanup()`,
> > which invokes the child's release function and immediately afterwards
> > calls `kobject_put(parent)`.
> >
> > If we only defer the child's release callback, the `put_device()` call
> > itself will still execute in the atomic CPU_PM notifier context. If
> > the parent device's release path acquires any sleeping locks, we will
> > still hit a "scheduling while atomic" panic. Deferring `put_device()`
> > entirely protects against this parent cascade.
> >
> > If it is strictly guaranteed that Coresight parent devices (liike
> > AMBA) will never sleep during their release paths, I can happily
> > revert to the simpler approach of just deferring
> > `coresight_device_release()`.
> >
> > Would you prefer I revert to deferring just the release function, or
> > keep the current architecture to safeguard the parent put?
>
> Well deferring as I suggested if the kobj goes away is a not good.
> That I part I didn't get: You have the call chain:
> |  coresight_cpu_pm_notify() (IRQs off)
> |    -> coresight_put_percpu_source_ref()
> |      -> put_device()
> |        -> coresight_device_release()
> |          -> free_percpu()
>
>
> What you skipped is coresight_cpu_get_active_path() and this one has a
> get and a put. Your put has a irqlock on coresight_dev_lock which I am
> not sure you need. But more importantly, why is the reference going back
> to 0? There would have to be a coresight_clear_percpu_source() in
> between, right?
>
> If you could avoid grabbing a reference in the coresight_cpu_pm_notify()
> path then we wouldn't have that problem or is there more to it?
>
> > Best regards,,
> > Mohamed Ayman
>
> Sebastian