Re: [PATCH v11 1/9] coresight: etm4x: prohibit modifying ss_status and cntr_val while session is enabled

[Mike Leach <mike.leach@arm.com>]

Hi,

Preventing a write to the counter value register is correct, but 
protecting the ss_status is redundant.

On 9/15/26 12:34, Yeoreum Yun wrote:
> ss_status and cntr_val are overwritten with the values read from the
> corresponding registers by etm4_disable_hw() when the session is disabled.
> 
> This means that any changes to these values made while the session is
> enabled would be lost when the session is disabled.
> 
> Therefore, prohibit modifying ss_status and cntr_val while the session is
> enabled.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index cc6cdd3ae29d5..8d73248b840a6 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -1613,6 +1613,8 @@ static ssize_t cntr_val_store(struct device *dev,
>   		return -EINVAL;
>   	if (val > ETM_CNTR_MAX_VAL)
>   		return -EINVAL;
> +	if (coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
>   
>   	raw_spin_lock(&drvdata->spinlock);
>   	idx = config->cntr_idx;
> @@ -1797,6 +1799,8 @@ static ssize_t sshot_ctrl_store(struct device *dev,
>   
>   	if (kstrtoul(buf, 16, &val))
>   		return -EINVAL;
> +	if (coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
>   

This is protecting a write clearing the STATUS bit in TRCSSCSR - but 
this is cleared automatically when next time the hardware is enabled in 
etm4_enable_hw(). So the write to ss_status in this function is 
redundant as is this protection.

The actual write to the targeted control register will persist.

>   	raw_spin_lock(&drvdata->spinlock);
>   	idx = config->ss_idx;
> @@ -1847,6 +1851,8 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
>   
>   	if (kstrtoul(buf, 16, &val))
>   		return -EINVAL;
> +	if (coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
>   
>   	raw_spin_lock(&drvdata->spinlock);
>   	idx = config->ss_idx;
> 

as above

Regards

Mike