Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources
On 10/15/2018 04:39 PM, Sakari Ailus wrote: > Hi Bingbu, > > On Mon, Oct 15, 2018 at 03:15:05PM +0800, Bing Bu Cao wrote: >> On 10/10/2018 04:32 PM, Sakari Ailus wrote: >>> While there are issues related to object lifetime management, unregister >>> the media device first, followed immediately by other device nodes when >>> the driver is being unbound. Only then the resources needed by the driver >>> may be released. This is slightly safer. >>> >>> Signed-off-by: Sakari Ailus >>> --- >>> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >>> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >>> index 452eb9b42140..723022ef3662 100644 >>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c >>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c >>> @@ -1846,12 +1846,12 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) >>> struct cio2_device *cio2 = pci_get_drvdata(pci_dev); >>> unsigned int i; >>> >>> + media_device_unregister(>media_dev); >>> cio2_notifier_exit(cio2); >>> - cio2_fbpt_exit_dummy(cio2); >>> for (i = 0; i < CIO2_QUEUES; i++) >>> cio2_queue_exit(cio2, >queue[i]); >>> + cio2_fbpt_exit_dummy(cio2); >> Hi, Sakari, >> The fbpt dummy pages cleanup does not matter much before/after queues >> exit, right? > cio2_queue_exit() will unregister the video device and the video buffer > queue. Up to this point it's possible to open the video device and start > streaming on it. While this patch does not fully address the issue it makes > it a slightly lesser issue. Okay, thanks for your explanation. >>> v4l2_device_unregister(>v4l2_dev); >>> - media_device_unregister(>media_dev); >>> media_device_cleanup(>media_dev); >>> mutex_destroy(>lock); >>> }
Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources
Hi Bingbu, On Mon, Oct 15, 2018 at 03:15:05PM +0800, Bing Bu Cao wrote: > > On 10/10/2018 04:32 PM, Sakari Ailus wrote: > > While there are issues related to object lifetime management, unregister > > the media device first, followed immediately by other device nodes when > > the driver is being unbound. Only then the resources needed by the driver > > may be released. This is slightly safer. > > > > Signed-off-by: Sakari Ailus > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 452eb9b42140..723022ef3662 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -1846,12 +1846,12 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) > > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > > unsigned int i; > > > > + media_device_unregister(>media_dev); > > cio2_notifier_exit(cio2); > > - cio2_fbpt_exit_dummy(cio2); > > for (i = 0; i < CIO2_QUEUES; i++) > > cio2_queue_exit(cio2, >queue[i]); > > + cio2_fbpt_exit_dummy(cio2); > Hi, Sakari, > The fbpt dummy pages cleanup does not matter much before/after queues > exit, right? cio2_queue_exit() will unregister the video device and the video buffer queue. Up to this point it's possible to open the video device and start streaming on it. While this patch does not fully address the issue it makes it a slightly lesser issue. > > v4l2_device_unregister(>v4l2_dev); > > - media_device_unregister(>media_dev); > > media_device_cleanup(>media_dev); > > mutex_destroy(>lock); > > } > -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources
On 10/10/2018 04:32 PM, Sakari Ailus wrote: > While there are issues related to object lifetime management, unregister > the media device first, followed immediately by other device nodes when > the driver is being unbound. Only then the resources needed by the driver > may be released. This is slightly safer. > > Signed-off-by: Sakari Ailus > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 452eb9b42140..723022ef3662 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1846,12 +1846,12 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > unsigned int i; > > + media_device_unregister(>media_dev); > cio2_notifier_exit(cio2); > - cio2_fbpt_exit_dummy(cio2); > for (i = 0; i < CIO2_QUEUES; i++) > cio2_queue_exit(cio2, >queue[i]); > + cio2_fbpt_exit_dummy(cio2); Hi, Sakari, The fbpt dummy pages cleanup does not matter much before/after queues exit, right? > v4l2_device_unregister(>v4l2_dev); > - media_device_unregister(>media_dev); > media_device_cleanup(>media_dev); > mutex_destroy(>lock); > }
Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources
Tested-by: Bingbu Cao Reviewed-by: Bingbu Cao On 10/10/2018 04:32 PM, Sakari Ailus wrote: > While there are issues related to object lifetime management, unregister > the media device first, followed immediately by other device nodes when > the driver is being unbound. Only then the resources needed by the driver > may be released. This is slightly safer. > > Signed-off-by: Sakari Ailus > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 452eb9b42140..723022ef3662 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1846,12 +1846,12 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > unsigned int i; > > + media_device_unregister(>media_dev); > cio2_notifier_exit(cio2); > - cio2_fbpt_exit_dummy(cio2); > for (i = 0; i < CIO2_QUEUES; i++) > cio2_queue_exit(cio2, >queue[i]); > + cio2_fbpt_exit_dummy(cio2); > v4l2_device_unregister(>v4l2_dev); > - media_device_unregister(>media_dev); > media_device_cleanup(>media_dev); > mutex_destroy(>lock); > }
[PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources
While there are issues related to object lifetime management, unregister the media device first, followed immediately by other device nodes when the driver is being unbound. Only then the resources needed by the driver may be released. This is slightly safer. Signed-off-by: Sakari Ailus --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 452eb9b42140..723022ef3662 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1846,12 +1846,12 @@ static void cio2_pci_remove(struct pci_dev *pci_dev) struct cio2_device *cio2 = pci_get_drvdata(pci_dev); unsigned int i; + media_device_unregister(>media_dev); cio2_notifier_exit(cio2); - cio2_fbpt_exit_dummy(cio2); for (i = 0; i < CIO2_QUEUES; i++) cio2_queue_exit(cio2, >queue[i]); + cio2_fbpt_exit_dummy(cio2); v4l2_device_unregister(>v4l2_dev); - media_device_unregister(>media_dev); media_device_cleanup(>media_dev); mutex_destroy(>lock); } -- 2.11.0
