[PATCH -next v2 0/4] soc: ti: Simplify with scoped for each OF child loop and dev_err_probe()

[PATCH -next v2 0/4] soc: ti: Simplify with scoped for each OF child loop and dev_err_probe()

[Jinjie Ruan]

Changes in v2:
- Rebased.
- Split from the whole soc patch set.
- Split Simplify with scoped for each OF child and dev_err_probe().
- Update the commit message.

Jinjie Ruan (4):
  soc: ti: knav_dma: Simplify with scoped for each OF child loop
  soc: ti: knav_dma: Use dev_err_probe() to simplfy code
  soc: ti: knav_qmss_queue: Simplify with scoped for each OF child loop
  soc: ti: knav_qmss_queue: Simplify with dev_err_probe()

 drivers/soc/ti/knav_dma.c        | 16 ++++--------
 drivers/soc/ti/knav_qmss_queue.c | 45 ++++++++++----------------------
 2 files changed, 19 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH -next v2 1/4] soc: ti: knav_dma: Simplify with scoped for each OF child loop

[Jinjie Ruan]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/soc/ti/knav_dma.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index fb0746d8caad..15e41d3a5e22 100644
--- a/drivers/soc/ti/knav_dma.c
+++ b/drivers/soc/ti/knav_dma.c
@@ -706,7 +706,6 @@ static int knav_dma_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
-	struct device_node *child;
 	int ret = 0;
 
 	if (!node) {
@@ -732,10 +731,9 @@ static int knav_dma_probe(struct platform_device *pdev)
 	}
 
 	/* Initialise all packet dmas */
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		ret = dma_init(node, child);
 		if (ret) {
-			of_node_put(child);
 			dev_err(&pdev->dev, "init failed with %d\n", ret);
 			break;
 		}
-- 
2.34.1

[PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Jinjie Ruan]

Use the dev_err_probe() helper to simplify error handling
during probe.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
---
 drivers/soc/ti/knav_dma.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index 15e41d3a5e22..eeec422a46f0 100644
--- a/drivers/soc/ti/knav_dma.c
+++ b/drivers/soc/ti/knav_dma.c
@@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
 	struct device_node *node = pdev->dev.of_node;
 	int ret = 0;
 
