Re: [PATCH RESEND v13 04/14] coresight: etm4x: fix inconsistencies with sysfs configuration

[Yeoreum Yun]

On Sat, Oct 03, 2026 at 03:06:32AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] coresight: data race in etm4_disable_sysfs() due to missing spinlock
> --
> 
> commit 047fa1646a004922cd9c2e2d801758f38062c7c6
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> Subject: coresight: etm4x: fix inconsistencies with sysfs configuration
> 
> This commit separates the ETM4x configuration into a session-specific 
> curr_config and a sysfs-set config to prevent corruption and inconsistency. 
> It also removes drvdata->spinlock coverage from the enable/disable paths 
> except when explicitly accessing configuration fields.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> 
> [ ... ]
> 
> > @@ -1134,8 +1142,9 @@ static int etm4_disable_perf(struct coresight_device *csdev,
> >  static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  {
> >  	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> > -
> > -	raw_spin_lock(&drvdata->spinlock);
> > +	struct etmv4_config *config = &drvdata->config;
> > +	const struct etmv4_config *curr_config = &drvdata->curr_config;
> > +	int i;
> >  
> >  	/*
> >  	 * Executing etm4_disable_hw on the cpu whose ETM is being disabled
> > @@ -1144,9 +1153,19 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  	smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call,
> >  				 drvdata, 1);
> >  
> > -	raw_spin_unlock(&drvdata->spinlock);
> > +	/*
> > +	 * Userspace may read ss_status and cntr_val through sysfs after
> > +	 * the sysfs-session has been disabled.
> > +	 */
> > +	for (i = 0; i < drvdata->nr_ss_cmp; i++)
> > +		config->ss_status[i] = curr_config->ss_status[i];
> >  
> > -	cscfg_csdev_disable_active_config(csdev);
> > +	for (i = 0; i < drvdata->nr_cntr; i++)
> > +		config->cntr_val[i] = curr_config->cntr_val[i];
> > +
> > +	config->seq_state = curr_config->seq_state;
> 
> [Severity: Medium]
> Does this modification to the config fields require drvdata->spinlock
> protection?
> 
> The commit message notes the intent to remove the lock "except when to
> access config fields", which is correctly followed during sysfs enable
> via scoped_guard. However, sysfs readers expect this spinlock to be held
> when reading these arrays.

This is not matter. Since cntr_val and seq_sate wouldn't be
modified before disabled and ss_status' write purpose is clearing
the PENDING and STATUS bits, lockless read is fine so it's intended.

Therefore, this comment is ignorable.

-- 
Sincerely,
Yeoreum Yun