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: <e99c390f-229f-41cf-8646-b1400331605e@arm.com>
Date: Wed, 30 Apr 2025 10:21:13 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Leo Yan <leo.yan@....com>, Mike Leach <mike.leach@...aro.org>,
 James Clark <james.clark@...aro.org>, Jonathan Corbet <corbet@....net>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/7] coresight: etm4x: Extract the trace unit
 controlling

On 01/04/2025 19:07, Leo Yan wrote:
> The trace unit is controlled in the ETM hardware enabling and disabling.
> The sequential changes for support AUX pause and resume will reuse the
> same operations.
> 
> Extract the operations in the etm4_{enable|disable}_trace_unit()
> functions.  A minor improvement in etm4_enable_trace_unit() is for
> returning the timeout error to callers.
> 
> Signed-off-by: Leo Yan <leo.yan@....com>
> Reviewed-by: Mike Leach <mike.leach@...aro.org>

The patch looks good to me. One comment below, nothing with your patch 
though.


> ---
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 103 +++++++++++++++++++++++++----------------
>   1 file changed, 62 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..53cb0569dbbf 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -431,6 +431,44 @@ static int etm4x_wait_status(struct csdev_access *csa, int pos, int val)
>   	return coresight_timeout(csa, TRCSTATR, pos, val);
>   }
>   
> +static int etm4_enable_trace_unit(struct etmv4_drvdata *drvdata)
> +{
> +	struct coresight_device *csdev = drvdata->csdev;
> +	struct device *etm_dev = &csdev->dev;
> +	struct csdev_access *csa = &csdev->access;
> +
> +	/*
> +	 * ETE mandates that the TRCRSR is written to before
> +	 * enabling it.
> +	 */
> +	if (etm4x_is_ete(drvdata))
> +		etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
> +
> +	etm4x_allow_trace(drvdata);
> +	/* Enable the trace unit */
> +	etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
> +
> +	/* Synchronize the register updates for sysreg access */
> +	if (!csa->io_mem)
> +		isb();
> +
> +	/* wait for TRCSTATR.IDLE to go back down to '0' */
> +	if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 0)) {
> +		dev_err(etm_dev,
> +			"timeout while waiting for Idle Trace Status\n");
> +		return -ETIME;
> +	}
> +
> +	/*
> +	 * As recommended by section 4.3.7 ("Synchronization when using the
> +	 * memory-mapped interface") of ARM IHI 0064D
> +	 */
> +	dsb(sy);
> +	isb();
> +
> +	return 0;
> +}
> +
>   static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>   {
>   	int i, rc;
> @@ -539,33 +577,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>   		etm4x_relaxed_write32(csa, trcpdcr | TRCPDCR_PU, TRCPDCR);
>   	}
>   
> -	/*
> -	 * ETE mandates that the TRCRSR is written to before
> -	 * enabling it.
> -	 */
> -	if (etm4x_is_ete(drvdata))
> -		etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
> -
> -	etm4x_allow_trace(drvdata);
> -	/* Enable the trace unit */
> -	etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
> -
> -	/* Synchronize the register updates for sysreg access */
> -	if (!csa->io_mem)
> -		isb();
> -
> -	/* wait for TRCSTATR.IDLE to go back down to '0' */
> -	if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 0))
> -		dev_err(etm_dev,
> -			"timeout while waiting for Idle Trace Status\n");
> -
> -	/*
> -	 * As recommended by section 4.3.7 ("Synchronization when using the
> -	 * memory-mapped interface") of ARM IHI 0064D
> -	 */
> -	dsb(sy);
> -	isb();
> -
> +	rc = etm4_enable_trace_unit(drvdata);
>   done:
>   	etm4_cs_lock(drvdata, csa);
>   
> @@ -884,25 +896,12 @@ static int etm4_enable(struct coresight_device *csdev, struct perf_event *event,
>   	return ret;
>   }
>   
> -static void etm4_disable_hw(void *info)
> +static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata)
>   {
>   	u32 control;
> -	struct etmv4_drvdata *drvdata = info;
> -	struct etmv4_config *config = &drvdata->config;
>   	struct coresight_device *csdev = drvdata->csdev;
>   	struct device *etm_dev = &csdev->dev;
>   	struct csdev_access *csa = &csdev->access;
> -	int i;
> -
> -	etm4_cs_unlock(drvdata, csa);
> -	etm4_disable_arch_specific(drvdata);
> -
> -	if (!drvdata->skip_power_up) {
> -		/* power can be removed from the trace unit now */
> -		control = etm4x_relaxed_read32(csa, TRCPDCR);
> -		control &= ~TRCPDCR_PU;
> -		etm4x_relaxed_write32(csa, control, TRCPDCR);
> -	}
>   
>   	control = etm4x_relaxed_read32(csa, TRCPRGCTLR);
>   
> @@ -943,6 +942,28 @@ static void etm4_disable_hw(void *info)
>   	 * of ARM IHI 0064H.b.
>   	 */
>   	isb();
> +}
> +
> +static void etm4_disable_hw(void *info)
> +{
> +	u32 control;
> +	struct etmv4_drvdata *drvdata = info;
> +	struct etmv4_config *config = &drvdata->config;
> +	struct coresight_device *csdev = drvdata->csdev;
> +	struct csdev_access *csa = &csdev->access;
> +	int i;
> +
> +	etm4_cs_unlock(drvdata, csa);
> +	etm4_disable_arch_specific(drvdata);
> +
> +	if (!drvdata->skip_power_up) {
> +		/* power can be removed from the trace unit now */
> +		control = etm4x_relaxed_read32(csa, TRCPDCR);
> +		control &= ~TRCPDCR_PU;
> +		etm4x_relaxed_write32(csa, control, TRCPDCR);
> +	}

Shouldn't we delay ^^ until we have read all the registers back and 
disabled the trace ? As I said, this is an existing problem and may need
to be fixed. It would be good to move it after we have done everything 
with the accesses. If we start with that patch, we could easily backport
it to stable.


Suzuki


> +
> +	etm4_disable_trace_unit(drvdata);
>   
>   	/* read the status of the single shot comparators */
>   	for (i = 0; i < drvdata->nr_ss_cmp; i++) {


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