From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id A43193F0A81 for ; Fri, 15 May 2026 10:36:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778841415; cv=none; b=sxPyk7ca3jTrLtXZfPxj0cvPJe2xqHk/CiWPVuzMiIdsfZWd/ipcVALkt94nkt3qGBimTNHxgTIkVej9/q1ei7bMu6Zt+Qk/gNvu7mJuHaa0GmSGe7EU3F3KXYoX2L91ZoNK1FZsjxyv1M5QaR7JDl6eFjXvTO9pAtA32aQgKAg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778841415; c=relaxed/simple; bh=DcaLujhD8GzqFZWF7Lsqg/uxds9v5IVU/HlbuL0JRSg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dD4dP68FrpDhzrpiFyK7y7mXwE18zViaBMC06M1cpxV2mTn7NO+Ys051cUSJvO7Yc2kVSfIV84RTFQgG/RpMoTJXvELsrOJ2leX3IeDun+S0rgBi3eD2hQdyyTxhrdTEbtsjmqsLkEAw1CVwz41lC3aY0sVXUT5ojHlavI3ESag= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=aWwoPWje; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="aWwoPWje" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 480EB22FC; Fri, 15 May 2026 03:36:47 -0700 (PDT) Received: from e129823.arm.com (e129823.arm.com [10.1.197.6]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CF6AF3F7B4; Fri, 15 May 2026 03:36:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1778841412; bh=DcaLujhD8GzqFZWF7Lsqg/uxds9v5IVU/HlbuL0JRSg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=aWwoPWjeGqtY8G7nPdGF/nrC/wpJdCpyDgdDuAKCsYb+C7l+UQxGxSJNaJf6qNosq bx3pymVu+Yh73m9i+bcKuDTg/hqS4j4HxFkZB+9DJr8QwwLTp7zq3+luuAW2DfqO30 fpn6SbE2rH23OGrY+dj7B88YLy9znpRXVdfOwcz0= Date: Fri, 15 May 2026 11:36:48 +0100 From: Yeoreum Yun To: Leo Yan Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, suzuki.poulose@arm.com, mike.leach@arm.com, james.clark@linaro.org, alexander.shishkin@linux.intel.com, jie.gan@oss.qualcomm.com Subject: Re: [PATCH v6 07/13] coresight: etm4x: fix inconsistencies with sysfs configuration Message-ID: References: <20260422132203.977549-1-yeoreum.yun@arm.com> <20260422132203.977549-8-yeoreum.yun@arm.com> <20260515085354.GH34802@e132581.arm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260515085354.GH34802@e132581.arm.com> Hi Leo, > On Wed, Apr 22, 2026 at 02:21:57PM +0100, Yeoreum Yun wrote: > > [...] > > > - Since active_config and related fields are accessed only by the local CPU > > in etm4_enable/disable_sysfs_smp_call() (similar to perf enable/disable), > > remove the lock/unlock from the sysfs enable/disable path and > > startup/dying_cpu except when to access config fields. > > Thanks for writing this up, which is helpful for understanding. > > [...] > > > @@ -918,40 +948,29 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa > > > > /* enable any config activated by configfs */ > > cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset); > > With the patch [1], we can move cscfg_config_sysfs_get_active_cfg() to > smp call. As a result, all things for enabling cscfg can be in the > same place. > > [1] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-14-1c9dcb1de8c9@arm.com/ > > > - if (cfg_hash) { > > - ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); > > - if (ret) { > > - etm4_release_trace_id(drvdata); > > - return ret; > > - } > > - } > > - > > - raw_spin_lock(&drvdata->spinlock); > > - > > - drvdata->trcid = path->trace_id; > > - > > - /* Tracer will never be paused in sysfs mode */ > > - drvdata->paused = false; > > > > /* > > * Executing etm4_enable_hw on the cpu whose ETM is being enabled > > * ensures that register writes occur when cpu is powered. > > */ > > arg.drvdata = drvdata; > > + arg.path = path; > > + arg.cfg_hash = cfg_hash; > > + arg.preset = preset; > > Connect with the comment above , don't need to pass cfg_hash/preset anymore. This sounds like we're going to merge your patch first and then apply this patch series. Isn't it compromised with Suzuki? If so, I'll send this patch based on your own but if not, We still need this code and it's the same comment for your own in other patch. > > > + raw_spin_lock(&drvdata->spinlock); > > + arg.config = drvdata->config; > > + raw_spin_unlock(&drvdata->spinlock); > > Seems to me, this is right way for locking - here we simply use > spinlock for exclusive access config from sysfs knobs. > > However, we avoid the config copy and directly access in SMP call? > we still can use the raw spinlock in SMP call. No we couldn't. for example, suppose cpu0 modifies its own drvdata->data and holding lock, but other cpu try to enable cpu0's etm4x if SMP call is entered before release lock, It becomes deadlock situation. IOW, to grap the drvdata->spinlock in the SMP call, all sysfs raw_spin_lock() should be changed into raw_spin_lock_irqsave(). However this would make an unexpected latency for modifying drvdata via sysfs, It should grab lock in here. > > My suggestion is: > > - First use a patch to move the drvdata assignment to SMP call and > remove spinlock; > - Then, rebase this patch for moving cscfg into SMP call. > > If so, we only need add a new field "arg->path", right? Nope base on above. > > > > @@ -1857,13 +1875,11 @@ static int etm4_starting_cpu(unsigned int cpu) > > if (!etmdrvdata[cpu]) > > return 0; > > > > - raw_spin_lock(&etmdrvdata[cpu]->spinlock); > > With the change [2], the starting and dying functions have been > removed. > > If you rebase patches on the top PM series, then you will not bother > this. Anyway, this is right to remove spinlock for hotplug notifiers. > > [2] https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-27-1c9dcb1de8c9@arm.com/ Agree but let me confirm the plan for merge. -- Sincerely, Yeoreum Yun