[bug report] remoteproc: Add Renesas rcar driver

[bug report] remoteproc: Add Renesas rcar driver

[Dan Carpenter]

Hello Julien Massot,

The patch 285892a74f13: "remoteproc: Add Renesas rcar driver" from
Dec 7, 2021, leads to the following Smatch static checker warning:

	drivers/remoteproc/rcar_rproc.c:171 rcar_rproc_probe()
	warn: pm_runtime_get_sync() also returns 1 on success

drivers/remoteproc/rcar_rproc.c
    147 static int rcar_rproc_probe(struct platform_device *pdev)
    148 {
    149         struct device *dev = &pdev->dev;
    150         struct device_node *np = dev->of_node;
    151         struct rcar_rproc *priv;
    152         struct rproc *rproc;
    153         int ret;
    154 
    155         rproc = devm_rproc_alloc(dev, np->name, &rcar_rproc_ops,
    156                                 NULL, sizeof(*priv));
    157         if (!rproc)
    158                 return -ENOMEM;
    159 
    160         priv = rproc->priv;
    161 
    162         priv->rst = devm_reset_control_get_exclusive(dev, NULL);
    163         if (IS_ERR(priv->rst)) {
    164                 ret = PTR_ERR(priv->rst);
    165                 dev_err_probe(dev, ret, "fail to acquire rproc reset\n");
    166                 return ret;;
    167         }
    168 
    169         pm_runtime_enable(dev);
    170         ret = pm_runtime_get_sync(dev);
--> 171         if (ret) {

The pm_runtime_get_sync() returns both 0 and 1 on success.  The comments
to that function suggest that this should be changed to instead use:

	ret = pm_runtime_resume_and_get();
	if (ret) {

I've got no idea what that function does but it has standard error codes
and cleanup, so I *love* it.

    172                 dev_err(dev, "failed to power up\n");
    173                 return ret;
    174         }
    175 
    176         dev_set_drvdata(dev, rproc);
    177 
    178         /* Manually start the rproc */
    179         rproc->auto_boot = false;
    180 
    181         ret = devm_rproc_add(dev, rproc);
    182         if (ret) {
    183                 dev_err(dev, "rproc_add failed\n");
    184                 goto pm_disable;
    185         }
    186 
    187         return 0;
    188 
    189 pm_disable:
    190         pm_runtime_disable(dev);
    191 
    192         return ret;
    193 }

regards,
dan carpenter

Re: [bug report] remoteproc: Add Renesas rcar driver

[Julien Massot]

Hi Dan,

On 12/16/21 09:26, Dan Carpenter wrote:
> Hello Julien Massot,
> 
> The patch 285892a74f13: "remoteproc: Add Renesas rcar driver" from
> Dec 7, 2021, leads to the following Smatch static checker warning:
> 
> 	drivers/remoteproc/rcar_rproc.c:171 rcar_rproc_probe()
> 	warn: pm_runtime_get_sync() also returns 1 on success
Indeed the patch was not correct.
> 
> drivers/remoteproc/rcar_rproc.c
>      147 static int rcar_rproc_probe(struct platform_device *pdev)
>      148 {
>      149         struct device *dev = &pdev->dev;
>      150         struct device_node *np = dev->of_node;
>      151         struct rcar_rproc *priv;
>      152         struct rproc *rproc;
>      153         int ret;
>      154
>      155         rproc = devm_rproc_alloc(dev, np->name, &rcar_rproc_ops,
>      156                                 NULL, sizeof(*priv));
>      157         if (!rproc)
>      158                 return -ENOMEM;
>      159
>      160         priv = rproc->priv;
>      161
>      162         priv->rst = devm_reset_control_get_exclusive(dev, NULL);
>      163         if (IS_ERR(priv->rst)) {
>      164                 ret = PTR_ERR(priv->rst);
>      165                 dev_err_probe(dev, ret, "fail to acquire rproc reset\n");
>      166                 return ret;;
>      167         }
>      168
>      169         pm_runtime_enable(dev);
>      170         ret = pm_runtime_get_sync(dev);
> --> 171         if (ret) {
> 
> The pm_runtime_get_sync() returns both 0 and 1 on success.  The comments
> to that function suggest that this should be changed to instead use:
> 
> 	ret = pm_runtime_resume_and_get();
> 	if (ret) {
> 
> I've got no idea what that function does but it has standard error codes
> and cleanup, so I *love* it.
Indeed checking for non 0 error code is not enough.
After looking at the pm_runtime_resume_and_get error handling, I also realize
that this driver never call pm_runtime_put, so pm_runtime_get/put is unbalanced.

I plan to address both issues in a single patch.

> 
>      172                 dev_err(dev, "failed to power up\n");
>      173                 return ret;
>      174         }
>      175
>      176         dev_set_drvdata(dev, rproc);
>      177
>      178         /* Manually start the rproc */
>      179         rproc->auto_boot = false;
>      180
>      181         ret = devm_rproc_add(dev, rproc);
>      182         if (ret) {
>      183                 dev_err(dev, "rproc_add failed\n");
>      184                 goto pm_disable;
>      185         }
>      186
>      187         return 0;
>      188
>      189 pm_disable:
>      190         pm_runtime_disable(dev);
missing
pm_runtime_put(dev);
(it also applies to the remove function)
>      191
>      192         return ret;
>      193 }
> 
> regards,
> dan carpenter
> 

Will try to address both issues today.

Regards,
-- 
Julien Massot [IoT.bzh]