diff mbox series

[2/2] coresight: trbe: Allocate platform data per device

Message ID 20230816141008.535450-2-suzuki.poulose@arm.com
State New
Headers
Series [v2,1/2] coresight: trbe: Fix TRBE potential sleep in atomic context |

Commit Message

Suzuki K Poulose Aug. 16, 2023, 2:10 p.m. UTC 
  Coresight TRBE driver shares a single platform data (which is empty btw).
However, with the commit 4e8fe7e5c3a5
("coresight: Store pointers to connections rather than an array of them")
the coresight core would free up the pdata, resulting in multiple attempts
to free the same pdata for TRBE instances. Fix this by allocating a pdata per
coresight_device.

Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
Link: https://lore.kernel.org/r/20230814093813.19152-3-hejunhao3@huawei.com
Reported-by: Junhao He <hejunhao3@huawei.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Anshuman Khandual Aug. 17, 2023, 6:37 a.m. UTC | #1 
Hi Suzuki,

Seems like this patch is going to conflict with the below proposed change

https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/

Please let me know how should we resolve this conflict.

On 8/16/23 19:40, Suzuki K Poulose wrote:
> Coresight TRBE driver shares a single platform data (which is empty btw).
> However, with the commit 4e8fe7e5c3a5
> ("coresight: Store pointers to connections rather than an array of them")
> the coresight core would free up the pdata, resulting in multiple attempts
> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
> coresight_device.
> 
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")

The above mentioned commit i.e 4e8fe7e5c3a5 seems to be a more recent one which
has triggered this problem. But would the problem be still there without that ?
Else 'Fixes:' tag would need changing.

