* [bug report] media: amphion: add vpu core driver
@ 2022-03-09 10:43 Dan Carpenter
2022-03-09 10:45 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dan Carpenter @ 2022-03-09 10:43 UTC (permalink / raw)
To: ming.qian; +Cc: linux-media
Hello Ming Qian,
The patch 9f599f351e86: "media: amphion: add vpu core driver" from
Feb 24, 2022, leads to the following Smatch static checker warning:
drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
warn: pm_runtime_get_sync() also returns 1 on success
drivers/media/platform/amphion/vpu_core.c
577 static int vpu_core_probe(struct platform_device *pdev)
578 {
579 struct device *dev = &pdev->dev;
580 struct vpu_core *core;
581 struct vpu_dev *vpu = dev_get_drvdata(dev->parent);
582 struct vpu_shared_addr *iface;
583 u32 iface_data_size;
584 int ret;
585
586 dev_dbg(dev, "probe\n");
587 if (!vpu)
588 return -EINVAL;
589 core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
590 if (!core)
591 return -ENOMEM;
592
593 core->pdev = pdev;
594 core->dev = dev;
595 platform_set_drvdata(pdev, core);
596 core->vpu = vpu;
597 INIT_LIST_HEAD(&core->instances);
598 mutex_init(&core->lock);
599 mutex_init(&core->cmd_lock);
600 init_completion(&core->cmp);
601 init_waitqueue_head(&core->ack_wq);
602 core->state = VPU_CORE_DEINIT;
603
604 core->res = of_device_get_match_data(dev);
605 if (!core->res)
606 return -ENODEV;
607
608 core->type = core->res->type;
609 core->id = of_alias_get_id(dev->of_node, "vpu_core");
610 if (core->id < 0) {
611 dev_err(dev, "can't get vpu core id\n");
612 return core->id;
613 }
614 dev_info(core->dev, "[%d] = %s\n", core->id, vpu_core_type_desc(core->type));
615 ret = vpu_core_parse_dt(core, dev->of_node);
616 if (ret)
617 return ret;
618
619 core->base = devm_platform_ioremap_resource(pdev, 0);
620 if (IS_ERR(core->base))
621 return PTR_ERR(core->base);
622
623 if (!vpu_iface_check_codec(core)) {
624 dev_err(core->dev, "is not supported\n");
625 return -EINVAL;
626 }
627
628 ret = vpu_mbox_init(core);
629 if (ret)
630 return ret;
631
632 iface = devm_kzalloc(dev, sizeof(*iface), GFP_KERNEL);
633 if (!iface)
634 return -ENOMEM;
635
636 iface_data_size = vpu_iface_get_data_size(core);
637 if (iface_data_size) {
638 iface->priv = devm_kzalloc(dev, iface_data_size, GFP_KERNEL);
639 if (!iface->priv)
640 return -ENOMEM;
641 }
642
643 ret = vpu_iface_init(core, iface, &core->rpc, core->fw.phys);
644 if (ret) {
645 dev_err(core->dev, "init iface fail, ret = %d\n", ret);
646 return ret;
647 }
648
649 vpu_iface_config_system(core, vpu->res->mreg_base, vpu->base);
650 vpu_iface_set_log_buf(core, &core->log);
651
652 pm_runtime_enable(dev);
653 ret = pm_runtime_get_sync(dev);
--> 654 if (ret) {
^^^
This isn't right.
655 pm_runtime_put_noidle(dev);
656 pm_runtime_set_suspended(dev);
657 goto err_runtime_disable;
658 }
The documentation for pm_runtime_get_sync() suggests using
pm_runtime_resume_and_get() instead. I think you can just do
ret = pm_runtime_resume_and_get(dev);
if (ret)
goto err_runtime_disable;
659
660 ret = vpu_core_register(dev->parent, core);
661 if (ret)
662 goto err_core_register;
663 core->parent = dev->parent;
664
665 pm_runtime_put_sync(dev);
666 vpu_core_create_dbgfs_file(core);
667
668 return 0;
669
670 err_core_register:
671 pm_runtime_put_sync(dev);
672 err_runtime_disable:
673 pm_runtime_disable(dev);
674
675 return ret;
676 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] media: amphion: add vpu core driver
2022-03-09 10:43 [bug report] media: amphion: add vpu core driver Dan Carpenter
@ 2022-03-09 10:45 ` Dan Carpenter
2022-03-09 13:33 ` Hans Verkuil
2022-03-10 1:54 ` [EXT] " Ming Qian
2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2022-03-09 10:45 UTC (permalink / raw)
To: ming.qian; +Cc: linux-media
Same thing in vpu_probe() as well.
drivers/media/platform/amphion/vpu_drv.c:122 vpu_probe() warn: pm_runtime_get_sync() also returns 1 on success
regards,
dan carpenter
On Wed, Mar 09, 2022 at 01:43:37PM +0300, Dan Carpenter wrote:
> Hello Ming Qian,
>
> The patch 9f599f351e86: "media: amphion: add vpu core driver" from
> Feb 24, 2022, leads to the following Smatch static checker warning:
>
> drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
> warn: pm_runtime_get_sync() also returns 1 on success
>
> drivers/media/platform/amphion/vpu_core.c
> 577 static int vpu_core_probe(struct platform_device *pdev)
> 578 {
> 579 struct device *dev = &pdev->dev;
> 580 struct vpu_core *core;
> 581 struct vpu_dev *vpu = dev_get_drvdata(dev->parent);
> 582 struct vpu_shared_addr *iface;
> 583 u32 iface_data_size;
> 584 int ret;
> 585
> 586 dev_dbg(dev, "probe\n");
> 587 if (!vpu)
> 588 return -EINVAL;
> 589 core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> 590 if (!core)
> 591 return -ENOMEM;
> 592
> 593 core->pdev = pdev;
> 594 core->dev = dev;
> 595 platform_set_drvdata(pdev, core);
> 596 core->vpu = vpu;
> 597 INIT_LIST_HEAD(&core->instances);
> 598 mutex_init(&core->lock);
> 599 mutex_init(&core->cmd_lock);
> 600 init_completion(&core->cmp);
> 601 init_waitqueue_head(&core->ack_wq);
> 602 core->state = VPU_CORE_DEINIT;
> 603
> 604 core->res = of_device_get_match_data(dev);
> 605 if (!core->res)
> 606 return -ENODEV;
> 607
> 608 core->type = core->res->type;
> 609 core->id = of_alias_get_id(dev->of_node, "vpu_core");
> 610 if (core->id < 0) {
> 611 dev_err(dev, "can't get vpu core id\n");
> 612 return core->id;
> 613 }
> 614 dev_info(core->dev, "[%d] = %s\n", core->id, vpu_core_type_desc(core->type));
> 615 ret = vpu_core_parse_dt(core, dev->of_node);
> 616 if (ret)
> 617 return ret;
> 618
> 619 core->base = devm_platform_ioremap_resource(pdev, 0);
> 620 if (IS_ERR(core->base))
> 621 return PTR_ERR(core->base);
> 622
> 623 if (!vpu_iface_check_codec(core)) {
> 624 dev_err(core->dev, "is not supported\n");
> 625 return -EINVAL;
> 626 }
> 627
> 628 ret = vpu_mbox_init(core);
> 629 if (ret)
> 630 return ret;
> 631
> 632 iface = devm_kzalloc(dev, sizeof(*iface), GFP_KERNEL);
> 633 if (!iface)
> 634 return -ENOMEM;
> 635
> 636 iface_data_size = vpu_iface_get_data_size(core);
> 637 if (iface_data_size) {
> 638 iface->priv = devm_kzalloc(dev, iface_data_size, GFP_KERNEL);
> 639 if (!iface->priv)
> 640 return -ENOMEM;
> 641 }
> 642
> 643 ret = vpu_iface_init(core, iface, &core->rpc, core->fw.phys);
> 644 if (ret) {
> 645 dev_err(core->dev, "init iface fail, ret = %d\n", ret);
> 646 return ret;
> 647 }
> 648
> 649 vpu_iface_config_system(core, vpu->res->mreg_base, vpu->base);
> 650 vpu_iface_set_log_buf(core, &core->log);
> 651
> 652 pm_runtime_enable(dev);
> 653 ret = pm_runtime_get_sync(dev);
> --> 654 if (ret) {
> ^^^
> This isn't right.
>
> 655 pm_runtime_put_noidle(dev);
> 656 pm_runtime_set_suspended(dev);
> 657 goto err_runtime_disable;
> 658 }
>
> The documentation for pm_runtime_get_sync() suggests using
> pm_runtime_resume_and_get() instead. I think you can just do
>
> ret = pm_runtime_resume_and_get(dev);
> if (ret)
> goto err_runtime_disable;
>
> 659
> 660 ret = vpu_core_register(dev->parent, core);
> 661 if (ret)
> 662 goto err_core_register;
> 663 core->parent = dev->parent;
> 664
> 665 pm_runtime_put_sync(dev);
> 666 vpu_core_create_dbgfs_file(core);
> 667
> 668 return 0;
> 669
> 670 err_core_register:
> 671 pm_runtime_put_sync(dev);
> 672 err_runtime_disable:
> 673 pm_runtime_disable(dev);
> 674
> 675 return ret;
> 676 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] media: amphion: add vpu core driver
2022-03-09 10:43 [bug report] media: amphion: add vpu core driver Dan Carpenter
2022-03-09 10:45 ` Dan Carpenter
@ 2022-03-09 13:33 ` Hans Verkuil
2022-03-09 15:20 ` Dan Carpenter
2022-03-10 1:54 ` [EXT] " Ming Qian
2 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2022-03-09 13:33 UTC (permalink / raw)
To: Dan Carpenter, ming.qian; +Cc: linux-media
Hi Dan,
On 3/9/22 11:43, Dan Carpenter wrote:
> Hello Ming Qian,
>
> The patch 9f599f351e86: "media: amphion: add vpu core driver" from
> Feb 24, 2022, leads to the following Smatch static checker warning:
>
> drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
> warn: pm_runtime_get_sync() also returns 1 on success
Odd, I didn't get this warning when I ran smatch.
I'm running smatch from the master branch of git://repo.or.cz/smatch.git.
To be honest, I don't see the string 'also returns' at all in the git repo.
I'd really like to see this warning since it's so easy to get pm_runtime_get_sync()
wrong, and to miss it as a reviewer as well.
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] media: amphion: add vpu core driver
2022-03-09 13:33 ` Hans Verkuil
@ 2022-03-09 15:20 ` Dan Carpenter
2022-03-09 15:43 ` Hans Verkuil
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-03-09 15:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: ming.qian, linux-media
On Wed, Mar 09, 2022 at 02:33:20PM +0100, Hans Verkuil wrote:
> Hi Dan,
>
> On 3/9/22 11:43, Dan Carpenter wrote:
> > Hello Ming Qian,
> >
> > The patch 9f599f351e86: "media: amphion: add vpu core driver" from
> > Feb 24, 2022, leads to the following Smatch static checker warning:
> >
> > drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
> > warn: pm_runtime_get_sync() also returns 1 on success
>
> Odd, I didn't get this warning when I ran smatch.
Sorry, I will push this Smatch check tomorrow. It's simple and has few
false positives.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] media: amphion: add vpu core driver
2022-03-09 15:20 ` Dan Carpenter
@ 2022-03-09 15:43 ` Hans Verkuil
0 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2022-03-09 15:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: ming.qian, linux-media
On 3/9/22 16:20, Dan Carpenter wrote:
> On Wed, Mar 09, 2022 at 02:33:20PM +0100, Hans Verkuil wrote:
>> Hi Dan,
>>
>> On 3/9/22 11:43, Dan Carpenter wrote:
>>> Hello Ming Qian,
>>>
>>> The patch 9f599f351e86: "media: amphion: add vpu core driver" from
>>> Feb 24, 2022, leads to the following Smatch static checker warning:
>>>
>>> drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
>>> warn: pm_runtime_get_sync() also returns 1 on success
>>
>> Odd, I didn't get this warning when I ran smatch.
>
> Sorry, I will push this Smatch check tomorrow. It's simple and has few
> false positives.
Ah! I suddenly feel a lot less guilty for having missed this issue in the
amphion driver :-)
Looking forward to this new check!
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] [bug report] media: amphion: add vpu core driver
2022-03-09 10:43 [bug report] media: amphion: add vpu core driver Dan Carpenter
2022-03-09 10:45 ` Dan Carpenter
2022-03-09 13:33 ` Hans Verkuil
@ 2022-03-10 1:54 ` Ming Qian
2 siblings, 0 replies; 6+ messages in thread
From: Ming Qian @ 2022-03-10 1:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media@vger.kernel.org
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, March 9, 2022 6:44 PM
> To: Ming Qian <ming.qian@nxp.com>
> Cc: linux-media@vger.kernel.org
> Subject: [EXT] [bug report] media: amphion: add vpu core driver
>
> Caution: EXT Email
>
> Hello Ming Qian,
>
> The patch 9f599f351e86: "media: amphion: add vpu core driver" from Feb 24,
> 2022, leads to the following Smatch static checker warning:
>
> drivers/media/platform/amphion/vpu_core.c:654 vpu_core_probe()
> warn: pm_runtime_get_sync() also returns 1 on success
>
> drivers/media/platform/amphion/vpu_core.c
> 577 static int vpu_core_probe(struct platform_device *pdev)
> 578 {
> 579 struct device *dev = &pdev->dev;
> 580 struct vpu_core *core;
> 581 struct vpu_dev *vpu = dev_get_drvdata(dev->parent);
> 582 struct vpu_shared_addr *iface;
> 583 u32 iface_data_size;
> 584 int ret;
> 585
> 586 dev_dbg(dev, "probe\n");
> 587 if (!vpu)
> 588 return -EINVAL;
> 589 core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> 590 if (!core)
> 591 return -ENOMEM;
> 592
> 593 core->pdev = pdev;
> 594 core->dev = dev;
> 595 platform_set_drvdata(pdev, core);
> 596 core->vpu = vpu;
> 597 INIT_LIST_HEAD(&core->instances);
> 598 mutex_init(&core->lock);
> 599 mutex_init(&core->cmd_lock);
> 600 init_completion(&core->cmp);
> 601 init_waitqueue_head(&core->ack_wq);
> 602 core->state = VPU_CORE_DEINIT;
> 603
> 604 core->res = of_device_get_match_data(dev);
> 605 if (!core->res)
> 606 return -ENODEV;
> 607
> 608 core->type = core->res->type;
> 609 core->id = of_alias_get_id(dev->of_node, "vpu_core");
> 610 if (core->id < 0) {
> 611 dev_err(dev, "can't get vpu core id\n");
> 612 return core->id;
> 613 }
> 614 dev_info(core->dev, "[%d] = %s\n", core->id,
> vpu_core_type_desc(core->type));
> 615 ret = vpu_core_parse_dt(core, dev->of_node);
> 616 if (ret)
> 617 return ret;
> 618
> 619 core->base = devm_platform_ioremap_resource(pdev, 0);
> 620 if (IS_ERR(core->base))
> 621 return PTR_ERR(core->base);
> 622
> 623 if (!vpu_iface_check_codec(core)) {
> 624 dev_err(core->dev, "is not supported\n");
> 625 return -EINVAL;
> 626 }
> 627
> 628 ret = vpu_mbox_init(core);
> 629 if (ret)
> 630 return ret;
> 631
> 632 iface = devm_kzalloc(dev, sizeof(*iface), GFP_KERNEL);
> 633 if (!iface)
> 634 return -ENOMEM;
> 635
> 636 iface_data_size = vpu_iface_get_data_size(core);
> 637 if (iface_data_size) {
> 638 iface->priv = devm_kzalloc(dev, iface_data_size,
> GFP_KERNEL);
> 639 if (!iface->priv)
> 640 return -ENOMEM;
> 641 }
> 642
> 643 ret = vpu_iface_init(core, iface, &core->rpc, core->fw.phys);
> 644 if (ret) {
> 645 dev_err(core->dev, "init iface fail, ret = %d\n", ret);
> 646 return ret;
> 647 }
> 648
> 649 vpu_iface_config_system(core, vpu->res->mreg_base,
> vpu->base);
> 650 vpu_iface_set_log_buf(core, &core->log);
> 651
> 652 pm_runtime_enable(dev);
> 653 ret = pm_runtime_get_sync(dev);
> --> 654 if (ret) {
> ^^^
> This isn't right.
>
> 655 pm_runtime_put_noidle(dev);
> 656 pm_runtime_set_suspended(dev);
> 657 goto err_runtime_disable;
> 658 }
>
> The documentation for pm_runtime_get_sync() suggests using
> pm_runtime_resume_and_get() instead. I think you can just do
>
> ret = pm_runtime_resume_and_get(dev);
> if (ret)
> goto err_runtime_disable;
>
Got it, I'll fix it
> 659
> 660 ret = vpu_core_register(dev->parent, core);
> 661 if (ret)
> 662 goto err_core_register;
> 663 core->parent = dev->parent;
> 664
> 665 pm_runtime_put_sync(dev);
> 666 vpu_core_create_dbgfs_file(core);
> 667
> 668 return 0;
> 669
> 670 err_core_register:
> 671 pm_runtime_put_sync(dev);
> 672 err_runtime_disable:
> 673 pm_runtime_disable(dev);
> 674
> 675 return ret;
> 676 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-10 1:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-09 10:43 [bug report] media: amphion: add vpu core driver Dan Carpenter
2022-03-09 10:45 ` Dan Carpenter
2022-03-09 13:33 ` Hans Verkuil
2022-03-09 15:20 ` Dan Carpenter
2022-03-09 15:43 ` Hans Verkuil
2022-03-10 1:54 ` [EXT] " Ming Qian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.