| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c1de12a1-a9ee-4d8f-957c-811b4eb33890@amlogic.com>
Date: Tue, 28 Oct 2025 19:22:32 +0800
From: Zhentao Guo <zhentao.guo@...ogic.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH 3/3] decoder: Add V4L2 stateless H.264 decoder driver
在 2025/10/27 21:10, Krzysztof Kozlowski 写道:
> [You don't often get email from krzk@...nel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> [ EXTERNAL EMAIL ]
>
> On 27/10/2025 06:42, Zhentao Guo via B4 Relay wrote:
>> From: Zhentao Guo <zhentao.guo@...ogic.com>
>>
>> This patch introduces initial driver support for Amlogic's new
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94
Got it, I'll refer to the document and revise the commit message.
>
>> video acceleration hardware architecture, designed for video
>> stream decoding. The driver is designed to support the
>> V4L2 M2M stateless decoder API. In phase 1, it supports H.264
>> bitstreams decoding.
>>
>> Signed-off-by: Zhentao Guo <zhentao.guo@...ogic.com>
> ...
>
>
>> +
>> +static int aml_vdec_drv_probe(struct platform_device *pdev)
>> +{
>> + struct aml_vdec_dev *dev;
>> + struct video_device *vfd_dec;
>> + struct aml_vdec_hw *hw;
>> + int ret = 0;
>> +
>> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> + if (!dev)
>> + return -ENOMEM;
>> +
>> + dev->plat_dev = pdev;
>> + mutex_init(&dev->dev_mutex);
>> +
>> + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>> + if (ret)
>> + return dev_err_probe(&pdev->dev, ret, "v4l2_device_register err\n");
>> +
>> + vfd_dec = video_device_alloc();
>> + if (!vfd_dec) {
>> + v4l2_err(&dev->v4l2_dev, "Failed to allocate video device\n");
>> + ret = -ENOMEM;
>> + goto err_device_alloc;
>> + }
>> + *vfd_dec = dec_dev;
>> + vfd_dec->v4l2_dev = &dev->v4l2_dev;
>> + vfd_dec->lock = &dev->dev_mutex;
>> + video_set_drvdata(vfd_dec, dev);
>> + dev->vfd = vfd_dec;
>> + platform_set_drvdata(pdev, dev);
>> +
>> + hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> Why aren't you using devm interfaces?
Thanks for the reminder, It would be better to use the devm interfaces here,
I'll fix this.
>> + if (!hw) {
>> + ret = -ENOMEM;
>> + goto err_dec_mem_init;
>> + }
>> + dev->dec_hw = hw;
>> +
>> + dev->pvdec_data = of_device_get_match_data(&pdev->dev);
>> + ret = dev->pvdec_data->req_hw_resource(dev);
>> + if (ret < 0)
>> + goto err_hw_init;
>> +
>> + dev->m2m_dev_dec = v4l2_m2m_init(&aml_vdec_m2m_ops);
>> + if (IS_ERR((__force void *)dev->m2m_dev_dec)) {
> Huh? Why the cast?
The cast seems superfluous here, I'll remove it.
>> + v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem dec device\n");
>> + ret = PTR_ERR((__force void *)dev->m2m_dev_dec);
>> + goto err_hw_init;
>> + }
>> +
>> + ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
>> + if (ret) {
>> + v4l2_err(&dev->v4l2_dev, "Failed to register video device");
>> + goto err_vid_dev_register;
>> + }
>> +
>> + v4l2_info(&dev->v4l2_dev, "Registered %s as /dev/%s\n",
>> + vfd_dec->name, video_device_node_name(vfd_dec));
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
Got it, I'll get rid of it.
>
>> +
>> + dev->mdev.dev = &pdev->dev;
>> + strscpy(dev->mdev.model, AML_VDEC_DRV_NAME, sizeof(dev->mdev.model));
>> + media_device_init(&dev->mdev);
>> + dev->mdev.ops = &aml_m2m_media_ops;
>> + dev->v4l2_dev.mdev = &dev->mdev;
>> +
>> + ret = v4l2_m2m_register_media_controller(dev->m2m_dev_dec, vfd_dec,
>> + MEDIA_ENT_F_PROC_VIDEO_DECODER);
>> + if (ret) {
>> + v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem media controller\n");
>> + goto error_m2m_mc_register;
>> + }
>> +
>> + ret = media_device_register(&dev->mdev);
>> + if (ret) {
>> + v4l2_err(&dev->v4l2_dev, "Failed to register media device");
>> + goto err_media_dev_register;
>> + }
>> + vdec_enable(dev->dec_hw);
>> + return 0;
>> +
>> +err_media_dev_register:
>> + v4l2_m2m_unregister_media_controller(dev->m2m_dev_dec);
>> +error_m2m_mc_register:
>> + media_device_cleanup(&dev->mdev);
>> +err_vid_dev_register:
>> + v4l2_m2m_release(dev->m2m_dev_dec);
>> +err_hw_init:
>> + kfree(hw);
>> + dev->dec_hw = NULL;
>> +err_dec_mem_init:
>> + video_device_release(vfd_dec);
>> +err_device_alloc:
>> + v4l2_device_unregister(&dev->v4l2_dev);
>> + return ret;
>> +}
>> +
>> +static void aml_vdec_drv_remove(struct platform_device *pdev)
>> +{
>> + struct aml_vdec_dev *dev = platform_get_drvdata(pdev);
>> +
>> + vdec_disable(dev->dec_hw);
>> +
>> + if (media_devnode_is_registered(dev->mdev.devnode)) {
>> + media_device_unregister(&dev->mdev);
>> + media_device_cleanup(&dev->mdev);
>> + }
>> +
>> + if (dev->m2m_dev_dec)
>> + v4l2_m2m_release(dev->m2m_dev_dec);
>> + if (dev->vfd)
>> + video_unregister_device(dev->vfd);
>> + if (dev->dec_hw)
>> + dev->pvdec_data->destroy_hw_resource(dev);
>> +
>> + v4l2_device_unregister(&dev->v4l2_dev);
>> +
>> + pr_debug("aml v4l2 decoder driver remove\n");
>
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
I'll remove this message.
>
>> +}
>> +
>> +static const struct of_device_id aml_vdec_match[] = {
>> + {.compatible = "amlogic,s4-vcodec-dec", .data = &aml_vdec_s4_pdata},
>> + {},
>> +};
>> +
>> +static struct platform_driver aml_vcodec_dec_driver = {
>> + .probe = aml_vdec_drv_probe,
>> + .remove = aml_vdec_drv_remove,
>> + .driver = {
>> + .name = AML_VDEC_DRV_NAME,
>> + .of_match_table = aml_vdec_match,
>> + },
>> +};
>> +
>> +static int __init aml_vdec_init(void)
>> +{
>> + pr_debug("aml v4l2 decoder module init\n");
>> +
>> + if (platform_driver_register(&aml_vcodec_dec_driver)) {
>> + pr_err("failed to register aml v4l2 decoder\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void __exit aml_vdec_exit(void)
>> +{
>> + pr_debug("aml v4l2 decoder module exit\n");
>> + platform_driver_unregister(&aml_vcodec_dec_driver);
> No, drop entire init/exit. This is some really odd code, probably
> copy-paste from poor downstream. Please look at upstream drivers - do
> you see anything like that anywhere?
Yes, it should be dropped. I reviewed some other upstream drivers, nobody
add this.
>> +}
>> +
>> +module_init(aml_vdec_init);
>> +module_exit(aml_vdec_exit);
>> +
> Best regards,
> Krzysztof
Thank you
Zhentao
Powered by blists - more mailing lists