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 <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Bhupesh Sharma <[email protected]>
---
- v1 can be seen here: https://lore.kernel.org/lkml/[email protected]/
- 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
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 <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Bhupesh Sharma <[email protected]>
> ---
> - v1 can be seen here: https://lore.kernel.org/lkml/[email protected]/
> - 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
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 <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Bhupesh Sharma <[email protected]>
> ---
> - v1 can be seen here: https://lore.kernel.org/lkml/[email protected]/
> - 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
>
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 <[email protected]>
>> Cc: Suzuki K Poulose <[email protected]>
>> Cc: Mathieu Poirier <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Signed-off-by: Bhupesh Sharma <[email protected]>
>> ---
>> - v1 can be seen here: https://lore.kernel.org/lkml/[email protected]/
>> - 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
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
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 <[email protected]>
>>> Cc: Suzuki K Poulose <[email protected]>
>>> Cc: Mathieu Poirier <[email protected]>
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Signed-off-by: Bhupesh Sharma <[email protected]>
>>> ---
>>> - v1 can be seen here:
>>> https://lore.kernel.org/lkml/[email protected]/
>>> - 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