[PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Bhupesh Sharma]

Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.

Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.

Fix the same.

Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
 - Addressed the review comments from Suzuki.
 - Rebased on linux-next.

 drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d39660a3e50c..14c1c7869795 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
 	if (!cpu_supports_sysreg_trace())
 		return false;
 
+	/*
+	 * Some Qualcomm implementations require skipping powering up the trace unit,
+	 * as the ETMs are in the same power domain as their CPU cores.
+	 *
+	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
+	 * initialize it before the function is called.
+	 */
+	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+		drvdata->skip_power_up = true;
+
 	/*
 	 * ETMs implementing sysreg access must implement TRCDEVARCH.
 	 */
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
 		return -EINVAL;
 
 	/* TRCPDCR is not accessible with system instructions. */
-	if (!desc.access.io_mem ||
-	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+	if (!desc.access.io_mem)
 		drvdata->skip_power_up = true;
 
 	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
-- 
2.35.3

[PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Bhupesh Sharma]

Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.

Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.

Fix the same.

Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
 - Addressed the review comments from Suzuki.
 - Rebased on linux-next.

 drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d39660a3e50c..14c1c7869795 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
 	if (!cpu_supports_sysreg_trace())
 		return false;
 
+	/*
+	 * Some Qualcomm implementations require skipping powering up the trace unit,
+	 * as the ETMs are in the same power domain as their CPU cores.
+	 *
+	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
+	 * initialize it before the function is called.
+	 */
+	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+		drvdata->skip_power_up = true;
+
 	/*
 	 * ETMs implementing sysreg access must implement TRCDEVARCH.
 	 */
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
 		return -EINVAL;
 
 	/* TRCPDCR is not accessible with system instructions. */
-	if (!desc.access.io_mem ||
-	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+	if (!desc.access.io_mem)
 		drvdata->skip_power_up = true;
 
 	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
-- 
2.35.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Bhupesh Sharma]

On 8/25/22 10:52 AM, Bhupesh Sharma wrote:
> Some Qualcomm ETM implementations require skipping powering up
> the trace unit, as the ETMs are in the same power domain as
> their CPU cores.
> 
> Via commit 5214b563588e ("coresight: etm4x: Add support for
> sysreg only devices"), the setting of 'skip_power_up' flag was
> moved after the 'etm4_init_arch_data' function is called, whereas
> the flag value is itself used inside the function. This causes
> a crash when ETM mode 'Low-power state behavior override' is set
> on some Qualcomm parts.
> 
> Fix the same.
> 
> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>   - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>   - Addressed the review comments from Suzuki.
>   - Rebased on linux-next.
> 
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d39660a3e50c..14c1c7869795 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>   	if (!cpu_supports_sysreg_trace())
>   		return false;
>   
> +	/*
> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> +	 * as the ETMs are in the same power domain as their CPU cores.
> +	 *
> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> +	 * initialize it before the function is called.
> +	 */
> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +		drvdata->skip_power_up = true;
> +
>   	/*
>   	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>   	 */
> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>   		return -EINVAL;
>   
>   	/* TRCPDCR is not accessible with system instructions. */
> -	if (!desc.access.io_mem ||
> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +	if (!desc.access.io_mem)
>   		drvdata->skip_power_up = true;
>   
>   	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);

Gentle ping. Any review comments on the v2?

Thanks,
Bhupesh

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Bhupesh Sharma]

On 8/25/22 10:52 AM, Bhupesh Sharma wrote:
> Some Qualcomm ETM implementations require skipping powering up
> the trace unit, as the ETMs are in the same power domain as
> their CPU cores.
> 
> Via commit 5214b563588e ("coresight: etm4x: Add support for
> sysreg only devices"), the setting of 'skip_power_up' flag was
> moved after the 'etm4_init_arch_data' function is called, whereas
> the flag value is itself used inside the function. This causes
> a crash when ETM mode 'Low-power state behavior override' is set
> on some Qualcomm parts.
> 
> Fix the same.
> 
> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>   - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>   - Addressed the review comments from Suzuki.
>   - Rebased on linux-next.
> 
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d39660a3e50c..14c1c7869795 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>   	if (!cpu_supports_sysreg_trace())
>   		return false;
>   
> +	/*
> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> +	 * as the ETMs are in the same power domain as their CPU cores.
> +	 *
> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> +	 * initialize it before the function is called.
> +	 */
> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +		drvdata->skip_power_up = true;
> +
>   	/*
>   	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>   	 */
> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>   		return -EINVAL;
>   
>   	/* TRCPDCR is not accessible with system instructions. */
> -	if (!desc.access.io_mem ||
> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +	if (!desc.access.io_mem)
>   		drvdata->skip_power_up = true;
>   
>   	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);

Gentle ping. Any review comments on the v2?

Thanks,
Bhupesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Leo Yan]

Hi Bhupesh,

On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
> Some Qualcomm ETM implementations require skipping powering up
> the trace unit, as the ETMs are in the same power domain as
> their CPU cores.
> 
> Via commit 5214b563588e ("coresight: etm4x: Add support for
> sysreg only devices"), the setting of 'skip_power_up' flag was
> moved after the 'etm4_init_arch_data' function is called, whereas
> the flag value is itself used inside the function. This causes
> a crash when ETM mode 'Low-power state behavior override' is set
> on some Qualcomm parts.
> 
> Fix the same.
> 
> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>  - Addressed the review comments from Suzuki.
>  - Rebased on linux-next.
> 
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d39660a3e50c..14c1c7869795 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>  	if (!cpu_supports_sysreg_trace())
>  		return false;
>  
> +	/*
> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> +	 * as the ETMs are in the same power domain as their CPU cores.
> +	 *
> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> +	 * initialize it before the function is called.
> +	 */
> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +		drvdata->skip_power_up = true;
> +

I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.

>  	/*
>  	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>  	 */
> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>  		return -EINVAL;
>  
>  	/* TRCPDCR is not accessible with system instructions. */
> -	if (!desc.access.io_mem ||
> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +	if (!desc.access.io_mem)
>  		drvdata->skip_power_up = true;

I prefer to move the condition checking for "desc.access.io_mem" to
etm4_init_sysreg_access(), this can make sure the flag skip_power_up
is set correctly based on property of system register access.

A side topic, in the mainline kernel I found the value
"desc.access.io_mem" is always zero (see the initialized value in
etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
desc.access.io_mem to true in etm4_probe()?

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d39660a3e50c..cf2555c50abb 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1939,6 +1939,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
        if (drvdata->cpu < 0)
                return drvdata->cpu;
 
+       desc.access.io_mem = true;
        init_arg.drvdata = drvdata;
        init_arg.csa = &desc.access;
        init_arg.pid = etm_pid;
  
Thanks,
Leo

>  	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
> -- 
> 2.35.3
> 

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Leo Yan]

Hi Bhupesh,

On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
> Some Qualcomm ETM implementations require skipping powering up
> the trace unit, as the ETMs are in the same power domain as
> their CPU cores.
> 
> Via commit 5214b563588e ("coresight: etm4x: Add support for
> sysreg only devices"), the setting of 'skip_power_up' flag was
> moved after the 'etm4_init_arch_data' function is called, whereas
> the flag value is itself used inside the function. This causes
> a crash when ETM mode 'Low-power state behavior override' is set
> on some Qualcomm parts.
> 
> Fix the same.
> 
> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>  - Addressed the review comments from Suzuki.
>  - Rebased on linux-next.
> 
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d39660a3e50c..14c1c7869795 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>  	if (!cpu_supports_sysreg_trace())
>  		return false;
>  
> +	/*
> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> +	 * as the ETMs are in the same power domain as their CPU cores.
> +	 *
> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> +	 * initialize it before the function is called.
> +	 */
> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +		drvdata->skip_power_up = true;
> +

I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.

>  	/*
>  	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>  	 */
> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>  		return -EINVAL;
>  
>  	/* TRCPDCR is not accessible with system instructions. */
> -	if (!desc.access.io_mem ||
> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> +	if (!desc.access.io_mem)
>  		drvdata->skip_power_up = true;

I prefer to move the condition checking for "desc.access.io_mem" to
etm4_init_sysreg_access(), this can make sure the flag skip_power_up
is set correctly based on property of system register access.

A side topic, in the mainline kernel I found the value
"desc.access.io_mem" is always zero (see the initialized value in
etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
desc.access.io_mem to true in etm4_probe()?

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d39660a3e50c..cf2555c50abb 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1939,6 +1939,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
        if (drvdata->cpu < 0)
                return drvdata->cpu;
 
+       desc.access.io_mem = true;
        init_arg.drvdata = drvdata;
        init_arg.csa = &desc.access;
        init_arg.pid = etm_pid;
  
Thanks,
Leo

>  	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
> -- 
> 2.35.3
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Suzuki K Poulose]

On 13/09/2022 11:52, Leo Yan wrote:
> Hi Bhupesh,
> 
> On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
>> Some Qualcomm ETM implementations require skipping powering up
>> the trace unit, as the ETMs are in the same power domain as
>> their CPU cores.
>>
>> Via commit 5214b563588e ("coresight: etm4x: Add support for
>> sysreg only devices"), the setting of 'skip_power_up' flag was
>> moved after the 'etm4_init_arch_data' function is called, whereas
>> the flag value is itself used inside the function. This causes
>> a crash when ETM mode 'Low-power state behavior override' is set
>> on some Qualcomm parts.
>>
>> Fix the same.
>>
>> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> ---
>>   - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>>   - Addressed the review comments from Suzuki.
>>   - Rebased on linux-next.
>>
>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index d39660a3e50c..14c1c7869795 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>>   	if (!cpu_supports_sysreg_trace())
>>   		return false;
>>   
>> +	/*
>> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
>> +	 * as the ETMs are in the same power domain as their CPU cores.
>> +	 *
>> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
>> +	 * initialize it before the function is called.
>> +	 */
>> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>> +		drvdata->skip_power_up = true;
>> +
> 
> I personally think this sentence should be placed in the function
> etm4_probe(), you need to move it just before smp call
> etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
> to be respected.

Or we could move this to init_iomem_access() and explicitly set the flag
to true in init_sysreg_access().

> 
>>   	/*
>>   	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>>   	 */
>> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>>   		return -EINVAL;
>>   
>>   	/* TRCPDCR is not accessible with system instructions. */
>> -	if (!desc.access.io_mem ||
>> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>> +	if (!desc.access.io_mem)
>>   		drvdata->skip_power_up = true;
> 
> I prefer to move the condition checking for "desc.access.io_mem" to
> etm4_init_sysreg_access(), this can make sure the flag skip_power_up
> is set correctly based on property of system register access.

+1

> 
> A side topic, in the mainline kernel I found the value
> "desc.access.io_mem" is always zero (see the initialized value in
> etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
> desc.access.io_mem to true in etm4_probe()?

Thats not true. It is initialised at :

etm4_init_iomem_access():

	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);

Where

#define CSDEV_ACCESS_IOMEM(_addr)               \
         ((struct csdev_access)  {               \
                 .io_mem         = true,         \
                 .base           = (_addr),      \
         })



Suzuki

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Suzuki K Poulose]

On 13/09/2022 11:52, Leo Yan wrote:
> Hi Bhupesh,
> 
> On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
>> Some Qualcomm ETM implementations require skipping powering up
>> the trace unit, as the ETMs are in the same power domain as
>> their CPU cores.
>>
>> Via commit 5214b563588e ("coresight: etm4x: Add support for
>> sysreg only devices"), the setting of 'skip_power_up' flag was
>> moved after the 'etm4_init_arch_data' function is called, whereas
>> the flag value is itself used inside the function. This causes
>> a crash when ETM mode 'Low-power state behavior override' is set
>> on some Qualcomm parts.
>>
>> Fix the same.
>>
>> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> ---
>>   - v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>>   - Addressed the review comments from Suzuki.
>>   - Rebased on linux-next.
>>
>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index d39660a3e50c..14c1c7869795 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>>   	if (!cpu_supports_sysreg_trace())
>>   		return false;
>>   
>> +	/*
>> +	 * Some Qualcomm implementations require skipping powering up the trace unit,
>> +	 * as the ETMs are in the same power domain as their CPU cores.
>> +	 *
>> +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
>> +	 * initialize it before the function is called.
>> +	 */
>> +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>> +		drvdata->skip_power_up = true;
>> +
> 
> I personally think this sentence should be placed in the function
> etm4_probe(), you need to move it just before smp call
> etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
> to be respected.

Or we could move this to init_iomem_access() and explicitly set the flag
to true in init_sysreg_access().

> 
>>   	/*
>>   	 * ETMs implementing sysreg access must implement TRCDEVARCH.
>>   	 */
>> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
>>   		return -EINVAL;
>>   
>>   	/* TRCPDCR is not accessible with system instructions. */
>> -	if (!desc.access.io_mem ||
>> -	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>> +	if (!desc.access.io_mem)
>>   		drvdata->skip_power_up = true;
> 
> I prefer to move the condition checking for "desc.access.io_mem" to
> etm4_init_sysreg_access(), this can make sure the flag skip_power_up
> is set correctly based on property of system register access.

+1

> 
> A side topic, in the mainline kernel I found the value
> "desc.access.io_mem" is always zero (see the initialized value in
> etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
> desc.access.io_mem to true in etm4_probe()?

Thats not true. It is initialised at :

etm4_init_iomem_access():

	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);

Where

#define CSDEV_ACCESS_IOMEM(_addr)               \
         ((struct csdev_access)  {               \
                 .io_mem         = true,         \
                 .base           = (_addr),      \
         })



Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Leo Yan]

On Wed, Sep 14, 2022 at 03:43:53PM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> > > +	/*
> > > +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> > > +	 * as the ETMs are in the same power domain as their CPU cores.
> > > +	 *
> > > +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> > > +	 * initialize it before the function is called.
> > > +	 */
> > > +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> > > +		drvdata->skip_power_up = true;
> > > +
> > 
> > I personally think this sentence should be placed in the function
> > etm4_probe(), you need to move it just before smp call
> > etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
> > to be respected.
> 
> Or we could move this to init_iomem_access() and explicitly set the flag
> to true in init_sysreg_access().

Agreed, this is more clear.

[...]

> > A side topic, in the mainline kernel I found the value
> > "desc.access.io_mem" is always zero (see the initialized value in
> > etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
> > desc.access.io_mem to true in etm4_probe()?
> 
> Thats not true. It is initialised at :
> 
> etm4_init_iomem_access():
> 
> 	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
> 
> Where
> 
> #define CSDEV_ACCESS_IOMEM(_addr)               \
>         ((struct csdev_access)  {               \
>                 .io_mem         = true,         \
>                 .base           = (_addr),      \
>         })

Thanks a lot for explaination, sorry for noise.

Leo

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Leo Yan]

On Wed, Sep 14, 2022 at 03:43:53PM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> > > +	/*
> > > +	 * Some Qualcomm implementations require skipping powering up the trace unit,
> > > +	 * as the ETMs are in the same power domain as their CPU cores.
> > > +	 *
> > > +	 * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
> > > +	 * initialize it before the function is called.
> > > +	 */
> > > +	if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> > > +		drvdata->skip_power_up = true;
> > > +
> > 
> > I personally think this sentence should be placed in the function
> > etm4_probe(), you need to move it just before smp call
> > etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
> > to be respected.
> 
> Or we could move this to init_iomem_access() and explicitly set the flag
> to true in init_sysreg_access().

Agreed, this is more clear.

[...]

> > A side topic, in the mainline kernel I found the value
> > "desc.access.io_mem" is always zero (see the initialized value in
> > etm4_probe() and etm4_init_sysreg_access()).  Should we initialize
> > desc.access.io_mem to true in etm4_probe()?
> 
> Thats not true. It is initialised at :
> 
> etm4_init_iomem_access():
> 
> 	*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
> 
> Where
> 
> #define CSDEV_ACCESS_IOMEM(_addr)               \
>         ((struct csdev_access)  {               \
>                 .io_mem         = true,         \
>                 .base           = (_addr),      \
>         })

Thanks a lot for explaination, sorry for noise.

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Suzuki K Poulose]

Hi Bupesh,
\
On 14/09/2022 15:43, Suzuki K Poulose wrote:
> On 13/09/2022 11:52, Leo Yan wrote:
>> Hi Bhupesh,
>>
>> On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
>>> Some Qualcomm ETM implementations require skipping powering up
>>> the trace unit, as the ETMs are in the same power domain as
>>> their CPU cores.
>>>
>>> Via commit 5214b563588e ("coresight: etm4x: Add support for
>>> sysreg only devices"), the setting of 'skip_power_up' flag was
>>> moved after the 'etm4_init_arch_data' function is called, whereas
>>> the flag value is itself used inside the function. This causes
>>> a crash when ETM mode 'Low-power state behavior override' is set
>>> on some Qualcomm parts.
>>>
>>> Fix the same.
>>>
>>> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only 
>>> devices")
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> ---
>>>   - v1 can be seen here: 
>>> https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>>>   - Addressed the review comments from Suzuki.
>>>   - Rebased on linux-next.
>>>
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c 
>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index d39660a3e50c..14c1c7869795 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct 
>>> etmv4_drvdata *drvdata,
>>>       if (!cpu_supports_sysreg_trace())
>>>           return false;
>>> +    /*
>>> +     * Some Qualcomm implementations require skipping powering up 
>>> the trace unit,
>>> +     * as the ETMs are in the same power domain as their CPU cores.
>>> +     *
>>> +     * Since the 'skip_power_up' flag is used inside 
>>> 'etm4_init_arch_data' function,
>>> +     * initialize it before the function is called.
>>> +     */
>>> +    if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>>> +        drvdata->skip_power_up = true;
>>> +
>>
>> I personally think this sentence should be placed in the function
>> etm4_probe(), you need to move it just before smp call
>> etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
>> to be respected.
> 
> Or we could move this to init_iomem_access() and explicitly set the flag
> to true in init_sysreg_access().
> 
>>
>>>       /*
>>>        * ETMs implementing sysreg access must implement TRCDEVARCH.
>>>        */
>>> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void 
>>> __iomem *base, u32 etm_pid)
>>>           return -EINVAL;
>>>       /* TRCPDCR is not accessible with system instructions. */
>>> -    if (!desc.access.io_mem ||
>>> -        fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>>> +    if (!desc.access.io_mem)
>>>           drvdata->skip_power_up = true;
>>
>> I prefer to move the condition checking for "desc.access.io_mem" to
>> etm4_init_sysreg_access(), this can make sure the flag skip_power_up
>> is set correctly based on property of system register access.
> 
> +1
> 

Do you plan to send an updated patch for this one ?

Kind regards
Suzuki

Re: [PATCH v2] coresight: etm4x: Fix crash observed on Qcom ETM parts with 'Low power override'

[Suzuki K Poulose]

Hi Bupesh,
\
On 14/09/2022 15:43, Suzuki K Poulose wrote:
> On 13/09/2022 11:52, Leo Yan wrote:
>> Hi Bhupesh,
>>
>> On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
>>> Some Qualcomm ETM implementations require skipping powering up
>>> the trace unit, as the ETMs are in the same power domain as
>>> their CPU cores.
>>>
>>> Via commit 5214b563588e ("coresight: etm4x: Add support for
>>> sysreg only devices"), the setting of 'skip_power_up' flag was
>>> moved after the 'etm4_init_arch_data' function is called, whereas
>>> the flag value is itself used inside the function. This causes
>>> a crash when ETM mode 'Low-power state behavior override' is set
>>> on some Qualcomm parts.
>>>
>>> Fix the same.
>>>
>>> Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only 
>>> devices")
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> ---
>>>   - v1 can be seen here: 
>>> https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
>>>   - Addressed the review comments from Suzuki.
>>>   - Rebased on linux-next.
>>>
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c 
>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index d39660a3e50c..14c1c7869795 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -977,6 +977,16 @@ static bool etm4_init_sysreg_access(struct 
>>> etmv4_drvdata *drvdata,
>>>       if (!cpu_supports_sysreg_trace())
>>>           return false;
>>> +    /*
>>> +     * Some Qualcomm implementations require skipping powering up 
>>> the trace unit,
>>> +     * as the ETMs are in the same power domain as their CPU cores.
>>> +     *
>>> +     * Since the 'skip_power_up' flag is used inside 
>>> 'etm4_init_arch_data' function,
>>> +     * initialize it before the function is called.
>>> +     */
>>> +    if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>>> +        drvdata->skip_power_up = true;
>>> +
>>
>> I personally think this sentence should be placed in the function
>> etm4_probe(), you need to move it just before smp call
>> etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
>> to be respected.
> 
> Or we could move this to init_iomem_access() and explicitly set the flag
> to true in init_sysreg_access().
> 
>>
>>>       /*
>>>        * ETMs implementing sysreg access must implement TRCDEVARCH.
>>>        */
>>> @@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void 
>>> __iomem *base, u32 etm_pid)
>>>           return -EINVAL;
>>>       /* TRCPDCR is not accessible with system instructions. */
>>> -    if (!desc.access.io_mem ||
>>> -        fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
>>> +    if (!desc.access.io_mem)
>>>           drvdata->skip_power_up = true;
>>
>> I prefer to move the condition checking for "desc.access.io_mem" to
>> etm4_init_sysreg_access(), this can make sure the flag skip_power_up
>> is set correctly based on property of system register access.
> 
> +1
> 

Do you plan to send an updated patch for this one ?

Kind regards
Suzuki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel