[bug report] dmaengine: qcom_hidma: simplify DT resource parsing

[bug report] dmaengine: qcom_hidma: simplify DT resource parsing

[Dan Carpenter]

Hello Rob Herring,

Commit 37fa4905d22a ("dmaengine: qcom_hidma: simplify DT resource
parsing") from Jan 4, 2018 (linux-next), leads to the following
Smatch static checker warning:

	drivers/dma/qcom/hidma_mgmt.c:408 hidma_mgmt_of_populate_channels()
	warn: both zero and negative are errors 'ret'

drivers/dma/qcom/hidma_mgmt.c
    348 static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
    349 {
    350         struct platform_device *pdev_parent = of_find_device_by_node(np);
    351         struct platform_device_info pdevinfo;
    352         struct device_node *child;
    353         struct resource *res;
    354         int ret = 0;
    355 
    356         /* allocate a resource array */
    357         res = kcalloc(3, sizeof(*res), GFP_KERNEL);
    358         if (!res)
    359                 return -ENOMEM;
    360 
    361         for_each_available_child_of_node(np, child) {
    362                 struct platform_device *new_pdev;
    363 
    364                 ret = of_address_to_resource(child, 0, &res[0]);
    365                 if (!ret)
    366                         goto out;

This if statement seems reversed.  It will exit with success on the
first iteration through the loop.

    367 
    368                 ret = of_address_to_resource(child, 1, &res[1]);
    369                 if (!ret)
    370                         goto out;

Same.

    371 
    372                 ret = of_irq_to_resource(child, 0, &res[2]);
    373                 if (ret <= 0)
    374                         goto out;

This is actually what triggers the warning.  of_irq_to_resource()
returns a mix of negative error codes and zero on failure and positive
values on success.

    375 
    376                 memset(&pdevinfo, 0, sizeof(pdevinfo));
    377                 pdevinfo.fwnode = &child->fwnode;
    378                 pdevinfo.parent = pdev_parent ? &pdev_parent->dev : NULL;
    379                 pdevinfo.name = child->name;
    380                 pdevinfo.id = object_counter++;
    381                 pdevinfo.res = res;
    382                 pdevinfo.num_res = 3;
    383                 pdevinfo.data = NULL;
    384                 pdevinfo.size_data = 0;
    385                 pdevinfo.dma_mask = DMA_BIT_MASK(64);
    386                 new_pdev = platform_device_register_full(&pdevinfo);
    387                 if (IS_ERR(new_pdev)) {
    388                         ret = PTR_ERR(new_pdev);
    389                         goto out;
    390                 }
    391                 new_pdev->dev.of_node = child;
    392                 of_dma_configure(&new_pdev->dev, child, true);
    393                 /*
    394                  * It is assumed that calling of_msi_configure is safe on
    395                  * platforms with or without MSI support.
    396                  */
    397                 of_msi_configure(&new_pdev->dev, child);
    398         }
    399 
    400         kfree(res);
    401 
    402         return ret;

This should just be "return 0;" otherwise it's returning the positive
result from of_irq_to_resource().

    403 
    404 out:
    405         of_node_put(child);
    406         kfree(res);
    407 
--> 408         return ret;
    409 }

regards,
dan carpenter

Re: [bug report] dmaengine: qcom_hidma: simplify DT resource parsing

[Rob Herring]

On Tue, Apr 23, 2024 at 9:32 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hello Rob Herring,
>
> Commit 37fa4905d22a ("dmaengine: qcom_hidma: simplify DT resource
> parsing") from Jan 4, 2018 (linux-next), leads to the following
> Smatch static checker warning:
>
>         drivers/dma/qcom/hidma_mgmt.c:408 hidma_mgmt_of_populate_channels()
>         warn: both zero and negative are errors 'ret'
>
> drivers/dma/qcom/hidma_mgmt.c
>     348 static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
>     349 {
>     350         struct platform_device *pdev_parent = of_find_device_by_node(np);
>     351         struct platform_device_info pdevinfo;
>     352         struct device_node *child;
>     353         struct resource *res;
>     354         int ret = 0;
>     355
>     356         /* allocate a resource array */
>     357         res = kcalloc(3, sizeof(*res), GFP_KERNEL);
>     358         if (!res)
>     359                 return -ENOMEM;
>     360
>     361         for_each_available_child_of_node(np, child) {
>     362                 struct platform_device *new_pdev;
>     363
>     364                 ret = of_address_to_resource(child, 0, &res[0]);
>     365                 if (!ret)
>     366                         goto out;
>
> This if statement seems reversed.  It will exit with success on the
> first iteration through the loop.

Indeed! Obviously no one is using DT with this driver as it has been
broken since 2018. It's for a server platform as well, and DT usage is
rare to begin with on them (excluding all of IBM Power of course). So
I'll send patches removing DT support in this driver and the
associated binding.

Rob