Trivial CIO2 patches

PATCH In review Sakari Ailus 2 participants 9 messages Oct 10, 2018 – Oct 15, 2018

Subsystems (2)

Media (V4L/DVB) Intel IPU3 CSI-2 CIO2 driver

lore

Share & tools

lore

Sakari Ailus

author maintainer

2018-10-10 15:53 UTC #0 lore raw

cover Trivial CIO2 patches

Hi,

Here are two simple cio2 patches. Slightly safer module removal plus a cleanup.

Sakari Ailus (2):
ipu3-cio2: Unregister device nodes first, then release resources
ipu3-cio2: Use cio2_queues_exit

 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2018-10-10 15:53 UTC #1 lore raw

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 <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +2 -2

index 452eb9b42140..723022ef3662 100644
@@ -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(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
 	for (i = 0; i < CIO2_QUEUES; i++)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_fbpt_exit_dummy(cio2);
 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 }

Signature · 1 line

2.11.0

Bing Bu Cao

Tested-by Bingbu Cao Reviewed-by Bingbu Cao on v1 1/2

2018-10-11 16:38 UTC #2 lore raw

Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources

Tested-by Bingbu Cao <[email protected]>

Reviewed-by Bingbu Cao <[email protected]>

Quoted 29 lines · Sakari Ailus

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 <[email protected]>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/pci/intel/ipu3/ipu3-cio2.c +2 -2
index 452eb9b42140..723022ef3662 100644
@@ -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(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
 	for (i = 0; i < CIO2_QUEUES; i++)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_fbpt_exit_dummy(cio2);
 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 }

Bing Bu Cao

2018-10-15 14:54 UTC #3 lore raw

Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources

In reply to Sakari Ailus

Quoted 15 lines · Sakari Ailus

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 <[email protected]>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -0
index 452eb9b42140..723022ef3662 100644

drivers/media/pci/intel/ipu3/ipu3-cio2.c static void cio2_pci_remove(struct pci_dev *pci_dev) · line 1853

Quoted 9 lines

drivers/media/pci/intel/ipu3/ipu3-cio2.c +2 -1

@@ -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(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
 	for (i = 0; i < CIO2_QUEUES; i++)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_fbpt_exit_dummy(cio2);

Hi, Sakari,
The fbpt dummy pages cleanup does not matter much before/after queues exit, right?

Quoted 5 lines

drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -1

 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 }

Sakari Ailus

author maintainer

2018-10-15 16:23 UTC #4 lore raw

Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources

Hi Bingbu,

Quoted 20 lines · Bing Bu Cao

Quoted 18 lines · Sakari Ailus
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 <[email protected]>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -0
index 452eb9b42140..723022ef3662 100644
@@ -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;
 

Quoted 9 lines

drivers/media/pci/intel/ipu3/ipu3-cio2.c static void cio2_pci_remove(struct pci_dev *pci_dev) · line 1853
Quoted 6 lines
drivers/media/pci/intel/ipu3/ipu3-cio2.c +2 -1
+	media_device_unregister(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
 	for (i = 0; i < CIO2_QUEUES; i++)
 		cio2_queue_exit(cio2, &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.

Quoted 6 lines

Quoted 5 lines

drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -1

 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 }

Signature · 2 lines

Sakari Ailus
[email protected]

Bing Bu Cao

2018-10-15 16:56 UTC #5 lore raw

Re: [PATCH 1/2] ipu3-cio2: Unregister device nodes first, then release resources

Quoted 31 lines · Sakari Ailus

Hi Bingbu,
Quoted 19 lines · Bing Bu Cao
Quoted 18 lines · Sakari Ailus
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 <[email protected]>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -0
index 452eb9b42140..723022ef3662 100644
@@ -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;
 
Quoted 9 lines
drivers/media/pci/intel/ipu3/ipu3-cio2.c static void cio2_pci_remove(struct pci_dev *pci_dev) · line 1853
Quoted 6 lines
drivers/media/pci/intel/ipu3/ipu3-cio2.c +2 -1
+	media_device_unregister(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
 	for (i = 0; i < CIO2_QUEUES; i++)
 		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_fbpt_exit_dummy(cio2);
Hi, Sakari,
The fbpt dummy pages cleanup does not matter much before/after queues exit, right?

Quoted 4 lines

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.

Quoted 5 lines

drivers/media/pci/intel/ipu3/ipu3-cio2.c +0 -1

 	v4l2_device_unregister(&cio2->v4l2_dev);
-	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 }

Sakari Ailus

author maintainer

2018-10-10 15:53 UTC #6 lore raw

PATCH 2/2 ipu3-cio2: Use cio2_queues_exit

In reply to Sakari Ailus

The ipu3-cio2 driver has a function to tear down video devices as well as the associated video buffer queues. Use it.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +1 -3

index 723022ef3662..447baaebca44 100644
@@ -1844,12 +1844,10 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 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(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	for (i = 0; i < CIO2_QUEUES; i++)
-		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_queues_exit(cio2);
 	cio2_fbpt_exit_dummy(cio2);
 	v4l2_device_unregister(&cio2->v4l2_dev);
 	media_device_cleanup(&cio2->media_dev);

Signature · 1 line

2.11.0

Bing Bu Cao

Tested-by Bingbu Cao Reviewed-by Bingbu Cao on v1 2/2

2018-10-11 16:38 UTC #7 lore raw

Re: [PATCH 2/2] ipu3-cio2: Use cio2_queues_exit

Tested-by Bingbu Cao <[email protected]>

Reviewed-by Bingbu Cao <[email protected]>

Quoted 26 lines · Sakari Ailus

The ipu3-cio2 driver has a function to tear down video devices as well as the associated video buffer queues. Use it.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +1 -3

index 723022ef3662..447baaebca44 100644
@@ -1844,12 +1844,10 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 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(&cio2->media_dev);
 	cio2_notifier_exit(cio2);
-	for (i = 0; i < CIO2_QUEUES; i++)
-		cio2_queue_exit(cio2, &cio2->queue[i]);
+	cio2_queues_exit(cio2);
 	cio2_fbpt_exit_dummy(cio2);
 	v4l2_device_unregister(&cio2->v4l2_dev);
 	media_device_cleanup(&cio2->media_dev);

Sakari Ailus

author maintainer

2018-10-14 03:53 UTC #8 lore raw

Re: [PATCH 2/2] ipu3-cio2: Use cio2_queues_exit

Quoted 2 lines · Bing Bu Cao

Tested-by Bingbu Cao <[email protected]>
Reviewed-by Bingbu Cao <[email protected]>

Thanks!!

Signature · 2 lines

Sakari Ailus
[email protected]