Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:52PM +0100, Yeoreum Yun wrote:
> TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100,
> in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000.
> 
> Therefore, drvdata->nrseqstate should be checked before entering the loop.

Since TRCSEQEVRn (n=0~2), to avoid confusion, we also need to rename
ETM_MAX_SEQ_STATES to ETM_MAX_SEQ_TRANSITIONS and define it as 3:

   #define ETM_MAX_SEQ_TRANSITIONS      3

Then we don't allocate 4 items but use only 3 of them.

Thanks,
Leo

Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

[Yeoreum Yun]

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:52PM +0100, Yeoreum Yun wrote:
> > TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100,
> > in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000.
> > 
> > Therefore, drvdata->nrseqstate should be checked before entering the loop.
> 
> Since TRCSEQEVRn (n=0~2), to avoid confusion, we also need to rename
> ETM_MAX_SEQ_STATES to ETM_MAX_SEQ_TRANSITIONS and define it as 3:
> 
>    #define ETM_MAX_SEQ_TRANSITIONS      3
> 
> Then we don't allocate 4 items but use only 3 of them.

TBH, the number of transition is determinied by the MAX number of
SEQ_STATE that's why I think define the ETM_MAX_SEQ_TRANSITIONS with

     #define ETM_MAX_SEQ_TRANSITIONS (ETM_MAX_SEQ_STATE - 1)

and uses ETM_MAX_SEQ_TRANSITIONS for the TCRSEQEVR and seq_ctrl.

Thought?

-- 
Sincerely,
Yeoreum Yun

Re: [PATCH v6 02/13] coresight: etm4x: fix underflow for nrseqstate

[Leo Yan]

On Fri, May 15, 2026 at 12:10:45PM +0100, Yeoreum Yun wrote:

[...]

> TBH, the number of transition is determinied by the MAX number of
> SEQ_STATE that's why I think define the ETM_MAX_SEQ_TRANSITIONS with
> 
>      #define ETM_MAX_SEQ_TRANSITIONS (ETM_MAX_SEQ_STATE - 1)
> 
> and uses ETM_MAX_SEQ_TRANSITIONS for the TCRSEQEVR and seq_ctrl.
> 
> Thought?

Looks good to me.

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:

[...]

> @@ -573,11 +573,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  		etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
>  
>  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> -		/* always clear status bit on restart if using single-shot */
> -		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
>  		etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> -		etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> +		/* always clear status and pending bits on restart if using single-shot */
> +		etm4x_relaxed_write32(csa, 0x0, TRCSSCSRn(i));

I am not confident what is the right way to handle the pending bit.
I looked a bit Arm ARM but still no clue. In particular, I suspect it
may need to be handled when disabling the trace in etm4_disable_hw().

Let's make it clear with some internal check.

> @@ -1829,8 +1829,8 @@ static ssize_t sshot_ctrl_store(struct device *dev,
>  	raw_spin_lock(&drvdata->spinlock);
>  	idx = config->ss_idx;
>  	config->ss_ctrl[idx] = FIELD_PREP(TRCSSCCRn_SAC_ARC_RST_MASK, val);
> -	/* must clear bit 31 in related status register on programming */
> -	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
> +	/* must clear bit 31 and 30 in related status register on programming */
> +	drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

Similarly, the question is: if it is in a pending state, how can we
ensure the state machine works properly with the new settings?

>  	raw_spin_unlock(&drvdata->spinlock);
>  	return size;
>  }

> @@ -1879,8 +1879,8 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
>  	raw_spin_lock(&drvdata->spinlock);
>  	idx = config->ss_idx;
>  	config->ss_pe_cmp[idx] = FIELD_PREP(TRCSSPCICRn_PC_MASK, val);
> -	/* must clear bit 31 in related status register on programming */
> -	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
> +	/* must clear bit 31 and 30 in related status register on programming */
> +	drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

Ditto.

Thanks,
Leo

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:

[...]

> @@ -573,11 +573,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  		etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
>  
>  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> -		/* always clear status bit on restart if using single-shot */
> -		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
>  		etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> -		etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> +		/* always clear status and pending bits on restart if using single-shot */
> +		etm4x_relaxed_write32(csa, 0x0, TRCSSCSRn(i));

After confirmed with hardware team, we should preserve status bits
(including STATUS and PENDING bits) during a session. So here we should
set drvdata->ss_status to TRCSSCSRn.

> @@ -1503,8 +1501,9 @@ static void etm4_init_arch_data(void *info)
>  	 */
>  	caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
>  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> -		drvdata->config.ss_status[i] =
> -			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> +		drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> +		drvdata->ss_status[i] &= (TRCSSCSRn_PC | TRCSSCSRn_DV |
> +					  TRCSSCSRn_DA | TRCSSCSRn_INST);

