All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leo Yan <leo.yan@arm.com>
To: James Clark <james.clark@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mike Leach <mike.leach@arm.com>, Suyash Mahar <smahar@meta.com>,
	Yeoreum Yun <yeoreum.yun@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Qi Liu <liuqi115@huawei.com>, Junhao He <hejunhao3@huawei.com>,
	coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
	sashiko-bot <sashiko-bot@kernel.org>
Subject: Re: [PATCH v3 2/8] coresight: configfs: Don't assume active until cscfg_mgr is set
Date: Thu, 13 Aug 2026 10:09:24 +0100	[thread overview]
Message-ID: <20260813090924.GA8904@e132581.arm.com> (raw)
In-Reply-To: <20260728-james-cs-multiple-per-threads-v3-2-6aee7579f1dc@linaro.org>

On Tue, Jul 28, 2026 at 04:00:14PM +0100, James Clark wrote:
> Perf's etm_setup_aux() calls cscfg_activate_config() and dereferences
> cscfg_mgr, which can be done after coresight_init() has initialized the
> Perf PMU but before cscfg_init() has finished while the driver is still
> loading.

For this patch, if Perf PMU is dependent on cscfg, why we cannot
adjust the init sequence that first call cscfg_init(), then register
cs_etm PMU event?

In _cscfg_activate_config(), I can see it dereferences cscfg_mgr:

  if (cscfg_mgr->load_state == CSCFG_UNLOAD)

This patch does not check if cscfg_mgr is set. If we try to check
cscfg_mgr every time before dereferences it, this approach is quite
fragile.


> Fix it by only assuming cscfg is initialized if cscfg_mgr is set.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Fixes: a0114b4740dd ("coresight: etm-perf: Update to activate selected configuration")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-syscfg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 2bfdd7b45e49..475e8fefa100 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -581,7 +581,7 @@ int cscfg_load_config_sets(struct cscfg_config_desc **config_descs,
>  	int err = 0;
>  
>  	mutex_lock(&cscfg_mutex);
> -	if (cscfg_mgr->load_state != CSCFG_NONE) {
> +	if (!cscfg_mgr || cscfg_mgr->load_state != CSCFG_NONE) {
>  		mutex_unlock(&cscfg_mutex);
>  		return -EBUSY;

If cscfg_mgr is NULL, would directly bail out? This can avoid adding
check for every dereference.

Thanks,
Leo

P.S. For me, this kind of rare race condition reported by Sashiko can
sometimes distract from the main issue. Trying to address it in the
same series may mix different issues together. I miss the simpler
approach where issues could be resolved with clear boundary.

  reply	other threads:[~2026-08-13  9:09 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-28 15:00 [PATCH v3 0/8] coresight: Prevent per-thread events from sharing a sink James Clark
2026-07-28 15:00 ` [PATCH v3 1/8] coresight: tmc-etr: Don't stop Perf cleanup for active sysfs reads James Clark
2026-08-12 16:45   ` Leo Yan
2026-08-13  9:02     ` James Clark
2026-07-28 15:00 ` [PATCH v3 2/8] coresight: configfs: Don't assume active until cscfg_mgr is set James Clark
2026-08-13  9:09   ` Leo Yan [this message]
2026-08-13  9:24     ` James Clark
2026-07-28 15:00 ` [PATCH v3 3/8] coresight: etm-perf: Flush workqueue before unloading module James Clark
2026-08-13 14:19   ` Leo Yan
2026-07-28 15:00 ` [PATCH v3 4/8] coresight: tmc-etr: Prevent per-thread events from sharing a sink James Clark
2026-08-13 16:05   ` Leo Yan
2026-07-28 15:00 ` [PATCH v3 5/8] coresight: tmc-etr: Use session ID for buffer ownership James Clark
2026-07-28 15:00 ` [PATCH v3 6/8] coresight: tmc-etf: Prevent per-thread events from sharing a sink James Clark
2026-07-28 15:00 ` [PATCH v3 7/8] coresight: etb10: " James Clark
2026-07-28 15:00 ` [PATCH v3 8/8] coresight: ultrasoc-smb: " James Clark

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=20260813090924.GA8904@e132581.arm.com \
    --to=leo.yan@arm.com \
    --cc=coresight@lists.linaro.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hejunhao3@huawei.com \
    --cc=james.clark@linaro.org \
    --cc=jic23@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liuqi115@huawei.com \
    --cc=mike.leach@arm.com \
    --cc=sashiko-bot@kernel.org \
    --cc=smahar@meta.com \
    --cc=suzuki.poulose@arm.com \
    --cc=yeoreum.yun@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.