linux-kernel - Re: [PATCH] coresight: Drop atomics in connection refcounts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z1F8HvLY/04jukUH@e129823.arm.com>
Date: Thu, 5 Dec 2024 10:10:38 +0000
From: Yeoreum Yun <yeoreum.yun@....com>
To: James Clark <james.clark@...aro.org>
Cc: suzuki.poulose@....com, coresight@...ts.linaro.org,
	mike.leach@...aro.org,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] coresight: Drop atomics in connection refcounts

This patch looks good to me.

Reviewed-by: Yeoreum Yun <yeoreum.yun@....com>

On Thu, Nov 28, 2024 at 12:14:14PM +0000, James Clark wrote:
> These belong to the device being enabled or disabled and are only ever
> used inside the device's spinlock. Remove the atomics to not imply that
> there are any other concurrent accesses.
>
> If atomics were necessary I don't think they would have been enough
> anyway. There would be nothing to prevent an enable or disable running
> concurrently if not for the spinlock.
>
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
>  drivers/hwtracing/coresight/coresight-funnel.c     | 6 +++---
>  drivers/hwtracing/coresight/coresight-replicator.c | 6 +++---
>  drivers/hwtracing/coresight/coresight-tpda.c       | 6 +++---
>  include/linux/coresight.h                          | 4 ++--
>  4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 5a819c8970fb..bd32f74cbdae 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -86,14 +86,14 @@ static int funnel_enable(struct coresight_device *csdev,
>  	bool first_enable = false;
>
>  	spin_lock_irqsave(&drvdata->spinlock, flags);
> -	if (atomic_read(&in->dest_refcnt) == 0) {
> +	if (in->dest_refcnt == 0) {
>  		if (drvdata->base)
>  			rc = dynamic_funnel_enable_hw(drvdata, in->dest_port);
>  		if (!rc)
>  			first_enable = true;
>  	}
>  	if (!rc)
> -		atomic_inc(&in->dest_refcnt);
> +		in->dest_refcnt++;
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
>  	if (first_enable)
> @@ -130,7 +130,7 @@ static void funnel_disable(struct coresight_device *csdev,
>  	bool last_disable = false;
>
>  	spin_lock_irqsave(&drvdata->spinlock, flags);
> -	if (atomic_dec_return(&in->dest_refcnt) == 0) {
> +	if (--in->dest_refcnt == 0) {
>  		if (drvdata->base)
>  			dynamic_funnel_disable_hw(drvdata, in->dest_port);
>  		last_disable = true;
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index 3e55be9c8418..31322aea19f2 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -126,7 +126,7 @@ static int replicator_enable(struct coresight_device *csdev,
>  	bool first_enable = false;
>
>  	spin_lock_irqsave(&drvdata->spinlock, flags);
> -	if (atomic_read(&out->src_refcnt) == 0) {
> +	if (out->src_refcnt == 0) {
>  		if (drvdata->base)
>  			rc = dynamic_replicator_enable(drvdata, in->dest_port,
>  						       out->src_port);
> @@ -134,7 +134,7 @@ static int replicator_enable(struct coresight_device *csdev,
>  			first_enable = true;
>  	}
>  	if (!rc)
> -		atomic_inc(&out->src_refcnt);
> +		out->src_refcnt++;
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
>  	if (first_enable)
> @@ -180,7 +180,7 @@ static void replicator_disable(struct coresight_device *csdev,
>  	bool last_disable = false;
>
>  	spin_lock_irqsave(&drvdata->spinlock, flags);
> -	if (atomic_dec_return(&out->src_refcnt) == 0) {
> +	if (--out->src_refcnt == 0) {
>  		if (drvdata->base)
>  			dynamic_replicator_disable(drvdata, in->dest_port,
>  						   out->src_port);
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index bfca103f9f84..4ec676bea1ce 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -190,10 +190,10 @@ static int tpda_enable(struct coresight_device *csdev,
>  	int ret = 0;
>
>  	spin_lock(&drvdata->spinlock);
> -	if (atomic_read(&in->dest_refcnt) == 0) {
> +	if (in->dest_refcnt == 0) {
>  		ret = __tpda_enable(drvdata, in->dest_port);
>  		if (!ret) {
> -			atomic_inc(&in->dest_refcnt);
> +			in->dest_refcnt++;
>  			csdev->refcnt++;
>  			dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
>  		}
> @@ -223,7 +223,7 @@ static void tpda_disable(struct coresight_device *csdev,
>  	struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
>  	spin_lock(&drvdata->spinlock);
> -	if (atomic_dec_return(&in->dest_refcnt) == 0) {
> +	if (--in->dest_refcnt == 0) {
>  		__tpda_disable(drvdata, in->dest_port);
>  		csdev->refcnt--;
>  	}
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index c13342594278..834029cb9ba2 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -200,8 +200,8 @@ struct coresight_connection {
>  	struct coresight_device *dest_dev;
>  	struct coresight_sysfs_link *link;
>  	struct coresight_device *src_dev;
> -	atomic_t src_refcnt;
> -	atomic_t dest_refcnt;
> +	int src_refcnt;
> +	int dest_refcnt;
>  };
>
>  /**
> --
> 2.34.1
>