| Message ID | 20231220103713.113386-21-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:37:04PM +0200, Sakari Ailus wrote: > v4l2_device_unregister() unregisters V4L2 sub-device nodes among other > things. Call it before releasing memory and other resources. Please expand the commit message, it's not immediately clear why this is needed and what the consequences are. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 5d3b0ffd3d08..da82d09b46ab 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1827,11 +1827,11 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > > media_device_unregister(&cio2->media_dev); > + v4l2_device_unregister(&cio2->v4l2_dev); > v4l2_async_nf_unregister(&cio2->notifier); > v4l2_async_nf_cleanup(&cio2->notifier); > cio2_queues_exit(cio2); > cio2_fbpt_exit_dummy(cio2); > - v4l2_device_unregister(&cio2->v4l2_dev); > media_device_cleanup(&cio2->media_dev); > mutex_destroy(&cio2->lock); >
Hi Laurent, On Wed, Feb 07, 2024 at 04:24:35PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:37:04PM +0200, Sakari Ailus wrote: > > v4l2_device_unregister() unregisters V4L2 sub-device nodes among other > > things. Call it before releasing memory and other resources. > > Please expand the commit message, it's not immediately clear why this is > needed and what the consequences are. Thanks for the review. There's actually a change in APIs here as the async notifier expects the V4L2 device to remain in place so the notifier can't be cleaned up before unregistering the V4L2 device. Further patches in the set move cleaning up the notifier to the release callback but I think it can well be done here, too. So I think this patch can be dropped.
On Tue, Mar 05, 2024 at 10:21:54AM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Wed, Feb 07, 2024 at 04:24:35PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Wed, Dec 20, 2023 at 12:37:04PM +0200, Sakari Ailus wrote: > > > v4l2_device_unregister() unregisters V4L2 sub-device nodes among other > > > things. Call it before releasing memory and other resources. > > > > Please expand the commit message, it's not immediately clear why this is > > needed and what the consequences are. > > Thanks for the review. > > There's actually a change in APIs here as the async notifier expects the > V4L2 device to remain in place so the notifier can't be cleaned up before > unregistering the V4L2 device. Further patches in the set move cleaning up > the notifier to the release callback but I think it can well be done here, > too. So I think this patch can be dropped. That went a bit too fast. The call should be moved after the notifier cleanup.
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 5d3b0ffd3d08..da82d09b46ab 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1827,11 +1827,11 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) struct cio2_device *cio2 = pci_get_drvdata(pci_dev); media_device_unregister(&cio2->media_dev); + v4l2_device_unregister(&cio2->v4l2_dev); v4l2_async_nf_unregister(&cio2->notifier); v4l2_async_nf_cleanup(&cio2->notifier); cio2_queues_exit(cio2); cio2_fbpt_exit_dummy(cio2); - v4l2_device_unregister(&cio2->v4l2_dev); media_device_cleanup(&cio2->media_dev); mutex_destroy(&cio2->lock);