It is fine for read these capacity bits when probe, but we need to clear
status when a session is starting to avoid the stale value left from
previous session:

  drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

We can do this in etm4_parse_event_config() for perf mode, and might
create a new function (say etm4_parse_sysfs_config()) for preparing
config for sysfs mode?

The rest looks good to me.

Thanks,
Leo

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Yeoreum Yun]

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:54PM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> > @@ -573,11 +573,9 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> >  		etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
> >  
> >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > -		/* always clear status bit on restart if using single-shot */
> > -		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> > -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> >  		etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> > -		etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> > +		/* always clear status and pending bits on restart if using single-shot */
> > +		etm4x_relaxed_write32(csa, 0x0, TRCSSCSRn(i));
> 
> After confirmed with hardware team, we should preserve status bits
> (including STATUS and PENDING bits) during a session. So here we should
> set drvdata->ss_status to TRCSSCSRn.
> 
> > @@ -1503,8 +1501,9 @@ static void etm4_init_arch_data(void *info)
> >  	 */
> >  	caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
> >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > -		drvdata->config.ss_status[i] =
> > -			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > +		drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > +		drvdata->ss_status[i] &= (TRCSSCSRn_PC | TRCSSCSRn_DV |
> > +					  TRCSSCSRn_DA | TRCSSCSRn_INST);
> 
> It is fine for read these capacity bits when probe, but we need to clear
> status when a session is starting to avoid the stale value left from
> previous session:
> 
>   drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> 
> We can do this in etm4_parse_event_config() for perf mode, and might
> create a new function (say etm4_parse_sysfs_config()) for preparing
> config for sysfs mode?
> 

As we discussed in offline, those bits should be cleared at the begining
of the session. so clearing drvdata->ss_status at start of session
in each mode is fine for me and for future integration for cpu suspend/resume.

But, I want to clarify that the perf is one of exceptional case
since the "etm4_parse_event_config()" is called at the "resume" of session
for per-thread mode event.

TBH, We don't have some specific usage how STATUS or PENDING bit could
be used with perf session and until now those bits are always cleared 
at the time of "sched-in" though we need to keep those bits theoretically.

Anyway as we discussed, since now there have been no issue
relavant for those bits, let the clear drvdata->ss_status at the
etm4_parse_event_config() or when setting a active config for start/resume
in this patchset and let me fix this with another patchset.

-- 
Sincerely,
Yeoreum Yun

Re: [PATCH v6 04/13] coresight: etm4x: exclude ss_status from drvdata->config

[Leo Yan]

On Sat, May 09, 2026 at 12:55:19PM +0100, Yeoreum Yun wrote:

> > >  	caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
> > >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > > -		drvdata->config.ss_status[i] =
> > > -			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > > +		drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> > > +		drvdata->ss_status[i] &= (TRCSSCSRn_PC | TRCSSCSRn_DV |
> > > +					  TRCSSCSRn_DA | TRCSSCSRn_INST);
> > 
> > It is fine for read these capacity bits when probe, but we need to clear
> > status when a session is starting to avoid the stale value left from
> > previous session:
> > 
> >   drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

> But, I want to clarify that the perf is one of exceptional case
> since the "etm4_parse_event_config()" is called at the "resume" of session
> for per-thread mode event.

Good point!

The right way would move etm4_parse_event_config() to setup AUX phase
(e.g., call it when build path).

> Anyway as we discussed, since now there have been no issue
> relavant for those bits, let the clear drvdata->ss_status at the
> etm4_parse_event_config().

I am fine to clear status in etm4_parse_event_config() in this series.

Thanks,
Leo

Re: [PATCH v6 07/13] coresight: etm4x: fix inconsistencies with sysfs configuration

[Leo Yan]

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.

> +	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.

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?


> @@ -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/

Re: [PATCH v6 07/13] coresight: etm4x: fix inconsistencies with sysfs configuration

[Yeoreum Yun]

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

Re: [PATCH v6 08/13] coresight: etm4x: remove redundant call etm4_enable_hw() with hotplug

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:58PM +0100, Yeoreum Yun wrote:

[...]

> +	/*
> +	 * Take the hotplug lock to prevent redundant calls to etm4_enable_hw().
> +	 *
> +	 * The cpu_online_mask is set at the CPUHP_BRINGUP_CPU step.
> +	 * In other words, if etm4_enable_sysfs() is called between
> +	 * CPUHP_BRINGUP_CPU and CPUHP_AP_ARM_CORESIGHT_STARTING,
> +	 * etm4_enable_hw() may be invoked in etm4_enable_sysfs_smp_call()
> +	 * and then executed again in etm4_starting_cpu().
> +	 */
> +	cpus_read_lock();
>  	ret = smp_call_function_single(drvdata->cpu,
>  				       etm4_enable_sysfs_smp_call, &arg, 1);
> +	cpus_read_unlock();

