Re: [PATCH v11 7/9] coresight: etm4x: exclude ss_status from drvdata->config

[Yeoreum Yun <yeoreum.yun@arm.com>]

On Tue, Sep 15, 2026 at 11:49:58AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] coresight: data race and state leak in shared ss_status array
> - [Medium] coresight: contradictory bitmask change introduces unhandled behavior
> --
> 
> commit 1a408a1bbc8676e137115f3c8798cfbd8a499e15
> Author: Yeoreum Yun <yeoreum.yun@arm.com>
> 
> coresight: etm4x: exclude ss_status from drvdata->config
>     
> This patch moves the ss_status array out of the session-specific configuration 
> structures and into the shared etmv4_drvdata structure. It aims to preserve 
> existing behavior regarding single-shot comparator events while cleaning up 
> the configuration data.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 70a5c0b4fad1c..9de0c268ff954 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> [ ... ]
> > @@ -573,11 +573,11 @@ 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 */
> > +		/* always clear status and pending bits on restart if using single-shot */
> >  		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> > -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> > +			drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> 
> [Severity: Medium]
> The commit message states that this patch intentionally preserves the existing
> behavior regarding the clearing of PENDING and STATUS bits. However, the
> pre-patch code only cleared TRCSSCSRn_STATUS. By explicitly adding
> TRCSSCSRn_PENDING to the bitmask here, does this code actively introduce the
> behavioral change it claims to defer?

No perf do always clean, just make the same for right now.

> 
> [ ... ]
> > @@ -1081,7 +1081,7 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> >  
> >  	/* read the status of the single shot comparators */
> >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > -		config->ss_status[i] =
> > +		drvdata->ss_status[i] =
> >  			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> >  	}
> 
> [Severity: High]
> When a perf session stops, etm4_disable_hw() writes the hardware state
> directly into the shared drvdata->ss_status array without holding
> drvdata->spinlock.
> 
> Since a concurrent sysfs session could be writing to this same array in
> sshot_ctrl_store() under the spinlock, can this lockless write cause a data
> race?
> 
> Additionally, does this overwrite leak the perf session's single-shot state
> into the shared array, allowing a subsequent sysfs session to inadvertently
> observe or re-inject the leftover perf state?

perf uses active_config directly, sysfs only uses config. so this is not
true.

> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > index f6d053f73264c..50ef2e957300f 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> [ ... ]
> > @@ -1833,8 +1833,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);
> >  	raw_spin_unlock(&drvdata->spinlock);
> 
> [Severity: High]
> As seen here, sysfs modifies the same shared array under the spinlock.
> Is it safe to move ss_status to the shared drvdata struct without
> synchronizing the hardware stop paths or segregating the session state?

It checks the mode first. So it's fine.


-- 
Sincerely,
Yeoreum Yun