Hi Jie,
Thanks for fixing! It is shame that my single patch caused issues both
in driver's probe and remove - I have to admit that I don't understand
runtime PM's state machine.
TBC, this patch only fixes probe. The driver's remove is fixed in:
https://lore.kernel.org/linux-arm-kernel/20260710-fix-clock-refcount-unbala…
The series above and this patch should be picked up together so can
have complete fix.
> The probe wrapper then unconditionally calls pm_runtime_put()
> regardless of whether the inner probe succeeded, so on failure this
> also fires runtime_suspend() and disables the same clocks a first
> time.
pm_runtime_put() can be used for success case, but for the failure
case, we should disable the runtime PM but not release reference:
https://docs.kernel.org/power/runtime_pm.html#runtime-pm-initialization-dev…
> @@ -632,11 +632,14 @@ static int catu_platform_probe(struct platform_device *pdev)
> pm_runtime_enable(&pdev->dev);
>
> ret = __catu_probe(&pdev->dev, res);
> - pm_runtime_put(&pdev->dev);
> - if (ret)
> + if (ret) {
> + pm_runtime_put_noidle(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
Nitpick: please reverse the sequence between pm_runtime_put_noidle()
and pm_runtime_disable(). As we need to first disable runtime PM for
the device, then release usage reference.
Since the driver core will reset device's active state, AI told me that
calling pm_runtime_set_suspended() is redundant. It is still good to
explicitly call it for bookkeeping. This can be aligned with the change
in driver remove.
Thus, please update the flow:
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(&pdev->dev);
With the update:
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Tue, Aug 25, 2026 at 10:06:47AM +0800, yingchao wrote:
> From: Yingchao Deng <dengyingchao(a)kylinsec.com.cn>
>
> tpdm_probe() initializes drvdata->spinlock after coresight_register(), but
> the sysfs attributes registered by coresight_register() use the spinlock.
> This exposes a window where a concurrent sysfs write can lock an
> uninitialized spinlock.
>
> Initialize the spinlock before coresight_register().
>
> Fixes: b3c71626a933 ("Coresight: Add coresight TPDM source driver")
> Signed-off-by: Yingchao Deng <dengyingchao(a)kylinsec.com.cn>
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Tue, Aug 25, 2026 at 09:47:17AM +0800, yingchao wrote:
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 2bfdd7b45e49..d0e7e4720e46 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -1210,8 +1210,14 @@ static int cscfg_create_device(void)
> dev->init_name = "cs_system_cfg";
>
> err = device_register(dev);
> - if (err)
> + if (err) {
> + /* put_device() triggers cscfg_dev_release() which takes
> + * cscfg_mutex, so drop the lock first to avoid deadlocking.
> + */
> + mutex_unlock(&cscfg_mutex);
> put_device(dev);
> + return err;
> + }
As module init and exit are serialized by the kernel, I don't think we
need the mutex to protect the allocation and freeing of cscfg_mgr.
The mutex should only be used for exclusively access cscfg_mgr.
So how about the change below?
---8<---
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index 2bfdd7b45e49..2dd0b29f44e4 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -1173,27 +1173,21 @@ struct device *cscfg_device(void)
/* Must have a release function or the kernel will complain on module unload */
static void cscfg_dev_release(struct device *dev)
{
- mutex_lock(&cscfg_mutex);
kfree(cscfg_mgr);
cscfg_mgr = NULL;
- mutex_unlock(&cscfg_mutex);
}
/* a device is needed to "own" some kernel elements such as sysfs entries. */
static int cscfg_create_device(void)
{
struct device *dev;
- int err = -ENOMEM;
-
- mutex_lock(&cscfg_mutex);
- if (cscfg_mgr) {
- err = -EINVAL;
- goto create_dev_exit_unlock;
- }
+ int err;
cscfg_mgr = kzalloc_obj(struct cscfg_manager);
if (!cscfg_mgr)
- goto create_dev_exit_unlock;
+ return -ENOMEM;
+
+ mutex_lock(&cscfg_mutex);
/* initialise the cscfg_mgr structure */
INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list);
@@ -1204,6 +1198,8 @@ static int cscfg_create_device(void)
cscfg_mgr->load_state = CSCFG_NONE;
raw_spin_lock_init(&cscfg_mgr->sysfs_store_lock);
+ mutex_unlock(&cscfg_mutex);
+
/* setup the device */
dev = cscfg_device();
dev->release = cscfg_dev_release;
@@ -1213,8 +1209,6 @@ static int cscfg_create_device(void)
if (err)
put_device(dev);
-create_dev_exit_unlock:
- mutex_unlock(&cscfg_mutex);
return err;
}
On Fri, Sep 04, 2026 at 04:40:51PM +0530, Hemanth Selam wrote:
> Fix typos in comments, reported by scripts/checkpatch.pl using the
> misspelling list in scripts/spelling.txt. Only touches comments, no code
> changes.
>
> Assisted-by: Cursor:claude-opus-5
> Signed-off-by: Hemanth Selam <hemanth.selam(a)gmail.com>
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Fri, Sep 04, 2026 at 09:49:42AM +0530, tspamiitesh(a)gmail.com wrote:
[...]
> -Description: (RW) Enable/disable tracing on this specific trace entiry.
> +Description: (RW) Enable/disable tracing on this specific trace entry.
> Enabling a source implies the source has been configured
> - properly and a sink has been identidifed for it. The path
> + properly and a sink has been indentified for it. The path
s/indentified/identified ?
On Wed, Sep 02, 2026 at 05:41:43PM +0800, Jie Gan wrote:
> For a tnoc device not on the AMBA bus, atid was set to -EOPNOTSUPP.
> trace_noc_id() returns this value directly to
> coresight_path_assign_trace_id(), which only treats a literal 0
> return as "this device has no ID, keep searching the path" -
> any other value is checked against IS_VALID_CS_TRACE_ID() and
> rejected. A negative atid therefore made path assignment fail with
> -EINVAL instead of falling through to the next device in the path
> that could supply a valid trace ID.
>
> Use 0, the same sentinel coresight_path_assign_trace_id() already
> recognizes as "not allocated", instead of -EOPNOTSUPP.
>
> Fixes: 5799dee92dc2 ("coresight-tnoc: add platform driver to support Interconnect TNOC")
> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
On Wed, Sep 02, 2026 at 04:35:11PM +0800, Jie Gan wrote:
[...]
> Hi Leo,
>
> Thanks for the suggestion.
You're welcome!
> I will fix this in the TNOC driver. I agree that there is no need to use
> additional error codes to report the failure.
>
> I will post the new fix patch after below patch[1] to be applied to avoid
> conflict.
The patch in the link has no fix tag, but current patch is a simple fix.
I'd suggest to give priority this patch - this is friendly for porting
to stable kernels.
If there have dependency (based on your local test), you could explictly
mention the dependency in cover letter and resend the tnoc probe series.
Thanks,
Leo
> [1] https://lore.kernel.org/all/20260710-fix-tracenoc-probe-issue-v6-2-41eb36fe…
Hi Jie,
On Mon, Aug 17, 2026 at 04:50:15PM +0800, Jie Gan wrote:
[...]
> @@ -953,8 +953,12 @@ int coresight_path_assign_trace_id(struct coresight_path *path,
> /* Assign a trace ID to the path for the first device that wants to do it */
> trace_id = coresight_get_trace_id(nd->csdev, mode, sink);
>
> - /* 0 means the device has no ID assignment, so keep searching */
> - if (trace_id == 0)
> + /*
> + * 0 means the device has no ID assignment, and -EOPNOTSUPP
> + * means the device explicitly declines to assign one (e.g. a
> + * pass-through NoC) - in both cases keep searching downstream.
> + */
> + if (trace_id == 0 || trace_id == -EOPNOTSUPP)
> continue;
Based on IS_VALID_CS_TRACE_ID(), I see 0 is for no ID assignment,
could you improve a bit tnoc.c instead?
If so, We don't need to add a new error for the same purpose.
---8<---
diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
index 9e8de4323d28..bf221c1e5c48 100644
--- a/drivers/hwtracing/coresight/coresight-tnoc.c
+++ b/drivers/hwtracing/coresight/coresight-tnoc.c
@@ -51,8 +51,8 @@ static void trace_noc_enable_hw(struct trace_noc_drvdata *drvdata)
{
u32 val;
- /* No valid ATID, simply enable the unit */
- if (drvdata->atid == -EOPNOTSUPP) {
+ /* 0 means no ID assignment, simply enable the unit */
+ if (!drvdata->atid) {
writel(TRACE_NOC_CTRL_PORTEN, drvdata->base + TRACE_NOC_CTRL);
return;
}
@@ -130,10 +130,8 @@ static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
{
int atid;
- if (!dev_is_amba(drvdata->dev)) {
- drvdata->atid = -EOPNOTSUPP;
+ if (!dev_is_amba(drvdata->dev))
return 0;
- }
atid = coresight_trace_id_get_system_id();
if (atid < 0)
On Mon, Aug 10, 2026 at 04:10:48PM +0100, Will Deacon wrote:
> On Mon, Aug 10, 2026 at 03:44:42PM +0100, Leo Yan wrote:
> > Commit 18049c8cff9c ("perf/aux: Allocate non-contiguous AUX pages by
> > default") made the AUX allocator use order-0 pages by default unless a
> > PMU explicitly asks for contiguous allocations.
>
> But that commit specifically calls out SPE as benefitting from
> non-contiguous pages:
>
> "For instance, ARM SPE and TRBE operate with virtual pages, and
> Coresight ETR allocates a separate buffer. For these PMUs,
> allocating contiguous AUX pages unnecessarily exacerbates memory
> fragmentation. This fragmentation can prevent their use on
> long-running devices."
>
> so why doesn't passing PERF_PMU_CAP_AUX_PREFER_LARGE reintroduce the
> problems that 18049c8cff9c was trying to solve?
The question is how "allocating contiguous AUX pages unnecessarily
exacerbates memory fragmentation." The relevant information I could find
is [1]:
"On Android, we collect ETM data periodically on internal user devices
for AutoFDO optimization (for both userspace libraries and the
kernel). Allocating a large chunk of contiguous AUX pages (4M for each
CPU) periodically is almost unbearable. The kernel may need to kill
many processes to fulfill the request. It affects user experience even
after using PMU."
We might have missed chance to clarify how the fragmentation issue
occurs in the first place. Let's say, a phone with 8 CPUs, allocating
4MB per CPU requires 32MB in total, which is a relatively small
portion of 4GiB or 8GiB of RAM commonly found in phones. Moreover, once
contiguous pages are freed, the buddy allocator can coalesce them
again into buddy list. It is not obvious to me that PREFER_LARGE
directly causes fragmentation.
One case where AUX allocation could exacerbate fragmentation is when the
system is already fragmented. If a high-order allocation fails and the
allocator falls back to smaller-order blocks, those allocations may
consume free blocks scattered across different buddy regions and make
subsequent high-order allocations more difficult.
If this is the main concern, I'd suggest using a smaller AUX buffer
(e.g. 1MB or even 512KB) for TRBE/SPE to reduce memory pressure.
Snapshot mode '-S' could also be considered, as it allows the buffer to
be allocated once and reused for subsequent recordings by signals.
OTOH, using only order-0 pages can significantly increase TTW overhead
on the trace path and lead to overflows, we observe this causes huge
trace discontinuity. In the end, we need to trace-off the fragmentation
concern against the trace discontinuity.
Thanks,
Leo
[1] https://lore.kernel.org/lkml/CALJ9ZPNLgEBxOmDim-vztUknEETwdL-Z2gJ8K9s44TiPg…