linux-kernel - Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers handling added

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <51c24e3d-be89-44c9-8247-95fb776aed78@amd.com>
Date: Thu, 16 Oct 2025 16:13:47 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
 laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org,
 sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 mario.limonciello@....com, richard.gong@....com, anson.tsao@....com,
 Svetoslav Stoilov <Svetoslav.Stoilov@....com>,
 Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers
 handling added

Hi Sultan,

On 10/11/2025 5:30 PM, Du, Bin wrote:
> Many thanks Sultan for your review, that's really helpful.
> 
> On 10/1/2025 2:53 PM, Sultan Alsawaf wrote:
>> Hi Bin,
>>
>> On Thu, Sep 11, 2025 at 06:08:45PM +0800, Bin Du wrote:
>>> Isp video implements v4l2 video interface and supports NV12 and YUYV. It
>>> manages buffers, pipeline power and state. Cherry-picked Sultan's DMA
>>> buffer related fix from branch v6.16-drm-tip-isp4-for-amd on
>>> https://github.com/kerneltoast/kernel_x86_laptop.git
>>>
>>> Co-developed-by: Sultan Alsawaf <sultan@...neltoast.com>
>>> Signed-off-by: Sultan Alsawaf <sultan@...neltoast.com>
>>> Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@....com>
>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@....com>
>>> Signed-off-by: Bin Du <Bin.Du@....com>
>>> Tested-by: Alexey Zagorodnikov <xglooom@...il.com>
>>
>> [snip]
>>
>>> +++ b/drivers/media/platform/amd/isp4/isp4.c
>>> @@ -178,6 +178,16 @@ static int isp4_capture_probe(struct 
>>> platform_device *pdev)
>>>           goto err_isp4_deinit;
>>>       }
>>> +    ret = media_create_pad_link(&isp_dev->isp_sdev.sdev.entity,
>>> +                    0, &isp_dev->isp_sdev.isp_vdev.vdev.entity,
>>> +                    0,
>>> +                    MEDIA_LNK_FL_ENABLED |
>>> +                    MEDIA_LNK_FL_IMMUTABLE);
>>> +    if (ret) {
>>> +        dev_err(dev, "fail to create pad link %d\n", ret);
>>> +        goto err_isp4_deinit;
>>> +    }
>>> +
>>
>> Two problems with this hunk:
>>
>> 1. According to the comment in include/media/media-device.h [1],
>>     media_create_pad_link() should be called before 
>> media_device_register():
>>
>>      * So drivers need to first initialize the media device, register 
>> any entity
>>      * within the media device, create pad to pad links and then 
>> finally register
>>      * the media device by calling media_device_register() as a final 
>> step.
>>
>> 2. Missing call to media_device_unregister() on error when
>>     media_create_pad_link() fails.
>>
>> Since the media_create_pad_link() will be moved before 
>> media_device_register(),
>> we will need to clean up media_create_pad_link() when 
>> media_device_register()
>> fails.
>>
>> The clean-up function for media_create_pad_link() is 
>> media_device_unregister().
>> According to the comment for media_device_unregister() [2], it is safe 
>> to call
>> media_device_unregister() on an unregistered media device that is 
>> initialized
>> (through media_device_init()).
>>
>> In addition, this made me realize that there's no call to 
>> media_device_cleanup()
>> in the entire driver too. This is the cleanup function for 
>> media_device_init(),
>> so it should be called on error and on module unload.
>>
>> To summarize, make the following changes:
>>
>> 1. Move the media_create_pad_link() up, right before 
>> media_device_register().
>>
>> 2. When media_device_register() fails, clean up 
>> media_create_pad_link() by
>>     calling media_device_unregister().
>>
>> 3. Add a missing call to media_device_cleanup() on error and module 
>> unload to
>>     clean up media_device_init().
>>
> 
> Very clear guide, will follow your advice.
> 
>>>       platform_set_drvdata(pdev, isp_dev);
>>>       return 0;

For 2, we found when media_device_register() fails, calling 
media_device_unregister() won't clean up media_create_pad_link() because 
it simply returns without doing anything(see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/mc/mc-device.c?h=v6.17-rc7#n797). 
Therefore like other kernel drivers, we'd rather not call 
media_device_unregister() in this scenario, it doesn't cause issues, but 
it's not logically correct. Cleanup for media_create_pad_link() occurs 
during error handling via 
isp4sd_deinit()->isp4vid_dev_deinit()->vb2_video_unregister_device()->...->media_entity_remove_link(). 
What do you think?

[snip]

-- 
Regards,
Bin