Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] coresight: Set affinity to invalid for missing CPU phandle
@ 2019-06-20 13:45 Sai Prakash Ranjan
  2019-06-20 13:45 ` [PATCH 1/2] " Sai Prakash Ranjan
  2019-06-20 13:45 ` [PATCH 2/2] coresight: Abort probe " Sai Prakash Ranjan
  0 siblings, 2 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 13:45 UTC (permalink / raw)
  To: Mathieu Poirier, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
	Andy Gross, David Brown, Mark Rutland
  Cc: Rajendra Nayak, Vivek Gautam, Sibi Sankar, linux-arm-kernel,
	linux-kernel, linux-arm-msm, Sai Prakash Ranjan

In case of missing CPU phandle, the affinity is set default to
CPU0 which is not a correct assumption and leads to crashes
in few cases. Fix this by returning -ENODEV in coresight
platform and abort the probe in coresight etm and cpu-debug
drivers.

Sai Prakash Ranjan (2):
  coresight: Set affinity to invalid for missing CPU phandle
  coresight: Abort probe for missing CPU phandle

 drivers/hwtracing/coresight/coresight-cpu-debug.c |  3 +++
 drivers/hwtracing/coresight/coresight-etm3x.c     |  3 +++
 drivers/hwtracing/coresight/coresight-etm4x.c     |  3 +++
 drivers/hwtracing/coresight/coresight-platform.c  | 10 ++++++----
 4 files changed, 15 insertions(+), 4 deletions(-)

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] coresight: Set affinity to invalid for missing CPU phandle
  2019-06-20 13:45 [PATCH 0/2] coresight: Set affinity to invalid for missing CPU phandle Sai Prakash Ranjan
@ 2019-06-20 13:45 ` " Sai Prakash Ranjan
  2019-06-20 13:55   ` Suzuki K Poulose
  2019-06-20 17:39   ` Mathieu Poirier
  2019-06-20 13:45 ` [PATCH 2/2] coresight: Abort probe " Sai Prakash Ranjan
  1 sibling, 2 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 13:45 UTC (permalink / raw)
  To: Mathieu Poirier, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
	Andy Gross, David Brown, Mark Rutland
  Cc: Rajendra Nayak, Vivek Gautam, Sibi Sankar, linux-arm-kernel,
	linux-kernel, linux-arm-msm, Sai Prakash Ranjan

Affinity defaults to CPU0 in case of missing CPU phandle
and this leads to crashes in some cases because of such
wrong assumption. Fix this by returning -ENODEV in
coresight platform for such cases and then handle it
in the coresight drivers.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-platform.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 3c5ceda8db24..b1ea60c210e1 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -160,15 +160,17 @@ static int of_coresight_get_cpu(struct device *dev)
 
 	if (!dev->of_node)
 		return 0;
+
 	dn = of_parse_phandle(dev->of_node, "cpu", 0);
-	/* Affinity defaults to CPU0 */
+
+	/* Affinity defaults to invalid if no cpu nodes are found*/
 	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;
+	return cpu;
 }
 
 /*
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 13:45 [PATCH 0/2] coresight: Set affinity to invalid for missing CPU phandle Sai Prakash Ranjan
  2019-06-20 13:45 ` [PATCH 1/2] " Sai Prakash Ranjan
@ 2019-06-20 13:45 ` " Sai Prakash Ranjan
  2019-06-20 13:58   ` Suzuki K Poulose
  2019-06-20 17:43   ` Mathieu Poirier
  1 sibling, 2 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 13:45 UTC (permalink / raw)
  To: Mathieu Poirier, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
	Andy Gross, David Brown, Mark Rutland
  Cc: Rajendra Nayak, Vivek Gautam, Sibi Sankar, linux-arm-kernel,
	linux-kernel, linux-arm-msm, Sai Prakash Ranjan

Currently the coresight etm and cpu-debug drivers
assume the affinity to CPU0 returned by coresight
platform and continue the probe in case of missing
CPU phandle. This is not true and leads to crash
in some cases, so abort the probe in case of missing
CPU phandle.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +++
 drivers/hwtracing/coresight/coresight-etm3x.c     | 3 +++
 drivers/hwtracing/coresight/coresight-etm4x.c     | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 07a1367c733f..43f32fa71ff9 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev, const struct amba_id *id)
 		return -ENOMEM;
 
 	drvdata->cpu = coresight_get_cpu(dev);
+	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-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c
index 225c2982e4fe..882e2751746c 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x.c
@@ -816,6 +816,9 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	drvdata->cpu = coresight_get_cpu(dev);
+	if (drvdata->cpu == -ENODEV)
+		return -ENODEV;
+
 	desc.name  = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
 	if (!desc.name)
 		return -ENOMEM;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index 7fe266194ab5..97d71dbbeb19 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -1101,6 +1101,9 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 	spin_lock_init(&drvdata->spinlock);
 
 	drvdata->cpu = coresight_get_cpu(dev);
+	if (drvdata->cpu == -ENODEV)
+		return -ENODEV;
+
 	desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
 	if (!desc.name)
 		return -ENOMEM;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] coresight: Set affinity to invalid for missing CPU phandle
  2019-06-20 13:45 ` [PATCH 1/2] " Sai Prakash Ranjan
@ 2019-06-20 13:55   ` Suzuki K Poulose
  2019-06-20 14:54     ` Sai Prakash Ranjan
  2019-06-20 17:39   ` Mathieu Poirier
  1 sibling, 1 reply; 13+ messages in thread
From: Suzuki K Poulose @ 2019-06-20 13:55 UTC (permalink / raw)
  To: saiprakash.ranjan, mathieu.poirier, leo.yan, alexander.shishkin,
	andy.gross, david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm


Sai,

Thanks for the patch. Please could you change the subject to :

"coresight: Do not default to CPU0 for missing CPU phandle"

On 20/06/2019 14:45, Sai Prakash Ranjan wrote:
> Affinity defaults to CPU0 in case of missing CPU phandle
> and this leads to crashes in some cases because of such
> wrong assumption. Fix this by returning -ENODEV in

Thats not the right justification. Causing crashes is due to
bad DT/firmware. I would be happy with something like :

"Coresight platform support assumes that a missing \"cpu\" phandle
defaults to CPU0. This could be problematic and unnecessarily binds
components to CPU0, where they may not be. Let us make the DT binding
rules a bit stricter by not defaulting to CPU0 for missing "cpu"
affinity information."

Also, you must

1) update the devicetree/bindings document to reflect the same.
2) update the drivers to take appropriate action on the missing CPU
    where they are expected (e.g, CPU-debug, etm*), to prevent
    breaking a bisect.


> coresight platform for such cases and then handle it
> in the coresight drivers.
> 
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>   drivers/hwtracing/coresight/coresight-platform.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 3c5ceda8db24..b1ea60c210e1 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -160,15 +160,17 @@ static int of_coresight_get_cpu(struct device *dev)
>   
>   	if (!dev->of_node)
>   		return 0;
> +
>   	dn = of_parse_phandle(dev->of_node, "cpu", 0);
> -	/* Affinity defaults to CPU0 */
> +
> +	/* Affinity defaults to invalid if no cpu nodes are found*/

The code is self explanatory here. You could drop the comment.

>   	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;
> +	return cpu;
>   }
>   

Suzuki

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 13:45 ` [PATCH 2/2] coresight: Abort probe " Sai Prakash Ranjan
@ 2019-06-20 13:58   ` Suzuki K Poulose
  2019-06-20 14:55     ` Sai Prakash Ranjan
  2019-06-20 17:43   ` Mathieu Poirier
  1 sibling, 1 reply; 13+ messages in thread
From: Suzuki K Poulose @ 2019-06-20 13:58 UTC (permalink / raw)
  To: saiprakash.ranjan, mathieu.poirier, leo.yan, alexander.shishkin,
	david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm



On 20/06/2019 14:45, Sai Prakash Ranjan wrote:
> Currently the coresight etm and cpu-debug drivers
> assume the affinity to CPU0 returned by coresight
> platform and continue the probe in case of missing
> CPU phandle. This is not true and leads to crash
> in some cases, so abort the probe in case of missing
> CPU phandle.
> 
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>   drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +++
>   drivers/hwtracing/coresight/coresight-etm3x.c     | 3 +++
>   drivers/hwtracing/coresight/coresight-etm4x.c     | 3 +++
>   3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index 07a1367c733f..43f32fa71ff9 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>   		return -ENOMEM;
>   
>   	drvdata->cpu = coresight_get_cpu(dev);
> +	if (drvdata->cpu == -ENODEV)
> +		return -ENODEV;

if (drvdata->cpu < 0)
	return drvdata->cpu;

Same everywhere below ?

Also, I would like to hear Mathieu's thoughts on this change. If he's OK
with it:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> with the change above.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] coresight: Set affinity to invalid for missing CPU phandle
  2019-06-20 13:55   ` Suzuki K Poulose
@ 2019-06-20 14:54     ` Sai Prakash Ranjan
  0 siblings, 0 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 14:54 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, leo.yan, alexander.shishkin,
	andy.gross, david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm

Hi Suzuki,

Thanks for the review.

On 6/20/2019 7:25 PM, Suzuki K Poulose wrote:
> 
> Sai,
> 
> Thanks for the patch. Please could you change the subject to :
> 
> "coresight: Do not default to CPU0 for missing CPU phandle"
> 

Sure.

> On 20/06/2019 14:45, Sai Prakash Ranjan wrote:
>> Affinity defaults to CPU0 in case of missing CPU phandle
>> and this leads to crashes in some cases because of such
>> wrong assumption. Fix this by returning -ENODEV in
> 
> Thats not the right justification. Causing crashes is due to
> bad DT/firmware. I would be happy with something like :
> 
> "Coresight platform support assumes that a missing \"cpu\" phandle
> defaults to CPU0. This could be problematic and unnecessarily binds
> components to CPU0, where they may not be. Let us make the DT binding
> rules a bit stricter by not defaulting to CPU0 for missing "cpu"
> affinity information."
> 
> Also, you must
> 
> 1) update the devicetree/bindings document to reflect the same.
> 2) update the drivers to take appropriate action on the missing CPU
>     where they are expected (e.g, CPU-debug, etm*), to prevent
>     breaking a bisect.
> 
> 

Sure will do it and repost.

>> coresight platform for such cases and then handle it
>> in the coresight drivers.
>>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-platform.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c 
>> b/drivers/hwtracing/coresight/coresight-platform.c
>> index 3c5ceda8db24..b1ea60c210e1 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -160,15 +160,17 @@ static int of_coresight_get_cpu(struct device *dev)
>>       if (!dev->of_node)
>>           return 0;
>> +
>>       dn = of_parse_phandle(dev->of_node, "cpu", 0);
>> -    /* Affinity defaults to CPU0 */
>> +
>> +    /* Affinity defaults to invalid if no cpu nodes are found*/
> 
> The code is self explanatory here. You could drop the comment.
> 

Sure.

>>       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;
>> +    return cpu;
>>   }
> 
> Suzuki

