| 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: <a2ec6794-1131-421e-848b-3fddd39191e2@kernel.org>
Date: Mon, 27 Oct 2025 14:10:18 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: zhentao.guo@...ogic.com, 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
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
> 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?
> + 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?
> + 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
> +
> + 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
> +}
> +
> +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?
> +}
> +
> +module_init(aml_vdec_init);
> +module_exit(aml_vdec_exit);
> +
Best regards,
Krzysztof
Powered by blists - more mailing lists