RE: [PATCH 8/8] coresight: cti: Refactor cti_reg32_{show|store}()

[Mike Leach]

Hi Leo,

> -----Original Message-----
> From: Leo Yan <leo.yan@xxxxxxx>
> Sent: Monday, February 9, 2026 6:01 PM
> To: Suzuki Poulose <Suzuki.Poulose@xxxxxxx>; Mike Leach
> <Mike.Leach@xxxxxxx>; James Clark <james.clark@xxxxxxxxxx>; Alexander
> Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Mathieu Poirier
> <mathieu.poirier@xxxxxxxxxx>; Tingwei Zhang <quic_tingwei@xxxxxxxxxxx>;
> Yingchao Deng <yingchao.deng@xxxxxxxxxxxxxxxx>; Jie Gan
> <jie.gan@xxxxxxxxxxxxxxxx>
> Cc: coresight@xxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Leo Yan <Leo.Yan@xxxxxxx>
> Subject: [PATCH 8/8] coresight: cti: Refactor cti_reg32_{show|store}()
> 
> Return an error for any negative offset.  Since the cached value is used
> to store user config, it is not updated when reading back the register
> in cti_reg32_show().
> 
> Signed-off-by: Leo Yan <leo.yan@xxxxxxx>
> ---
>  drivers/hwtracing/coresight/coresight-cti-sysfs.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index
> 9ef44956ebdc7781717d773fa014165989df2048..baac2a5dd467032fafbc6
> 523d8885de59cb2665b 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -252,14 +252,14 @@ static ssize_t cti_reg32_show(struct device *dev,
> char *buf,
>  	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>  	struct cti_config *config = &drvdata->config;
> 
> +	if (reg_offset < 0)
> +		return -EINVAL;
> +
>  	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {
> -		if ((reg_offset >= 0) && cti_is_active(config)) {
> +		if (cti_is_active(config))
>  			val = cti_read_single_reg(drvdata, reg_offset);
> -			if (pcached_val)
> -				*pcached_val = val;

I think we still need this. If a register with an none-zero default value / status that can change over time is read when active, then read when inactive, but never written, then the inactive value will not reflect the last read value.
Relatively minor issue but does represent a potential change in functionality for the driver - even if I cannot see specific issues for current ARM CTIs. This is a R/W cache so should be updated on both R and W.

> -		} else if (pcached_val) {
> +		else if (pcached_val)
>  			val = *pcached_val;
> -		}
>  	}
> 
>  	return sprintf(buf, "%#x\n", val);
> @@ -280,13 +280,16 @@ static ssize_t cti_reg32_store(struct device *dev,
> const char *buf,
>  	if (kstrtoul(buf, 0, &val))
>  		return -EINVAL;
> 
> +	if (reg_offset < 0)
> +		return -EINVAL;
> +
>  	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {
>  		/* local store */
>  		if (pcached_val)
>  			*pcached_val = (u32)val;
> 
>  		/* write through if offset and enabled */
> -		if ((reg_offset >= 0) && cti_is_active(config))
> +		if (cti_is_active(config))
>  			cti_write_single_reg(drvdata, reg_offset, val);
>  	}
> 
> 
> --
> 2.34.1

Regards

Mike