[v2] soc-camera: fix missing clean up on error path

Commit Message

Guennadi Liakhovetski June 19, 2009, 6:54 a.m. UTC

soc-camera: fix missing clean up on error path

If soc_camera_init_user_formats() fails in soc_camera_probe(), we have to call
client's .remove() method to unregister the video device.

Reported-by: Kuninori Morimoto <[e-mail]>
Signed-off-by: Guennadi Liakhovetski <[e-mail]>
---
(please use the new V4L Mailing List)

On Fri, 19 Jun 2009, Kuninori Morimoto wrote:

> > If soc_camera_init_user_formats() fails in soc_camera_probe(), we have to call
> > client's .remove() method to unregister the video device.
> > 
> > Reported-by: Kuninori Morimoto <[e-mail]>
> > Signed-off-by: Guennadi Liakhovetski <[e-mail]>
> > ---
> > Hi Morimoto-san
> (snip)
> > Could you please verify that this patch fixed your problem?
> 
> Thank you nice patch.
> I tried this, but kernel stoped in boot.
> 
> soc_camera_video_stop is called from icd->ops->remove 
> I think it have dead lock by icd->video_lock.
> 
> my kernel is from Paul's git and it's Makefile said 2.6.30-rc6

Yes, you're right. Please, try this version, but this is a bigger change, 
also affecting the regular (not error) path, so, I will have to test it 
too.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kuninori Morimoto June 22, 2009, 12:26 a.m. UTC | #1

Dear Guennadi

Thank you for your hard work

> > soc_camera_video_stop is called from icd->ops->remove 
> > I think it have dead lock by icd->video_lock.
> > 
> > my kernel is from Paul's git and it's Makefile said 2.6.30-rc6
> 
> Yes, you're right. Please, try this version, but this is a bigger change, 
> also affecting the regular (not error) path, so, I will have to test it 
> too.

Thanks. but I'm very busy now.
Please wait for me.

Best regards
--
Kuninori Morimoto
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kuninori Morimoto June 26, 2009, 2:48 a.m. UTC | #2

Dear Guennadi

> > Yes, you're right. Please, try this version, but this is a bigger change, 
> > also affecting the regular (not error) path, so, I will have to test it 
> > too.
> 
> Thanks. but I'm very busy now.
> Please wait for me.

I tested this patch.
And I works well for me !!

Thank you

Best regards
--
Kuninori Morimoto
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 78010ab..9f5ae81 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -877,8 +877,11 @@  static int soc_camera_probe(struct device *dev)
 			(unsigned short)~0;
 
 		ret = soc_camera_init_user_formats(icd);
-		if (ret < 0)
+		if (ret < 0) {
+			if (icd->ops->remove)
+				icd->ops->remove(icd);
 			goto eiufmt;
+		}
 
 		icd->height	= DEFAULT_HEIGHT;
 		icd->width	= DEFAULT_WIDTH;
@@ -902,8 +905,10 @@  static int soc_camera_remove(struct device *dev)
 {
 	struct soc_camera_device *icd = to_soc_camera_dev(dev);
 
+	mutex_lock(&icd->video_lock);
 	if (icd->ops->remove)
 		icd->ops->remove(icd);
+	mutex_unlock(&icd->video_lock);
 
 	soc_camera_free_user_formats(icd);
 
@@ -1145,6 +1150,7 @@  evidallocd:
 }
 EXPORT_SYMBOL(soc_camera_video_start);
 
+/* Called from client .remove() methods with .video_lock held */
 void soc_camera_video_stop(struct soc_camera_device *icd)
 {
 	struct video_device *vdev = icd->vdev;
@@ -1154,10 +1160,8 @@  void soc_camera_video_stop(struct soc_camera_device *icd)
 	if (!icd->dev.parent || !vdev)
 		return;
 
-	mutex_lock(&icd->video_lock);
 	video_unregister_device(vdev);
 	icd->vdev = NULL;
-	mutex_unlock(&icd->video_lock);
 }
 EXPORT_SYMBOL(soc_camera_video_stop);