> Link: https://lore.kernel.org/r/20230814093813.19152-3-hejunhao3@huawei.com
> Reported-by: Junhao He <hejunhao3@huawei.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 025f70adee47..d3d34a833f01 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1255,10 +1255,13 @@ static void arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp
>  	if (!desc.name)
>  		goto cpu_clear;
>  
> +	desc.pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(desc.pdata))
> +		goto cpu_clear;
> +
>  	desc.type = CORESIGHT_DEV_TYPE_SINK;
>  	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
>  	desc.ops = &arm_trbe_cs_ops;
> -	desc.pdata = dev_get_platdata(dev);
>  	desc.groups = arm_trbe_groups;
>  	desc.dev = dev;
>  	trbe_csdev = coresight_register(&desc);
> @@ -1482,7 +1485,6 @@ static void arm_trbe_remove_irq(struct trbe_drvdata *drvdata)
>  
>  static int arm_trbe_device_probe(struct platform_device *pdev)
>  {
> -	struct coresight_platform_data *pdata;
>  	struct trbe_drvdata *drvdata;
>  	struct device *dev = &pdev->dev;
>  	int ret;
> @@ -1497,12 +1499,7 @@ static int arm_trbe_device_probe(struct platform_device *pdev)
>  	if (!drvdata)
>  		return -ENOMEM;
>  
> -	pdata = coresight_get_platform_data(dev);
> -	if (IS_ERR(pdata))
> -		return PTR_ERR(pdata);
> -
>  	dev_set_drvdata(dev, drvdata);
> -	dev->platform_data = pdata;
>  	drvdata->pdev = pdev;
>  	ret = arm_trbe_probe_irq(pdev, drvdata);
>  	if (ret)
hejunhao Aug. 17, 2023, 8:47 a.m. UTC | #2 
On 2023/8/16 22:10, Suzuki K Poulose wrote:
> Coresight TRBE driver shares a single platform data (which is empty btw).
> However, with the commit 4e8fe7e5c3a5
> ("coresight: Store pointers to connections rather than an array of them")
> the coresight core would free up the pdata, resulting in multiple attempts
> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
> coresight_device.
>
> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> Link: https://lore.kernel.org/r/20230814093813.19152-3-hejunhao3@huawei.com
> Reported-by: Junhao He <hejunhao3@huawei.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Test-by: Junhao He <hejunhao3@huawei.com>

> ---
>   drivers/hwtracing/coresight/coresight-trbe.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 025f70adee47..d3d34a833f01 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -1255,10 +1255,13 @@ static void arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp
>   	if (!desc.name)
>   		goto cpu_clear;
>   
> +	desc.pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(desc.pdata))
> +		goto cpu_clear;
> +
>   	desc.type = CORESIGHT_DEV_TYPE_SINK;
>   	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
>   	desc.ops = &arm_trbe_cs_ops;
> -	desc.pdata = dev_get_platdata(dev);
>   	desc.groups = arm_trbe_groups;
>   	desc.dev = dev;
>   	trbe_csdev = coresight_register(&desc);
> @@ -1482,7 +1485,6 @@ static void arm_trbe_remove_irq(struct trbe_drvdata *drvdata)
>   
>   static int arm_trbe_device_probe(struct platform_device *pdev)
>   {
> -	struct coresight_platform_data *pdata;
>   	struct trbe_drvdata *drvdata;
>   	struct device *dev = &pdev->dev;
>   	int ret;
> @@ -1497,12 +1499,7 @@ static int arm_trbe_device_probe(struct platform_device *pdev)
>   	if (!drvdata)
>   		return -ENOMEM;
>   
> -	pdata = coresight_get_platform_data(dev);
> -	if (IS_ERR(pdata))
> -		return PTR_ERR(pdata);
> -
>   	dev_set_drvdata(dev, drvdata);
> -	dev->platform_data = pdata;
>   	drvdata->pdev = pdev;
>   	ret = arm_trbe_probe_irq(pdev, drvdata);
>   	if (ret)
James Clark Aug. 17, 2023, 9:24 a.m. UTC | #3 
On 17/08/2023 07:37, Anshuman Khandual wrote:
> Hi Suzuki,
> 
> Seems like this patch is going to conflict with the below proposed change
> 
> https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/
> 
> Please let me know how should we resolve this conflict.

We could merge them both, with the fixes: one first, just to acknowledge
that there was a problem. But I suppose your one will have to be rebased
on top.

> 
> On 8/16/23 19:40, Suzuki K Poulose wrote:
>> Coresight TRBE driver shares a single platform data (which is empty btw).
>> However, with the commit 4e8fe7e5c3a5
>> ("coresight: Store pointers to connections rather than an array of them")
>> the coresight core would free up the pdata, resulting in multiple attempts
>> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
>> coresight_device.
>>
>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
> 
> The above mentioned commit i.e 4e8fe7e5c3a5 seems to be a more recent one which
> has triggered this problem. But would the problem be still there without that ?
> Else 'Fixes:' tag would need changing.
> 

Yes I think the fixes tag should point to 4e8fe7e5c3a5.

>> Link: https://lore.kernel.org/r/20230814093813.19152-3-hejunhao3@huawei.com
>> Reported-by: Junhao He <hejunhao3@huawei.com>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  drivers/hwtracing/coresight/coresight-trbe.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index 025f70adee47..d3d34a833f01 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -1255,10 +1255,13 @@ static void arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp
>>  	if (!desc.name)
>>  		goto cpu_clear;
>>  
>> +	desc.pdata = coresight_get_platform_data(dev);
>> +	if (IS_ERR(desc.pdata))
>> +		goto cpu_clear;
>> +
>>  	desc.type = CORESIGHT_DEV_TYPE_SINK;
>>  	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
>>  	desc.ops = &arm_trbe_cs_ops;
>> -	desc.pdata = dev_get_platdata(dev);
>>  	desc.groups = arm_trbe_groups;
>>  	desc.dev = dev;
>>  	trbe_csdev = coresight_register(&desc);
>> @@ -1482,7 +1485,6 @@ static void arm_trbe_remove_irq(struct trbe_drvdata *drvdata)
>>  
>>  static int arm_trbe_device_probe(struct platform_device *pdev)
>>  {
>> -	struct coresight_platform_data *pdata;
>>  	struct trbe_drvdata *drvdata;
>>  	struct device *dev = &pdev->dev;
>>  	int ret;
>> @@ -1497,12 +1499,7 @@ static int arm_trbe_device_probe(struct platform_device *pdev)
>>  	if (!drvdata)
>>  		return -ENOMEM;
>>  
>> -	pdata = coresight_get_platform_data(dev);
>> -	if (IS_ERR(pdata))
>> -		return PTR_ERR(pdata);
>> -
>>  	dev_set_drvdata(dev, drvdata);
>> -	dev->platform_data = pdata;
>>  	drvdata->pdev = pdev;
>>  	ret = arm_trbe_probe_irq(pdev, drvdata);
>>  	if (ret)
Suzuki K Poulose Aug. 17, 2023, 10:01 a.m. UTC | #4 
On 17/08/2023 10:24, James Clark wrote:
> 
> 
> On 17/08/2023 07:37, Anshuman Khandual wrote:
>> Hi Suzuki,
>>
>> Seems like this patch is going to conflict with the below proposed change
>>
>> https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/
>>
>> Please let me know how should we resolve this conflict.
> 
> We could merge them both, with the fixes: one first, just to acknowledge
> that there was a problem. But I suppose your one will have to be rebased
> on top.
> 
>>
>> On 8/16/23 19:40, Suzuki K Poulose wrote:
>>> Coresight TRBE driver shares a single platform data (which is empty btw).
>>> However, with the commit 4e8fe7e5c3a5
>>> ("coresight: Store pointers to connections rather than an array of them")
>>> the coresight core would free up the pdata, resulting in multiple attempts
>>> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
>>> coresight_device.
>>>
>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>
>> The above mentioned commit i.e 4e8fe7e5c3a5 seems to be a more recent one which
>> has triggered this problem. But would the problem be still there without that ?
>> Else 'Fixes:' tag would need changing.
>>
> 
> Yes I think the fixes tag should point to 4e8fe7e5c3a5.

Agreed, I will change the fixes tag and push this.

Suzuki
Suzuki K Poulose Aug. 17, 2023, 10:01 a.m. UTC | #5 
On 17/08/2023 07:37, Anshuman Khandual wrote:
> Hi Suzuki,
> 
> Seems like this patch is going to conflict with the below proposed change
> 
> https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/
> 
> Please let me know how should we resolve this conflict.

Please rebase your change on top of this one.

Suzuki
Anshuman Khandual Aug. 17, 2023, 10:16 a.m. UTC | #6 
On 8/17/23 15:31, Suzuki K Poulose wrote:
> On 17/08/2023 10:24, James Clark wrote:
>>
>>
>> On 17/08/2023 07:37, Anshuman Khandual wrote:
>>> Hi Suzuki,
>>>
>>> Seems like this patch is going to conflict with the below proposed change
>>>
>>> https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/
>>>
>>> Please let me know how should we resolve this conflict.
>>
>> We could merge them both, with the fixes: one first, just to acknowledge
>> that there was a problem. But I suppose your one will have to be rebased
>> on top.
>>
>>>
>>> On 8/16/23 19:40, Suzuki K Poulose wrote:
>>>> Coresight TRBE driver shares a single platform data (which is empty btw).
>>>> However, with the commit 4e8fe7e5c3a5
>>>> ("coresight: Store pointers to connections rather than an array of them")
>>>> the coresight core would free up the pdata, resulting in multiple attempts
>>>> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
>>>> coresight_device.
>>>>
>>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>>
>>> The above mentioned commit i.e 4e8fe7e5c3a5 seems to be a more recent one which
>>> has triggered this problem. But would the problem be still there without that ?
>>> Else 'Fixes:' tag would need changing.
>>>
>>
>> Yes I think the fixes tag should point to 4e8fe7e5c3a5.
> 
> Agreed, I will change the fixes tag and push this.

In the first patch, the last hunk might not be required to fix the
IPI problem and in fact might be bit problematic as well. Besides,
could you please hold off pushing this change into coresight tree
for some time ?
Suzuki K Poulose Aug. 17, 2023, 10:33 a.m. UTC | #7 
On 17/08/2023 11:16, Anshuman Khandual wrote:
> 
> 
> On 8/17/23 15:31, Suzuki K Poulose wrote:
>> On 17/08/2023 10:24, James Clark wrote:
>>>
>>>
>>> On 17/08/2023 07:37, Anshuman Khandual wrote:
>>>> Hi Suzuki,
>>>>
>>>> Seems like this patch is going to conflict with the below proposed change
>>>>
>>>> https://lore.kernel.org/all/20230817055405.249630-4-anshuman.khandual@arm.com/
>>>>
>>>> Please let me know how should we resolve this conflict.
>>>
>>> We could merge them both, with the fixes: one first, just to acknowledge
>>> that there was a problem. But I suppose your one will have to be rebased
>>> on top.
>>>
>>>>
>>>> On 8/16/23 19:40, Suzuki K Poulose wrote:
>>>>> Coresight TRBE driver shares a single platform data (which is empty btw).
>>>>> However, with the commit 4e8fe7e5c3a5
>>>>> ("coresight: Store pointers to connections rather than an array of them")
>>>>> the coresight core would free up the pdata, resulting in multiple attempts
>>>>> to free the same pdata for TRBE instances. Fix this by allocating a pdata per
>>>>> coresight_device.
>>>>>
>>>>> Fixes: 3fbf7f011f24 ("coresight: sink: Add TRBE driver")
>>>>
>>>> The above mentioned commit i.e 4e8fe7e5c3a5 seems to be a more recent one which
>>>> has triggered this problem. But would the problem be still there without that ?
>>>> Else 'Fixes:' tag would need changing.
>>>>
>>>
>>> Yes I think the fixes tag should point to 4e8fe7e5c3a5.
>>
>> Agreed, I will change the fixes tag and push this.
> 
> In the first patch, the last hunk might not be required to fix the
> IPI problem and in fact might be bit problematic as well. Besides,

Please could you comment your concerns on the patch ?

Suzuki

> could you please hold off pushing this change into coresight tree
> for some time ?
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 025f70adee47..d3d34a833f01 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1255,10 +1255,13 @@  static void arm_trbe_register_coresight_cpu(struct trbe_drvdata *drvdata, int cp
 	if (!desc.name)
 		goto cpu_clear;
 
+	desc.pdata = coresight_get_platform_data(dev);
+	if (IS_ERR(desc.pdata))
+		goto cpu_clear;
+
 	desc.type = CORESIGHT_DEV_TYPE_SINK;
 	desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
 	desc.ops = &arm_trbe_cs_ops;
-	desc.pdata = dev_get_platdata(dev);
 	desc.groups = arm_trbe_groups;
 	desc.dev = dev;
 	trbe_csdev = coresight_register(&desc);
@@ -1482,7 +1485,6 @@  static void arm_trbe_remove_irq(struct trbe_drvdata *drvdata)
 
 static int arm_trbe_device_probe(struct platform_device *pdev)
 {
-	struct coresight_platform_data *pdata;
 	struct trbe_drvdata *drvdata;
 	struct device *dev = &pdev->dev;
 	int ret;
@@ -1497,12 +1499,7 @@  static int arm_trbe_device_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	pdata = coresight_get_platform_data(dev);
-	if (IS_ERR(pdata))
-		return PTR_ERR(pdata);
-
 	dev_set_drvdata(dev, drvdata);
-	dev->platform_data = pdata;
 	drvdata->pdev = pdev;
 	ret = arm_trbe_probe_irq(pdev, drvdata);
 	if (ret)