| Message ID | 20240827114607.4019972-9-ruanjinjie@huawei.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | soc: Simplify with scoped for each OF child loop and dev_err_probe() | expand |
Jinjie Ruan <ruanjinjie@huawei.com> writes: > @@ -1080,17 +1080,13 @@ static int knav_queue_setup_regions(struct knav_device *kdev, > { > struct device *dev = kdev->dev; > struct knav_region *region; > - struct device_node *child; > u32 temp[2]; > int ret; > > - for_each_child_of_node(regions, child) { > + for_each_child_of_node_scoped(regions, child) { Are you sure using *_scoped() is better here? Since it seems that we need the memory pointed to by "child" in cases where we don't go into an error path. > 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; > - } > + 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", ®ion->id); Similarly in most of the other cases in this series where a similar change is done. Also FYI, as for dev_err_probe(), I think I covered all of them in this file and a patch for it is currently sitting in ti-drivers-soc-next. Thanks
On 21:28-20240829, Kousik Sanagavarapu wrote: > Jinjie Ruan <ruanjinjie@huawei.com> writes: > > @@ -1080,17 +1080,13 @@ static int knav_queue_setup_regions(struct knav_device *kdev, > > { > > struct device *dev = kdev->dev; > > struct knav_region *region; > > - struct device_node *child; > > u32 temp[2]; > > int ret; > > > > - for_each_child_of_node(regions, child) { > > + for_each_child_of_node_scoped(regions, child) { > > Are you sure using *_scoped() is better here? Since it seems that we > need the memory pointed to by "child" in cases where we don't go into an > error path. > > > 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; > > - } > > + 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", ®ion->id); > > Similarly in most of the other cases in this series where a similar > change is done. > > Also FYI, as for dev_err_probe(), I think I covered all of them in this > file and a patch for it is currently sitting in ti-drivers-soc-next. > Thanks Kousik. yeah - it will probably help with a rebase to latest next.
On 2024/8/29 23:58, Kousik Sanagavarapu wrote: > Jinjie Ruan <ruanjinjie@huawei.com> writes: >> @@ -1080,17 +1080,13 @@ static int knav_queue_setup_regions(struct knav_device *kdev, >> { >> struct device *dev = kdev->dev; >> struct knav_region *region; >> - struct device_node *child; >> u32 temp[2]; >> int ret; >> >> - for_each_child_of_node(regions, child) { >> + for_each_child_of_node_scoped(regions, child) { > > Are you sure using *_scoped() is better here? Since it seems that we > need the memory pointed to by "child" in cases where we don't go into an > error path. Hi, Jonathan, could you help review this code? > >> 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; >> - } >> + 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", ®ion->id); > > Similarly in most of the other cases in this series where a similar > change is done. > > Also FYI, as for dev_err_probe(), I think I covered all of them in this > file and a patch for it is currently sitting in ti-drivers-soc-next. > > Thanks
On Fri, 30 Aug 2024 11:24:14 +0800 Jinjie Ruan <ruanjinjie@huawei.com> wrote: > On 2024/8/29 23:58, Kousik Sanagavarapu wrote: > > Jinjie Ruan <ruanjinjie@huawei.com> writes: > >> @@ -1080,17 +1080,13 @@ static int knav_queue_setup_regions(struct knav_device *kdev, > >> { > >> struct device *dev = kdev->dev; > >> struct knav_region *region; > >> - struct device_node *child; > >> u32 temp[2]; > >> int ret; > >> > >> - for_each_child_of_node(regions, child) { > >> + for_each_child_of_node_scoped(regions, child) { > > > > Are you sure using *_scoped() is better here? Since it seems that we > > need the memory pointed to by "child" in cases where we don't go into an > > error path. > > Hi, Jonathan, could you help review this code? I don't understand the review comment. The reference counting before and after this patch is the same, just with the error path handled in a simpler fashion and the scope of the child variable reduced. > > > > >> 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; > >> - } > >> + 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", ®ion->id); > > > > Similarly in most of the other cases in this series where a similar > > change is done. > > > > Also FYI, as for dev_err_probe(), I think I covered all of them in this > > file and a patch for it is currently sitting in ti-drivers-soc-next. > > > > Thanks
diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c index a15eaa1900ab..5126863ed647 100644 --- a/drivers/soc/ti/knav_qmss_queue.c +++ b/drivers/soc/ti/knav_qmss_queue.c @@ -1080,17 +1080,13 @@ static int knav_queue_setup_regions(struct knav_device *kdev, { struct device *dev = kdev->dev; struct knav_region *region; - struct device_node *child; u32 temp[2]; int ret; - 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; - } + 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", ®ion->id); @@ -1116,10 +1112,8 @@ static int knav_queue_setup_regions(struct knav_device *kdev, INIT_LIST_HEAD(®ion->pools); list_add_tail(®ion->list, &kdev->regions); } - if (list_empty(&kdev->regions)) { - dev_err(dev, "no valid region information found\n"); - return -ENODEV; - } + if (list_empty(&kdev->regions)) + return dev_err_probe(dev, -ENODEV, "no valid region information found\n"); /* Next, we run through the regions and set things up */ for_each_region(kdev, region) @@ -1313,10 +1307,8 @@ static int knav_setup_queue_pools(struct knav_device *kdev, } /* ... and barf if they all failed! */ - if (list_empty(&kdev->queue_ranges)) { - dev_err(kdev->dev, "no valid queue range found\n"); - return -ENODEV; - } + if (list_empty(&kdev->queue_ranges)) + return dev_err_probe(kdev->dev, -ENODEV, "no valid queue range found\n"); return 0; } @@ -1388,17 +1380,13 @@ static int knav_queue_init_qmgrs(struct knav_device *kdev, { struct device *dev = kdev->dev; struct knav_qmgr_info *qmgr; - struct device_node *child; u32 temp[2]; int ret; - 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; - } + 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); @@ -1490,15 +1478,11 @@ 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; - } + 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, @@ -1755,16 +1739,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; @@ -1781,8 +1761,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)) {
Use scoped for_each_child_of_node_scoped() when iterating over device nodes to make code a bit simpler. And use the dev_err_probe() helper to simplify error handling during probe. This also handle scenario, when EDEFER is returned and useless error is printed. Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- drivers/soc/ti/knav_qmss_queue.c | 57 ++++++++++---------------------- 1 file changed, 18 insertions(+), 39 deletions(-)