-	if (!node) {
-		dev_err(&pdev->dev, "could not find device info\n");
-		return -EINVAL;
-	}
+	if (!node)
+		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
 
 	kdev = devm_kzalloc(dev,
 			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
-	if (!kdev) {
-		dev_err(dev, "could not allocate driver mem\n");
-		return -ENOMEM;
-	}
+	if (!kdev)
+		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
 
 	kdev->dev = dev;
 	INIT_LIST_HEAD(&kdev->list);
-- 
2.34.1

[PATCH -next v2 3/4] soc: ti: knav_qmss_queue: Simplify with scoped for each OF child loop

[Jinjie Ruan]

Use scoped for_each_child_of_node_scoped() when iterating over device
nodes to make code a bit simpler.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
- Rebased the newest next.
- Update the commit message.
---
 drivers/soc/ti/knav_qmss_queue.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 6c98738e548a..02983f8ba1b6 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -1082,7 +1082,6 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
 	struct device_node *regions __free(device_node) =
 			of_get_child_by_name(node, "descriptor-regions");
 	struct knav_region *region;
-	struct device_node *child;
 	u32 temp[2];
 	int ret;
 
@@ -1090,10 +1089,9 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
 		return dev_err_probe(dev, -ENODEV,
 				     "descriptor-regions not specified\n");
 
-	for_each_child_of_node(regions, child) {
+	for_each_child_of_node_scoped(regions, child) {
 		region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
 		if (!region) {
-			of_node_put(child);
 			dev_err(dev, "out of memory allocating region\n");
 			return -ENOMEM;
 		}
@@ -1400,7 +1398,6 @@ static int knav_queue_init_qmgrs(struct knav_device *kdev,
 	struct device_node *qmgrs __free(device_node) =
 			of_get_child_by_name(node, "qmgrs");
 	struct knav_qmgr_info *qmgr;
-	struct device_node *child;
 	u32 temp[2];
 	int ret;
 
@@ -1408,10 +1405,9 @@ static int knav_queue_init_qmgrs(struct knav_device *kdev,
 		return dev_err_probe(dev, -ENODEV,
 				     "queue manager info not specified\n");
 
-	for_each_child_of_node(qmgrs, child) {
+	for_each_child_of_node_scoped(qmgrs, child) {
 		qmgr = devm_kzalloc(dev, sizeof(*qmgr), GFP_KERNEL);
 		if (!qmgr) {
-			of_node_put(child);
 			dev_err(dev, "out of memory allocating qmgr\n");
 			return -ENOMEM;
 		}
@@ -1506,12 +1502,10 @@ static int knav_queue_init_pdsps(struct knav_device *kdev,
 {
 	struct device *dev = kdev->dev;
 	struct knav_pdsp_info *pdsp;
-	struct device_node *child;
 
-	for_each_child_of_node(pdsps, child) {
+	for_each_child_of_node_scoped(pdsps, child) {
 		pdsp = devm_kzalloc(dev, sizeof(*pdsp), GFP_KERNEL);
 		if (!pdsp) {
-			of_node_put(child);
 			dev_err(dev, "out of memory allocating pdsp\n");
 			return -ENOMEM;
 		}
-- 
2.34.1

[PATCH -next v2 4/4] soc: ti: knav_qmss_queue: Simplify with dev_err_probe()

[Jinjie Ruan]

Use the dev_err_probe() helper to simplify error handling
during probe.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- Split into 2 patches.
- Rebased the newest next.
- Update the commit message.
---
 drivers/soc/ti/knav_qmss_queue.c | 33 +++++++++++---------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 02983f8ba1b6..d583a86028af 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -1091,10 +1091,8 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
 
 	for_each_child_of_node_scoped(regions, child) {
 		region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
-		if (!region) {
-			dev_err(dev, "out of memory allocating region\n");
-			return -ENOMEM;
-		}
+		if (!region)
+			return dev_err_probe(dev, -ENOMEM, "out of memory allocating region\n");
 
 		region->name = knav_queue_find_name(child);
 		of_property_read_u32(child, "id", &region->id);
@@ -1407,10 +1405,8 @@ static int knav_queue_init_qmgrs(struct knav_device *kdev,
 
 	for_each_child_of_node_scoped(qmgrs, child) {
 		qmgr = devm_kzalloc(dev, sizeof(*qmgr), GFP_KERNEL);
-		if (!qmgr) {
-			dev_err(dev, "out of memory allocating qmgr\n");
-			return -ENOMEM;
-		}
+		if (!qmgr)
+			return dev_err_probe(dev, -ENOMEM, "out of memory allocating qmgr\n");
 
 		ret = of_property_read_u32_array(child, "managed-queues",
 						 temp, 2);
@@ -1505,10 +1501,8 @@ static int knav_queue_init_pdsps(struct knav_device *kdev,
 
 	for_each_child_of_node_scoped(pdsps, child) {
 		pdsp = devm_kzalloc(dev, sizeof(*pdsp), GFP_KERNEL);
-		if (!pdsp) {
-			dev_err(dev, "out of memory allocating pdsp\n");
-			return -ENOMEM;
-		}
+		if (!pdsp)
+			return dev_err_probe(dev, -ENOMEM, "out of memory allocating pdsp\n");
 		pdsp->name = knav_queue_find_name(child);
 		pdsp->iram =
 			knav_queue_map_reg(kdev, child,
@@ -1784,16 +1778,12 @@ static int knav_queue_probe(struct platform_device *pdev)
 	u32 temp[2];
 	int ret;
 
-	if (!node) {
-		dev_err(dev, "device tree info unavailable\n");
-		return -ENODEV;
-	}
+	if (!node)
+		return dev_err_probe(dev, -ENODEV, "device tree info unavailable\n");
 
 	kdev = devm_kzalloc(dev, sizeof(struct knav_device), GFP_KERNEL);
-	if (!kdev) {
-		dev_err(dev, "memory allocation failed\n");
-		return -ENOMEM;
-	}
+	if (!kdev)
+		return dev_err_probe(dev, -ENOMEM, "memory allocation failed\n");
 
 	if (device_get_match_data(dev))
 		kdev->version = QMSS_66AK2G;
@@ -1810,8 +1800,7 @@ static int knav_queue_probe(struct platform_device *pdev)
 	ret = pm_runtime_resume_and_get(&pdev->dev);
 	if (ret < 0) {
 		pm_runtime_disable(&pdev->dev);
-		dev_err(dev, "Failed to enable QMSS\n");
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to enable QMSS\n");
 	}
 
 	if (of_property_read_u32_array(node, "queue-range", temp, 2)) {
-- 
2.34.1

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Nishanth Menon]

On 14:32-20240830, Jinjie Ruan wrote:
> Use the dev_err_probe() helper to simplify error handling
> during probe.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v2:
> - Split into 2 patches.
> ---
>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
> index 15e41d3a5e22..eeec422a46f0 100644
> --- a/drivers/soc/ti/knav_dma.c
> +++ b/drivers/soc/ti/knav_dma.c
> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>  	struct device_node *node = pdev->dev.of_node;
>  	int ret = 0;
>  
> -	if (!node) {
> -		dev_err(&pdev->dev, "could not find device info\n");
> -		return -EINVAL;
> -	}
> +	if (!node)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>  
>  	kdev = devm_kzalloc(dev,
>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
> -	if (!kdev) {
> -		dev_err(dev, "could not allocate driver mem\n");
> -		return -ENOMEM;
> -	}
> +	if (!kdev)
> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");

These make no sense to me :( -> just using dev_err_probe when there is
no chance of -EPROBE_DEFER ?

>  
>  	kdev->dev = dev;
>  	INIT_LIST_HEAD(&kdev->list);
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Krzysztof Kozlowski]

On 30/08/2024 12:31, Nishanth Menon wrote:
> On 14:32-20240830, Jinjie Ruan wrote:
>> Use the dev_err_probe() helper to simplify error handling
>> during probe.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> v2:
>> - Split into 2 patches.
>> ---
>>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
>> index 15e41d3a5e22..eeec422a46f0 100644
>> --- a/drivers/soc/ti/knav_dma.c
>> +++ b/drivers/soc/ti/knav_dma.c
>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>>  	struct device_node *node = pdev->dev.of_node;
>>  	int ret = 0;
>>  
>> -	if (!node) {
>> -		dev_err(&pdev->dev, "could not find device info\n");
>> -		return -EINVAL;
>> -	}
>> +	if (!node)
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>>  
>>  	kdev = devm_kzalloc(dev,
>>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
>> -	if (!kdev) {
>> -		dev_err(dev, "could not allocate driver mem\n");
>> -		return -ENOMEM;
>> -	}
>> +	if (!kdev)
>> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
> 
> These make no sense to me :( -> just using dev_err_probe when there is
> no chance of -EPROBE_DEFER ?

Well, this does not make sense from other point of view - memory
allocation errors should have any printks...

This patchset - like several others from Jinjie - is some sort of
automation without careful consideration and without thinking whether
code makes sense.

Therefore I suggest to review it thoroughly and do not trust the
"innocent" look of such changes.

Best regards,
Krzysztof

Re: [PATCH -next v2 4/4] soc: ti: knav_qmss_queue: Simplify with dev_err_probe()

[Nishanth Menon]

On 14:32-20240830, Jinjie Ruan wrote:
> Use the dev_err_probe() helper to simplify error handling
> during probe.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v2:
> - Split into 2 patches.
> - Rebased the newest next.
> - Update the commit message.
> ---
>  drivers/soc/ti/knav_qmss_queue.c | 33 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
> index 02983f8ba1b6..d583a86028af 100644
> --- a/drivers/soc/ti/knav_qmss_queue.c
> +++ b/drivers/soc/ti/knav_qmss_queue.c
> @@ -1091,10 +1091,8 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
>  
>  	for_each_child_of_node_scoped(regions, child) {
>  		region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
> -		if (!region) {
> -			dev_err(dev, "out of memory allocating region\n");
> -			return -ENOMEM;
> -		}
> +		if (!region)
> +			return dev_err_probe(dev, -ENOMEM, "out of memory allocating region\n");


These make no sense to me :( -> just using dev_err_probe when there is
no chance of -EPROBE_DEFER ?
>  
>  		region->name = knav_queue_find_name(child);
>  		of_property_read_u32(child, "id", &region->id);
> @@ -1407,10 +1405,8 @@ static int knav_queue_init_qmgrs(struct knav_device *kdev,
>  
>  	for_each_child_of_node_scoped(qmgrs, child) {
>  		qmgr = devm_kzalloc(dev, sizeof(*qmgr), GFP_KERNEL);
> -		if (!qmgr) {
> -			dev_err(dev, "out of memory allocating qmgr\n");
> -			return -ENOMEM;
> -		}
> +		if (!qmgr)
> +			return dev_err_probe(dev, -ENOMEM, "out of memory allocating qmgr\n");
>  

Neither this

>  		ret = of_property_read_u32_array(child, "managed-queues",
>  						 temp, 2);
> @@ -1505,10 +1501,8 @@ static int knav_queue_init_pdsps(struct knav_device *kdev,
>  
>  	for_each_child_of_node_scoped(pdsps, child) {
>  		pdsp = devm_kzalloc(dev, sizeof(*pdsp), GFP_KERNEL);
> -		if (!pdsp) {
> -			dev_err(dev, "out of memory allocating pdsp\n");
> -			return -ENOMEM;
> -		}
> +		if (!pdsp)
> +			return dev_err_probe(dev, -ENOMEM, "out of memory allocating pdsp\n");

or this

>  		pdsp->name = knav_queue_find_name(child);
>  		pdsp->iram =
>  			knav_queue_map_reg(kdev, child,
> @@ -1784,16 +1778,12 @@ static int knav_queue_probe(struct platform_device *pdev)
>  	u32 temp[2];
>  	int ret;
>  
> -	if (!node) {
> -		dev_err(dev, "device tree info unavailable\n");
> -		return -ENODEV;
> -	}
> +	if (!node)
> +		return dev_err_probe(dev, -ENODEV, "device tree info unavailable\n");
>  
>  	kdev = devm_kzalloc(dev, sizeof(struct knav_device), GFP_KERNEL);
> -	if (!kdev) {
> -		dev_err(dev, "memory allocation failed\n");
> -		return -ENOMEM;
> -	}
> +	if (!kdev)
> +		return dev_err_probe(dev, -ENOMEM, "memory allocation failed\n");

or here.

>  
>  	if (device_get_match_data(dev))
>  		kdev->version = QMSS_66AK2G;
> @@ -1810,8 +1800,7 @@ static int knav_queue_probe(struct platform_device *pdev)
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	if (ret < 0) {
>  		pm_runtime_disable(&pdev->dev);
> -		dev_err(dev, "Failed to enable QMSS\n");
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to enable QMSS\n");

Do you see -EPROBE_DEFER chance here? Please explain.

>  	}
>  
>  	if (of_property_read_u32_array(node, "queue-range", temp, 2)) {
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Krzysztof Kozlowski]

On 30/08/2024 12:36, Krzysztof Kozlowski wrote:
> On 30/08/2024 12:31, Nishanth Menon wrote:
>> On 14:32-20240830, Jinjie Ruan wrote:
>>> Use the dev_err_probe() helper to simplify error handling
>>> during probe.
>>>
>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>> ---
>>> v2:
>>> - Split into 2 patches.
>>> ---
>>>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
>>> index 15e41d3a5e22..eeec422a46f0 100644
>>> --- a/drivers/soc/ti/knav_dma.c
>>> +++ b/drivers/soc/ti/knav_dma.c
>>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>>>  	struct device_node *node = pdev->dev.of_node;
>>>  	int ret = 0;
>>>  
>>> -	if (!node) {
>>> -		dev_err(&pdev->dev, "could not find device info\n");
>>> -		return -EINVAL;
>>> -	}
>>> +	if (!node)
>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>>>  
>>>  	kdev = devm_kzalloc(dev,
>>>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
>>> -	if (!kdev) {
>>> -		dev_err(dev, "could not allocate driver mem\n");
>>> -		return -ENOMEM;
>>> -	}
>>> +	if (!kdev)
>>> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
>>
>> These make no sense to me :( -> just using dev_err_probe when there is
>> no chance of -EPROBE_DEFER ?
> 
> Well, this does not make sense from other point of view - memory
> allocation errors should have any printks...

s/should/should not/

obviously :)

> 
> This patchset - like several others from Jinjie - is some sort of
> automation without careful consideration and without thinking whether
> code makes sense.
> 
> Therefore I suggest to review it thoroughly and do not trust the
> "innocent" look of such changes.


Best regards,
Krzysztof

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Jinjie Ruan]

On 2024/8/30 18:31, Nishanth Menon wrote:
> On 14:32-20240830, Jinjie Ruan wrote:
>> Use the dev_err_probe() helper to simplify error handling
>> during probe.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> v2:
>> - Split into 2 patches.
>> ---
>>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
>> index 15e41d3a5e22..eeec422a46f0 100644
>> --- a/drivers/soc/ti/knav_dma.c
>> +++ b/drivers/soc/ti/knav_dma.c
>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>>  	struct device_node *node = pdev->dev.of_node;
>>  	int ret = 0;
>>  
>> -	if (!node) {
>> -		dev_err(&pdev->dev, "could not find device info\n");
>> -		return -EINVAL;
>> -	}
>> +	if (!node)
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>>  
>>  	kdev = devm_kzalloc(dev,
>>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
>> -	if (!kdev) {
>> -		dev_err(dev, "could not allocate driver mem\n");
>> -		return -ENOMEM;
>> -	}
>> +	if (!kdev)
>> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
> 
> These make no sense to me :( -> just using dev_err_probe when there is
> no chance of -EPROBE_DEFER ?

I noticed a change in dev_err_probe() this year, which is described in
this patch:

For an out-of-memory error there should be no additional output. Adapt
dev_err_probe() to not emit the error message when err is -ENOMEM.
This simplifies handling errors that might among others be -ENOMEM.


And the comment of dev_err_probe() said below:

* Using this helper in your probe function is totally fine even if @err
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
is
~~
 * known to never be -EPROBE_DEFER.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

https://lore.kernel.org/all/3d1e308d45cddf67749522ca42d83f5b4f0b9634.1718311756.git.u.kleine-koenig@baylibre.com/
> 
>>  
>>  	kdev->dev = dev;
>>  	INIT_LIST_HEAD(&kdev->list);
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Krzysztof Kozlowski]

On 31/08/2024 03:59, Jinjie Ruan wrote:
> 
> 
> On 2024/8/30 18:31, Nishanth Menon wrote:
>> On 14:32-20240830, Jinjie Ruan wrote:
>>> Use the dev_err_probe() helper to simplify error handling
>>> during probe.
>>>
>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>> ---
>>> v2:
>>> - Split into 2 patches.
>>> ---
>>>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
>>> index 15e41d3a5e22..eeec422a46f0 100644
>>> --- a/drivers/soc/ti/knav_dma.c
>>> +++ b/drivers/soc/ti/knav_dma.c
>>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>>>  	struct device_node *node = pdev->dev.of_node;
>>>  	int ret = 0;
>>>  
>>> -	if (!node) {
>>> -		dev_err(&pdev->dev, "could not find device info\n");
>>> -		return -EINVAL;
>>> -	}
>>> +	if (!node)
>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>>>  
>>>  	kdev = devm_kzalloc(dev,
>>>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
>>> -	if (!kdev) {
>>> -		dev_err(dev, "could not allocate driver mem\n");
>>> -		return -ENOMEM;
>>> -	}
>>> +	if (!kdev)
>>> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
>>
>> These make no sense to me :( -> just using dev_err_probe when there is
>> no chance of -EPROBE_DEFER ?
> 
> I noticed a change in dev_err_probe() this year, which is described in
> this patch:
> 
> For an out-of-memory error there should be no additional output. Adapt
> dev_err_probe() to not emit the error message when err is -ENOMEM.
> This simplifies handling errors that might among others be -ENOMEM.
> 
> 
> And the comment of dev_err_probe() said below:
> 
> * Using this helper in your probe function is totally fine even if @err

Fine but not much useful and at the same time huge churn from @vivo.com.

Best regards,
Krzysztof

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Jinjie Ruan]

On 2024/8/31 13:21, Krzysztof Kozlowski wrote:
> On 31/08/2024 03:59, Jinjie Ruan wrote:
>>
>>
>> On 2024/8/30 18:31, Nishanth Menon wrote:
>>> On 14:32-20240830, Jinjie Ruan wrote:
>>>> Use the dev_err_probe() helper to simplify error handling
>>>> during probe.
>>>>
>>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>>> ---
>>>> v2:
>>>> - Split into 2 patches.
>>>> ---
>>>>  drivers/soc/ti/knav_dma.c | 12 ++++--------
>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
>>>> index 15e41d3a5e22..eeec422a46f0 100644
>>>> --- a/drivers/soc/ti/knav_dma.c
>>>> +++ b/drivers/soc/ti/knav_dma.c
>>>> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
>>>>  	struct device_node *node = pdev->dev.of_node;
>>>>  	int ret = 0;
>>>>  
>>>> -	if (!node) {
>>>> -		dev_err(&pdev->dev, "could not find device info\n");
>>>> -		return -EINVAL;
>>>> -	}
>>>> +	if (!node)
>>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
>>>>  
>>>>  	kdev = devm_kzalloc(dev,
>>>>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
>>>> -	if (!kdev) {
>>>> -		dev_err(dev, "could not allocate driver mem\n");
>>>> -		return -ENOMEM;
>>>> -	}
>>>> +	if (!kdev)
>>>> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
>>>
>>> These make no sense to me :( -> just using dev_err_probe when there is
>>> no chance of -EPROBE_DEFER ?
>>
>> I noticed a change in dev_err_probe() this year, which is described in
>> this patch:
>>
>> For an out-of-memory error there should be no additional output. Adapt
>> dev_err_probe() to not emit the error message when err is -ENOMEM.
>> This simplifies handling errors that might among others be -ENOMEM.
>>
>>
>> And the comment of dev_err_probe() said below:
>>
>> * Using this helper in your probe function is totally fine even if @err
> 
> Fine but not much useful and at the same time huge churn from @vivo.com.

I see, it is fine with these dev_err_probe() dropped, the main is the
scoped child iterate.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH -next v2 4/4] soc: ti: knav_qmss_queue: Simplify with dev_err_probe()

[Krzysztof Kozlowski]

On 30/08/2024 08:32, Jinjie Ruan wrote:
> Use the dev_err_probe() helper to simplify error handling
> during probe.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v2:
> - Split into 2 patches.
> - Rebased the newest next.
> - Update the commit message.
> ---
>  drivers/soc/ti/knav_qmss_queue.c | 33 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
> index 02983f8ba1b6..d583a86028af 100644
> --- a/drivers/soc/ti/knav_qmss_queue.c
> +++ b/drivers/soc/ti/knav_qmss_queue.c
> @@ -1091,10 +1091,8 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
>  
>  	for_each_child_of_node_scoped(regions, child) {
>  		region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
> -		if (!region) {
> -			dev_err(dev, "out of memory allocating region\n");
> -			return -ENOMEM;
> -		}
> +		if (!region)
> +			return dev_err_probe(dev, -ENOMEM, "out of memory allocating region\n");

This as well does not make any sense. Please read carefully coding style
and then use coccinelle on all your contributions.

You send useless changes without understanding how this works. The tools
would help you, but it seems you do not want to use them.

Best regards,
Krzysztof

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Andrew Lunn]

> > Fine but not much useful and at the same time huge churn from @vivo.com.
> 
> I see, it is fine with these dev_err_probe() dropped, the main is the
> scoped child iterate.

Which is also a lot of churn.

How about you start with those that are actually broken, missing a
put. Those changes are useful. Changing code which is already correct
has a lot less value.

	Andrew

Re: [PATCH -next v2 2/4] soc: ti: knav_dma: Use dev_err_probe() to simplfy code

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2863 bytes --]

Hello,

On Sat, Aug 31, 2024 at 09:59:33AM +0800, Jinjie Ruan wrote:
> On 2024/8/30 18:31, Nishanth Menon wrote:
> > On 14:32-20240830, Jinjie Ruan wrote:
> >> Use the dev_err_probe() helper to simplify error handling
> >> during probe.
> >>
> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> >> ---
> >> v2:
> >> - Split into 2 patches.
> >> ---
> >>  drivers/soc/ti/knav_dma.c | 12 ++++--------
> >>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
> >> index 15e41d3a5e22..eeec422a46f0 100644
> >> --- a/drivers/soc/ti/knav_dma.c
> >> +++ b/drivers/soc/ti/knav_dma.c
> >> @@ -708,17 +708,13 @@ static int knav_dma_probe(struct platform_device *pdev)
> >>  	struct device_node *node = pdev->dev.of_node;
> >>  	int ret = 0;
> >>  
> >> -	if (!node) {
> >> -		dev_err(&pdev->dev, "could not find device info\n");
> >> -		return -EINVAL;
> >> -	}
> >> +	if (!node)
> >> +		return dev_err_probe(&pdev->dev, -EINVAL, "could not find device info\n");
> >>  
> >>  	kdev = devm_kzalloc(dev,
> >>  			sizeof(struct knav_dma_pool_device), GFP_KERNEL);
> >> -	if (!kdev) {
> >> -		dev_err(dev, "could not allocate driver mem\n");
> >> -		return -ENOMEM;
> >> -	}
> >> +	if (!kdev)
> >> +		return dev_err_probe(dev, -ENOMEM, "could not allocate driver mem\n");
> > 
> > These make no sense to me :( -> just using dev_err_probe when there is
> > no chance of -EPROBE_DEFER ?
> 
> I noticed a change in dev_err_probe() this year, which is described in
> this patch:
> 
> For an out-of-memory error there should be no additional output. Adapt
> dev_err_probe() to not emit the error message when err is -ENOMEM.
> This simplifies handling errors that might among others be -ENOMEM.

Notice this was carefully worded. Calling dev_err_probe() if you know
that the error is ENOMEM isn't helpful. The change was introduced to
simplify

	ret = some_function_that_might_return_ENOMEM_and_other_errors()
	if (ret == -ENOMEM)
		return ret;
	else if (ret)
		return dev_err_probe(dev, ret, "....");

to

	ret = some_function_that_might_return_ENOMEM_and_other_errors()
	if (ret)
		return dev_err_probe(dev, ret, "....");

But adding a dev_err_probe() if you know in the ret != 0 branch that ret
must be -ENOMEM, actively adding a dev_err_probe() gives very little
improvement. The main effect is to increase the size of the resulting
kernel image (or module).

Back when I made dev_err_probe() silent for -ENOMEM, it was suggested to
even make it fail to compile if ret is ENOMEM.

I wouldn't necessarily object to new code using dev_err_probe() to check
the return value of a function that can only return (success or)
-ENOMEM, but I agree to Krzysztof that changing existing code is little
helpful.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]