-Sai

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 13:58   ` Suzuki K Poulose
@ 2019-06-20 14:55     ` Sai Prakash Ranjan
  2019-06-20 15:23       ` Suzuki K Poulose
  0 siblings, 1 reply; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 14:55 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, leo.yan, alexander.shishkin,
	david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm

On 6/20/2019 7:28 PM, Suzuki K Poulose wrote:
> 
> 
> On 20/06/2019 14:45, Sai Prakash Ranjan wrote:
>> Currently the coresight etm and cpu-debug drivers
>> assume the affinity to CPU0 returned by coresight
>> platform and continue the probe in case of missing
>> CPU phandle. This is not true and leads to crash
>> in some cases, so abort the probe in case of missing
>> CPU phandle.
>>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +++
>>   drivers/hwtracing/coresight/coresight-etm3x.c     | 3 +++
>>   drivers/hwtracing/coresight/coresight-etm4x.c     | 3 +++
>>   3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c 
>> b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> index 07a1367c733f..43f32fa71ff9 100644
>> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> @@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>           return -ENOMEM;
>>       drvdata->cpu = coresight_get_cpu(dev);
>> +    if (drvdata->cpu == -ENODEV)
>> +        return -ENODEV;
> 
> if (drvdata->cpu < 0)
>      return drvdata->cpu;
> 
> Same everywhere below ?
> 
> Also, I would like to hear Mathieu's thoughts on this change. If he's OK
> with it:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> with the change 
> above.
> 
> 

Thanks, I will make the change and repost.

-Sai

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 14:55     ` Sai Prakash Ranjan
@ 2019-06-20 15:23       ` Suzuki K Poulose
  2019-06-20 16:10         ` Sai Prakash Ranjan
  0 siblings, 1 reply; 13+ messages in thread
From: Suzuki K Poulose @ 2019-06-20 15:23 UTC (permalink / raw)
  To: saiprakash.ranjan, mathieu.poirier, leo.yan, alexander.shishkin,
	david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm



On 20/06/2019 15:55, Sai Prakash Ranjan wrote:
> On 6/20/2019 7:28 PM, Suzuki K Poulose wrote:
>>
>>
>> On 20/06/2019 14:45, Sai Prakash Ranjan wrote:
>>> Currently the coresight etm and cpu-debug drivers
>>> assume the affinity to CPU0 returned by coresight
>>> platform and continue the probe in case of missing
>>> CPU phandle. This is not true and leads to crash
>>> in some cases, so abort the probe in case of missing
>>> CPU phandle.
>>>
>>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> ---
>>>    drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +++
>>>    drivers/hwtracing/coresight/coresight-etm3x.c     | 3 +++
>>>    drivers/hwtracing/coresight/coresight-etm4x.c     | 3 +++
>>>    3 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>>> b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>>> index 07a1367c733f..43f32fa71ff9 100644
>>> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>>> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>>> @@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev,
>>> const struct amba_id *id)
>>>            return -ENOMEM;
>>>        drvdata->cpu = coresight_get_cpu(dev);
>>> +    if (drvdata->cpu == -ENODEV)
>>> +        return -ENODEV;
>>
>> if (drvdata->cpu < 0)
>>       return drvdata->cpu;
>>
>> Same everywhere below ?
>>
>> Also, I would like to hear Mathieu's thoughts on this change. If he's OK
>> with it:
>>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> with the change
>> above.
>>
>>
> 
> Thanks, I will make the change and repost.

Please wait for Mathieu's thoughts on it. And in general I would wait
for feedback from the people in a version, before posting another one,
to reduce the number of respins.

Cheers
Suzuki

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 15:23       ` Suzuki K Poulose
@ 2019-06-20 16:10         ` Sai Prakash Ranjan
  0 siblings, 0 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 16:10 UTC (permalink / raw)
  To: Suzuki K Poulose, mathieu.poirier, leo.yan, alexander.shishkin,
	david.brown, mark.rutland
  Cc: rnayak, vivek.gautam, sibis, linux-arm-kernel, linux-kernel,
	linux-arm-msm

On 6/20/2019 8:53 PM, Suzuki K Poulose wrote:
> 
> 
> 
> Please wait for Mathieu's thoughts on it. And in general I would wait
> for feedback from the people in a version, before posting another one,
> to reduce the number of respins.
> 

Mathieu already said he was OK in the other thread, but I will wait for
some more feedbacks.

Thanks,
Sai

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] coresight: Set affinity to invalid for missing CPU phandle
  2019-06-20 13:45 ` [PATCH 1/2] " Sai Prakash Ranjan
  2019-06-20 13:55   ` Suzuki K Poulose
@ 2019-06-20 17:39   ` Mathieu Poirier
  2019-06-20 18:05     ` Sai Prakash Ranjan
  1 sibling, 1 reply; 13+ messages in thread
From: Mathieu Poirier @ 2019-06-20 17:39 UTC (permalink / raw)
  To: Sai Prakash Ranjan
  Cc: Suzuki K Poulose, Leo Yan, Alexander Shishkin, Andy Gross,
	David Brown, Mark Rutland, Rajendra Nayak, Vivek Gautam,
	Sibi Sankar, linux-arm-kernel, linux-kernel, linux-arm-msm

Hi Sai,

On Thu, Jun 20, 2019 at 07:15:46PM +0530, Sai Prakash Ranjan wrote:
> Affinity defaults to CPU0 in case of missing CPU phandle
> and this leads to crashes in some cases because of such
> wrong assumption. Fix this by returning -ENODEV in
> coresight platform for such cases and then handle it
> in the coresight drivers.
> 
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight-platform.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 3c5ceda8db24..b1ea60c210e1 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -160,15 +160,17 @@ static int of_coresight_get_cpu(struct device *dev)
>  
>  	if (!dev->of_node)
>  		return 0;

An error should be returned if the above condition is true.  

> +

Spurious newline

>  	dn = of_parse_phandle(dev->of_node, "cpu", 0);
> -	/* Affinity defaults to CPU0 */
> +
> +	/* Affinity defaults to invalid if no cpu nodes are found*/
>  	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;
> +	return cpu;
>  }
>  
>  /*
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 13:45 ` [PATCH 2/2] coresight: Abort probe " Sai Prakash Ranjan
  2019-06-20 13:58   ` Suzuki K Poulose
@ 2019-06-20 17:43   ` Mathieu Poirier
  2019-06-20 18:07     ` Sai Prakash Ranjan
  1 sibling, 1 reply; 13+ messages in thread
From: Mathieu Poirier @ 2019-06-20 17:43 UTC (permalink / raw)
  To: Sai Prakash Ranjan
  Cc: Suzuki K Poulose, Leo Yan, Alexander Shishkin, Andy Gross,
	David Brown, Mark Rutland, Rajendra Nayak, Vivek Gautam,
	Sibi Sankar, linux-arm-kernel, linux-kernel, linux-arm-msm

On Thu, Jun 20, 2019 at 07:15:47PM +0530, Sai Prakash Ranjan wrote:
> Currently the coresight etm and cpu-debug drivers
> assume the affinity to CPU0 returned by coresight
> platform and continue the probe in case of missing
> CPU phandle. This is not true and leads to crash
> in some cases, so abort the probe in case of missing
> CPU phandle.
> 
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 3 +++
>  drivers/hwtracing/coresight/coresight-etm3x.c     | 3 +++
>  drivers/hwtracing/coresight/coresight-etm4x.c     | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index 07a1367c733f..43f32fa71ff9 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>  		return -ENOMEM;
>  
>  	drvdata->cpu = coresight_get_cpu(dev);
> +	if (drvdata->cpu == -ENODEV)
> +		return -ENODEV;

As Suzuki pointed out, simply return the error message conveyed by
coresight_get_cpu().

Also please merge both patches together to avoid bisect nightmare.

Thank you for the contribution,
Mathieu

> +
>  	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-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c
> index 225c2982e4fe..882e2751746c 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c
> @@ -816,6 +816,9 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
>  	}
>  
>  	drvdata->cpu = coresight_get_cpu(dev);
> +	if (drvdata->cpu == -ENODEV)
> +		return -ENODEV;
> +
>  	desc.name  = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
>  	if (!desc.name)
>  		return -ENOMEM;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 7fe266194ab5..97d71dbbeb19 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -1101,6 +1101,9 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  	spin_lock_init(&drvdata->spinlock);
>  
>  	drvdata->cpu = coresight_get_cpu(dev);
> +	if (drvdata->cpu == -ENODEV)
> +		return -ENODEV;
> +
>  	desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
>  	if (!desc.name)
>  		return -ENOMEM;
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] coresight: Set affinity to invalid for missing CPU phandle
  2019-06-20 17:39   ` Mathieu Poirier
@ 2019-06-20 18:05     ` Sai Prakash Ranjan
  0 siblings, 0 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 18:05 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Suzuki K Poulose, Leo Yan, Alexander Shishkin, Andy Gross,
	David Brown, Mark Rutland, Rajendra Nayak, Vivek Gautam,
	Sibi Sankar, linux-arm-kernel, linux-kernel, linux-arm-msm

Hi Mathieu,

Thanks for the review comments.

On 6/20/2019 11:09 PM, Mathieu Poirier wrote:
> Hi Sai,
> 
> On Thu, Jun 20, 2019 at 07:15:46PM +0530, Sai Prakash Ranjan wrote:
>> Affinity defaults to CPU0 in case of missing CPU phandle
>> and this leads to crashes in some cases because of such
>> wrong assumption. Fix this by returning -ENODEV in
>> coresight platform for such cases and then handle it
>> in the coresight drivers.
>>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-platform.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 3c5ceda8db24..b1ea60c210e1 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -160,15 +160,17 @@ static int of_coresight_get_cpu(struct device *dev)
>>   
>>   	if (!dev->of_node)
>>   		return 0;
> 
> An error should be returned if the above condition is true.
> 

Will do it, thanks.

>> +
> 
> Spurious newline
> 

This was on purpose, the code looks much cleaner.

-Sai

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] coresight: Abort probe for missing CPU phandle
  2019-06-20 17:43   ` Mathieu Poirier
@ 2019-06-20 18:07     ` Sai Prakash Ranjan
  0 siblings, 0 replies; 13+ messages in thread
From: Sai Prakash Ranjan @ 2019-06-20 18:07 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Suzuki K Poulose, Leo Yan, Alexander Shishkin, Andy Gross,
	David Brown, Mark Rutland, Rajendra Nayak, Vivek Gautam,
	Sibi Sankar, linux-arm-kernel, linux-kernel, linux-arm-msm

Hi Mathieu,

Thanks for the review comments.

On 6/20/2019 11:13 PM, Mathieu Poirier wrote:
> On Thu, Jun 20, 2019 at 07:15:47PM +0530, Sai Prakash Ranjan wrote:
>> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> index 07a1367c733f..43f32fa71ff9 100644
>> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> @@ -579,6 +579,9 @@ static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>>   		return -ENOMEM;
>>   
>>   	drvdata->cpu = coresight_get_cpu(dev);
>> +	if (drvdata->cpu == -ENODEV)
>> +		return -ENODEV;
> 
> As Suzuki pointed out, simply return the error message conveyed by
> coresight_get_cpu().
> 
> Also please merge both patches together to avoid bisect nightmare.

Sure, will do it.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-20 13:45 [PATCH 0/2] coresight: Set affinity to invalid for missing CPU phandle Sai Prakash Ranjan
2019-06-20 13:45 ` [PATCH 1/2] " Sai Prakash Ranjan
2019-06-20 13:55   ` Suzuki K Poulose
2019-06-20 14:54     ` Sai Prakash Ranjan
2019-06-20 17:39   ` Mathieu Poirier
2019-06-20 18:05     ` Sai Prakash Ranjan
2019-06-20 13:45 ` [PATCH 2/2] coresight: Abort probe " Sai Prakash Ranjan
2019-06-20 13:58   ` Suzuki K Poulose
2019-06-20 14:55     ` Sai Prakash Ranjan
2019-06-20 15:23       ` Suzuki K Poulose
2019-06-20 16:10         ` Sai Prakash Ranjan
2019-06-20 17:43   ` Mathieu Poirier
2019-06-20 18:07     ` Sai Prakash Ranjan

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git