Re: Coresight causes synchronous external abort on msm8916

[Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>]

Hi Suzuki,

On 6/20/2019 2:36 PM, Suzuki K Poulose wrote:
> 
> 
> We are not yet there in the Coresight driver and we crash at AMBA bus layer
> trying to read the PID of the CoreSight device. So I doubt if this is an
> issue your patch trying to address. I still think this is a debug power 
> domain
> issue. More your patch below.

Yes, I suppose you are right. Just for testing, I had disabled psci
enable method for non boot cpus on msm8916 and it just crashed without
any traces. So, I thought maybe that could have been a reason for 
Stephan's crash as well.

> 
>> like cpu affinity issue. Can you please try out this patch and let us
>> know?
> 
> In general I am for the patch, breaking the "assumption" that a missing CPU
> phandle gives you the affinity of "0".
> 
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> index e8819d750938..9acf9f190d42 100644
>> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> @@ -579,7 +579,11 @@ static int debug_probe(struct amba_device *adev,
>> const struct amba_id *id)
>>        if (!drvdata)
>>            return -ENOMEM;
>>
>> -    drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
>> +    drvdata->cpu = np ? of_coresight_get_cpu(np) : -ENODEV;
> 
> 
> of_coresight_get_cpu() must be modified to return -ENODEV, rather than
> defaulting to 0. This is something that is required by the CTI driver too.
> And lets not bring up something and assume it belongs to CPU0.
> 
>> +    if (drvdata->cpu == -ENODEV) {
>> +        return -ENODEV;
>> +    }
>> +
>>        if (per_cpu(debug_drvdata, drvdata->cpu)) {
>>            dev_err(dev, "CPU%d drvdata has already been initialized\n",
>>                drvdata->cpu);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
>> b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 8bb0092c7ec2..660432acbac0 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -1107,7 +1107,10 @@ static int etm4_probe(struct amba_device *adev,
>> const struct amba_id *id)
>>
>>        spin_lock_init(&drvdata->spinlock);
>>
>> -    drvdata->cpu = pdata ? pdata->cpu : 0;
> 
> I believe, we should simply abort when we don't have pdata. There is no 
> point
> in registering this ETM unless we know where this is connected to.
> 

I did not understand this comment since I am returning with ENODEV here
and not registering this ETM.

>> +    drvdata->cpu = pdata ? pdata->cpu : -ENODEV;
>> +    if (drvdata->cpu == -ENODEV) {
>> +        return -ENODEV;
>> +       }
> 
>>
>>        cpus_read_lock();
>>        etmdrvdata[drvdata->cpu] = drvdata;
>> diff --git a/drivers/hwtracing/coresight/of_coresight.c
>> b/drivers/hwtracing/coresight/of_coresight.c
>> index 7045930fc958..8c1b90ba233c 100644
>> --- a/drivers/hwtracing/coresight/of_coresight.c
>> +++ b/drivers/hwtracing/coresight/of_coresight.c
>> @@ -153,14 +153,14 @@ int of_coresight_get_cpu(const struct device_node
>> *node)
>>        struct device_node *dn;
>>
>>        dn = of_parse_phandle(node, "cpu", 0);
>> -    /* Affinity defaults to CPU0 */
>> +    /* Affinity defaults to invalid */
>>        if (!dn)
>> -        return 0;
>> +        return -ENODEV;
>>        cpu = of_cpu_node_to_id(dn);
>>        of_node_put(dn);
>>
>> -    /* Affinity to CPU0 if no cpu nodes are found */
>> -    return (cpu < 0) ? 0 : cpu;
>> +    /* Affinity to invalid if no cpu nodes are found */
>> +    return (cpu < 0) ? -ENODEV : cpu;
> 
>      return cpu ?
> 
> If you split this into 3 different patches, I would be happy to Ack them.
>

Sure, I will ready the patches.

Thanks,
Sai

> Mathieu,
> 
> What do you think ?
> 
> 
> Cheers
> Suzuki

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation