[bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver

[Dan Carpenter]

Hello Moudy Ho,

The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3
driver" from Aug 23, 2022, leads to the following Smatch static
checker warning:

	drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c:292 mdp_probe()
	error: we previously assumed 'mdp' could be null (see line 188)

My blog entry gives a good overview of how to write Free the Last Thing
Style error handling.  Let's take a look at it as it applies to
mdp_probe().

drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
    180 static int mdp_probe(struct platform_device *pdev)
    181 {
    182         struct device *dev = &pdev->dev;
    183         struct mdp_dev *mdp;
    184         struct platform_device *mm_pdev;
    185         int ret, i;
    186 
    187         mdp = kzalloc(sizeof(*mdp), GFP_KERNEL);
    188         if (!mdp) {
    189                 ret = -ENOMEM;
    190                 goto err_return;

There is no Last Thing to free.  This should be "return -ENOMEM;".
This goto will crash.  The Last thing is now "mdp".

    191         }
    192 
    193         mdp->pdev = pdev;
    194         mdp->mdp_data = of_device_get_match_data(&pdev->dev);
    195 
    196         mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MMSYS);
    197         if (!mm_pdev) {
    198                 ret = -ENODEV;
    199                 goto err_return;

This should be "goto err_free_mdp;".  This goto will trigger a series
of WARN_ON() stack traces.  The Last Thing is now mdp->mdp_mmsys.

    200         }
    201         mdp->mdp_mmsys = &mm_pdev->dev;
    202 
    203         mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MUTEX);
    204         if (WARN_ON(!mm_pdev)) {
    205                 ret = -ENODEV;
    206                 goto err_return;

This goto should call put on mdp->mdp_mmsys dev_something.  It instead
leaks that.  But it does call mtk_mutex_put() and each call will leads
to a series of stack traces.  The Last Thing is now mm_pdev.  That
doesn't seem to be stored anywhere so it's going to be complicated to
free it...  :/

    207         }
    208         for (i = 0; i < MDP_PIPE_MAX; i++) {
    209                 mdp->mdp_mutex[i] = mtk_mutex_get(&mm_pdev->dev);
    210                 if (!mdp->mdp_mutex[i]) {
    211                         ret = -ENODEV;
    212                         goto err_return;

If this fails it should unwind the successful allocations:

	while (--i >= 0) {

But instead it does unwinds everything.  Stack traces.

    213                 }
    214         }
    215 
    216         ret = mdp_comp_config(mdp);
    217         if (ret) {
    218                 dev_err(dev, "Failed to config mdp components\n");
    219                 goto err_return;

Alright.  Good.  This will leak the "mm_pdev" references but it will not
print any stack traces.  The mdp_comp_config() function uses One Magic
Free Function style error handling so it is buggy.  The Last thing is
now comp_config.

    220         }
    221 
    222         mdp->job_wq = alloc_workqueue(MDP_MODULE_NAME, WQ_FREEZABLE, 0);
    223         if (!mdp->job_wq) {
    224                 dev_err(dev, "Unable to create job workqueue\n");
    225                 ret = -ENOMEM;
    226                 goto err_deinit_comp;

Hooray!  This label has a good name and frees the Last Thing.  The new
last thing is the ->job_wq.

    227         }
    228 
    229         mdp->clock_wq = alloc_workqueue(MDP_MODULE_NAME "-clock", WQ_FREEZABLE,
    230                                         0);
    231         if (!mdp->clock_wq) {
    232                 dev_err(dev, "Unable to create clock workqueue\n");
    233                 ret = -ENOMEM;
    234                 goto err_destroy_job_wq;

Hooray!

    235         }
    236 
    237         mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_SCP);
    238         if (WARN_ON(!mm_pdev)) {
    239                 dev_err(&pdev->dev, "Could not get scp device\n");
    240                 ret = -ENODEV;
    241                 goto err_destroy_clock_wq;

Hooray!

    242         }
    243         mdp->scp = platform_get_drvdata(mm_pdev);
    244         mdp->rproc_handle = scp_get_rproc(mdp->scp);
    245         dev_dbg(&pdev->dev, "MDP rproc_handle: %pK", mdp->rproc_handle);
    246 
    247         mutex_init(&mdp->vpu_lock);
    248         mutex_init(&mdp->m2m_lock);
    249 
    250         mdp->cmdq_clt = cmdq_mbox_create(dev, 0);
    251         if (IS_ERR(mdp->cmdq_clt)) {
    252                 ret = PTR_ERR(mdp->cmdq_clt);
    253                 goto err_put_scp;

Ugh...  The mm_pdev name changed to scp.  It took a while to figure that
out.  This unwinds the __get_pdev_by_id().  Fair enough.  The Last thing
is now cmdq_clt.

    254         }
    255 
    256         init_waitqueue_head(&mdp->callback_wq);
    257         ida_init(&mdp->mdp_ida);
    258         platform_set_drvdata(pdev, mdp);
    259 
    260         vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
    261 
    262         ret = v4l2_device_register(dev, &mdp->v4l2_dev);
    263         if (ret) {
    264                 dev_err(dev, "Failed to register v4l2 device\n");
    265                 ret = -EINVAL;
    266                 goto err_mbox_destroy;

The cmdq_clt is an mbox.  Fair enough.  Good name, label does what we
expected.

    267         }
    268 
    269         ret = mdp_m2m_device_register(mdp);
    270         if (ret) {
    271                 v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
    272                 goto err_unregister_device;

Good.

    273         }
    274 
    275         dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
    276         return 0;

Summary:
BUG1: NULL dereference
BUG2: Stack traces calling mtk_mutex_put().
BUG3&4: Two __get_pdev_by_id() which need a matching put (reference leaks).

    277 
    278 err_unregister_device:
    279         v4l2_device_unregister(&mdp->v4l2_dev);
    280 err_mbox_destroy:
    281         cmdq_mbox_destroy(mdp->cmdq_clt);
    282 err_put_scp:
    283         scp_put(mdp->scp);
    284 err_destroy_clock_wq:
    285         destroy_workqueue(mdp->clock_wq);
    286 err_destroy_job_wq:
    287         destroy_workqueue(mdp->job_wq);
    288 err_deinit_comp:
    289         mdp_comp_destroy(mdp);
    290 err_return:
    291         for (i = 0; i < MDP_PIPE_MAX; i++)
--> 292                 mtk_mutex_put(mdp->mdp_mutex[i]);
    293         kfree(mdp);
    294         dev_dbg(dev, "Errno %d\n", ret);
    295         return ret;
    296 }

regards,
dan carpenter