Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: MOHAMED AYMAN <mohamedaymanworkspace@gmail.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mike Leach <mike.leach@arm.com>,
	James Clark <james.clark@linaro.org>, Leo Yan <leo.yan@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Clark Williams <clrkwllms@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	"moderated list:ARM/CORESIGHT FRAMEWORK AND DRIVERS"
	<coresight@lists.linaro.org>,
	"moderated list:ARM/CORESIGHT FRAMEWORK AND DRIVERS"
	<linux-arm-kernel@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:Real-time Linux (PREEMPT_RT):Keyword:PREEMPT_RT"
	<linux-rt-devel@lists.linux.dev>
Subject: Re: [PATCH v2] coresight: Fix scheduling while atomic in coresight_put_percpu_source_ref()
Date: Wed, 15 Jul 2026 08:58:32 +0200	[thread overview]
Message-ID: <20260715065832.qI7c2Px5@linutronix.de> (raw)
In-Reply-To: <CABWZVho1=oJHU6U16Kc-Nh1bQbn07oz7PPkM9Diy=JS0beGjFQ@mail.gmail.com>

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


      reply	other threads:[~2026-07-15  6:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-12 21:04 [PATCH] coresight: Fix scheduling while atomic in coresight_device_release() Mohamed Ayman
2026-07-13  3:07 ` Jie Gan
2026-07-13 23:00 ` [PATCH v2] coresight: Fix scheduling while atomic in coresight_put_percpu_source_ref() Mohamed Ayman
2026-07-14 10:42   ` Sebastian Andrzej Siewior
2026-07-14 19:42     ` MOHAMED AYMAN
2026-07-15  6:58       ` Sebastian Andrzej Siewior [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260715065832.qI7c2Px5@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=clrkwllms@kernel.org \
    --cc=coresight@lists.linaro.org \
    --cc=james.clark@linaro.org \
    --cc=leo.yan@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-devel@lists.linux.dev \
    --cc=mike.leach@arm.com \
    --cc=mohamedaymanworkspace@gmail.com \
    --cc=rostedt@goodmis.org \
    --cc=suzuki.poulose@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox