Re: [PATCH v3 3/3] coresight: cti: Remove atomic type from enable_req_count

[Mike Leach]

On Tue, 10 Jan 2023 at 11:08, James Clark <james.clark@xxxxxxx> wrote:
>
> enable_req_count is only ever accessed inside the spinlock, so to avoid
> confusion that there are concurrent accesses and simplify the code,
> change it to an int.
>
> One access outside of the spinlock is in enable_show() which appears to
> allow partially written data to be displayed between enable_req_count,
> powered and enabled so move this one inside the spin lock too.
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
>  drivers/hwtracing/coresight/coresight-cti-core.c  | 14 +++++++-------
>  drivers/hwtracing/coresight/coresight-cti-sysfs.c |  2 +-
>  drivers/hwtracing/coresight/coresight-cti.h       |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index 838872f2484d..277c890a1f1f 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -107,12 +107,12 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>         cti_write_all_hw_regs(drvdata);
>
>         config->hw_enabled = true;
> -       atomic_inc(&drvdata->config.enable_req_count);
> +       drvdata->config.enable_req_count++;
>         spin_unlock_irqrestore(&drvdata->spinlock, flags);
>         return rc;
>
>  cti_state_unchanged:
> -       atomic_inc(&drvdata->config.enable_req_count);
> +       drvdata->config.enable_req_count++;
>
>         /* cannot enable due to error */
>  cti_err_not_enabled:
> @@ -129,7 +129,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
>         config->hw_powered = true;
>
>         /* no need to do anything if no enable request */
> -       if (!atomic_read(&drvdata->config.enable_req_count))
> +       if (!drvdata->config.enable_req_count)
>                 goto cti_hp_not_enabled;
>
>         /* try to claim the device */
> @@ -156,13 +156,13 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
>         spin_lock(&drvdata->spinlock);
>
>         /* don't allow negative refcounts, return an error */
> -       if (!atomic_read(&drvdata->config.enable_req_count)) {
> +       if (!drvdata->config.enable_req_count) {
>                 ret = -EINVAL;
>                 goto cti_not_disabled;
>         }
>
>         /* check refcount - disable on 0 */
> -       if (atomic_dec_return(&drvdata->config.enable_req_count) > 0)
> +       if (--drvdata->config.enable_req_count > 0)
>                 goto cti_not_disabled;
>
>         /* no need to do anything if disabled or cpu unpowered */
> @@ -239,7 +239,7 @@ static void cti_set_default_config(struct device *dev,
>         /* Most regs default to 0 as zalloc'ed except...*/
>         config->trig_filter_enable = true;
>         config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
> -       atomic_set(&config->enable_req_count, 0);
> +       config->enable_req_count = 0;
>  }
>
>  /*
> @@ -696,7 +696,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
>                 drvdata->config.hw_enabled = false;
>
>                 /* check enable reference count to enable HW */
> -               if (atomic_read(&drvdata->config.enable_req_count)) {
> +               if (drvdata->config.enable_req_count) {
>                         /* check we can claim the device as we re-power */
>                         if (coresight_claim_device(csdev))
>                                 goto cti_notify_exit;
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 71e7a8266bb3..e528cff9d4e2 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -84,8 +84,8 @@ static ssize_t enable_show(struct device *dev,
>         bool enabled, powered;
>         struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> -       enable_req = atomic_read(&drvdata->config.enable_req_count);
>         spin_lock(&drvdata->spinlock);
> +       enable_req = drvdata->config.enable_req_count;
>         powered = drvdata->config.hw_powered;
>         enabled = drvdata->config.hw_enabled;
>         spin_unlock(&drvdata->spinlock);
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index acf7b545e6b9..8b106b13a244 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -141,7 +141,7 @@ struct cti_config {
>         int nr_trig_max;
>
>         /* cti enable control */
> -       atomic_t enable_req_count;
> +       int enable_req_count;
>         bool hw_enabled;
>         bool hw_powered;
>
> --
> 2.25.1
>

Reviewed-by: Mike Leach <mike.leach@xxxxxxxxxx>

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK