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

[Sultan Alsawaf <sultan@kerneltoast.com>]

On Fri, Oct 17, 2025 at 05:53:27PM +0800, Du, Bin wrote:
> On 10/17/2025 4:34 PM, Sultan Alsawaf wrote:
> > On Thu, Oct 16, 2025 at 04:13:47PM +0800, Du, Bin wrote:
> > > On 10/11/2025 5:30 PM, Du, Bin wrote:
> > > > On 10/1/2025 2:53 PM, Sultan Alsawaf wrote:
> > > > > On Thu, Sep 11, 2025 at 06:08:45PM +0800, Bin Du wrote:
> > > > > > +++ 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?
> > 
> > Oh, good catch! You are right about media_device_unregister() not cleaning up
> > media_create_pad_link().
> > 
> > But I don't see how vb2_video_unregister_device() ends up calling
> > media_entity_remove_links().
> > 
> > It looks like media_create_pad_link() is actually cleaned up via
> > v4l2_device_unregister_subdev()->media_device_unregister_entity()->__media_device_unregister_entity()->__media_entity_remove_links()
> > 
> > And I mentioned before to add a missing call to v4l2_device_unregister_subdev()
> > on error, so it looks like that will cover the media_create_pad_link() cleanup
> > and therefore you don't need to call media_device_unregister() in this scenario.
> > 
> > Does that look correct?
> > 
> 
> Yes, Sultan, we moved v4l2_device_unregister_subdev to isp4sd_deinit as you
> suggested, so current isp4sd_deinit() looks like this
> void isp4sd_deinit(struct isp4_subdev *isp_subdev)
> {
> 	struct isp4_interface *ispif = &isp_subdev->ispif;
> 
> 	isp4vid_dev_deinit(&isp_subdev->isp_vdev);
> 	v4l2_device_unregister_subdev(&isp_subdev->sdev);
> 	media_entity_cleanup(&isp_subdev->sdev.entity);
> 	isp4if_deinit(ispif);
> 	isp4sd_module_enable(isp_subdev, false);
> 
> 	ispif->status = ISP4IF_STATUS_PWR_OFF;
> }

Right, and v4l2_device_unregister_subdev() is *also* needed inside isp4sd_init()
to handle cleanup on error when isp4sd_init() doesn't complete successfully.
That's what I meant in our last few emails, since isp4sd_deinit() won't be
called in that scenario. Sorry that wasn't clear.

> 
> You are correct and I believe both isp4vid_dev_deinit and
> v4l2_device_unregister_subdev can cause media_create_pad_link() being
> cleaned up. Because isp4vid_dev_deinit is called first, so the link will be
> cleaned by it, here is the call stack FYI, does it make sense?
> [    5.198328] Call Trace:
> [    5.198329]  <TASK>
> [    5.198331]  dump_stack_lvl+0x76/0xa0
> [    5.198336]  dump_stack+0x10/0x20
> [    5.198338]  __media_entity_remove_link+0xdf/0x1f0 [mc]
> [    5.198342]  __media_entity_remove_links+0x31/0x70 [mc]
> [    5.198344]  __media_device_unregister_entity+0x93/0xf0 [mc]
> [    5.198346]  media_device_unregister_entity+0x2f/0x50 [mc]
> [    5.198348]  v4l2_device_release+0x112/0x190 [videodev]
> [    5.198355]  device_release+0x38/0xa0
> [    5.198358]  kobject_put+0x9e/0x200
> [    5.198359]  put_device+0x13/0x30
> [    5.198359]  vb2_video_unregister_device+0x8e/0xd0 [videobuf2_v4l2]
> [    5.198362]  isp4vid_dev_deinit+0xe/0x20 [amd_capture]
> [    5.198364]  isp4sd_deinit+0x25/0x80 [amd_capture]
> [    5.198366]  isp4_capture_probe+0x1ec/0x2f0 [amd_capture]
> [    5.198368]  platform_probe+0x3f/0xb0
> [    5.198370]  really_probe+0xf4/0x3b0

Thanks for clarifying! I was wondering where vdev.entity gets cleaned up, and it
looks like that is where it happens. And the subdev entity, sdev.entity, gets
cleaned up by v4l2_device_unregister_subdev().

To summarize: Assuming all of my other advice is implemented, the cleanup for
media_create_pad_link() will already be handled on error and device removal. So
the only action you should take is to disregard this advice from me:

 "2. When media_device_register() fails, clean up media_create_pad_link() by
     calling media_device_unregister()."

And that is all. :)

Sultan