This will cause double deadlock with the patch:
https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-6-1c9dcb1de8c9@arm.com/#t

I think we need to drop this one.

Thanks,
Leo

Re: [PATCH v6 08/13] coresight: etm4x: remove redundant call etm4_enable_hw() with hotplug

[Yeoreum Yun]

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:58PM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> > +	/*
> > +	 * Take the hotplug lock to prevent redundant calls to etm4_enable_hw().
> > +	 *
> > +	 * The cpu_online_mask is set at the CPUHP_BRINGUP_CPU step.
> > +	 * In other words, if etm4_enable_sysfs() is called between
> > +	 * CPUHP_BRINGUP_CPU and CPUHP_AP_ARM_CORESIGHT_STARTING,
> > +	 * etm4_enable_hw() may be invoked in etm4_enable_sysfs_smp_call()
> > +	 * and then executed again in etm4_starting_cpu().
> > +	 */
> > +	cpus_read_lock();
> >  	ret = smp_call_function_single(drvdata->cpu,
> >  				       etm4_enable_sysfs_smp_call, &arg, 1);
> > +	cpus_read_unlock();
> 
> This will cause double deadlock with the patch:
> https://lore.kernel.org/linux-arm-kernel/20260511-arm_coresight_path_power_management_improvement-v12-6-1c9dcb1de8c9@arm.com/#t
> 
> I think we need to drop this one.

As I asked on another thread, Are we're going to merge your patch first?
If not, we need to sustain it anyway.

Thanks.

-- 
Sincerely,
Yeoreum Yun

Re: [PATCH v6 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable

[Leo Yan]

On Wed, Apr 22, 2026 at 02:21:59PM +0100, Yeoreum Yun wrote:

[...]

> @@ -895,6 +895,8 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  			 * Missing BB support could cause silent decode errors
>  			 * so fail to open if it's not supported.
>  			 */
> +			if (cfg_hash)
> +				cscfg_csdev_disable_active_config(csdev);

I prefer do a bit refactoring for this.

we just save cfg_hash and cfg_preset into drvdata in
etm4_parse_event_config():

  drvdata->cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
  if (drvdata->cfg_hash)
        drvdata->preset = ATTR_CFG_GET_FLD(attr, preset);

Then create two helpers:

  etm4_cscfg_enable(csdev) {
    struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

    return cscfg_csdev_enable_active_config(csdev, drvdata->cfg_hash,
                                            drvdata->preset);
  }

  etm4_cscfg_disable(csdev) {
    cscfg_csdev_disable_active_config(csdev);
  }

These helpers will be used by etm4_{enable|disable}_perf()
and etm4_{enable|disable}_sysfs(). This might benefit the future cscfg
refactoring.

Thanks,
Leo

P.s. I will stop review at here. I assume ETMv3 changes just mirror
ETMv4's. So all ETMv4's comments would apply on ETMv3 patches. If I
miss anything, please let me know.

Re: [PATCH v6 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable

[Yeoreum Yun]

Hi Leo,

> On Wed, Apr 22, 2026 at 02:21:59PM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
> > @@ -895,6 +895,8 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> >  			 * Missing BB support could cause silent decode errors
> >  			 * so fail to open if it's not supported.
> >  			 */
> > +			if (cfg_hash)
> > +				cscfg_csdev_disable_active_config(csdev);
> 
> I prefer do a bit refactoring for this.
> 
> we just save cfg_hash and cfg_preset into drvdata in
> etm4_parse_event_config():
> 
>   drvdata->cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
>   if (drvdata->cfg_hash)
>         drvdata->preset = ATTR_CFG_GET_FLD(attr, preset);
> 
> Then create two helpers:
> 
>   etm4_cscfg_enable(csdev) {
>     struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> 
>     return cscfg_csdev_enable_active_config(csdev, drvdata->cfg_hash,
>                                             drvdata->preset);
>   }
> 
>   etm4_cscfg_disable(csdev) {
>     cscfg_csdev_disable_active_config(csdev);
>   }
> 
> These helpers will be used by etm4_{enable|disable}_perf()
> and etm4_{enable|disable}_sysfs(). This might benefit the future cscfg
> refactoring.

I think this seems some over-engineering since the
etm4_cscfg_enable/disable() just an wrapper for
cscfg_csdev_enable/disable_active_config() but just increase size of drvdata.

It's not late to delay when we do refactoring the cscfg
and at that time, we can consider some place to save cfg_hash and
preset. If we do right now, personally, there seems no benefit for this.

Am I missing something?

Thanks.

[...]

-- 
Sincerely,
Yeoreum Yun