Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Thu, 15 Dec 2016 09:42:35 -0300
Javier Martinez Canillas <javier@osg.samsung.com> escreveu:

> Hello Mauro,
> 
> On 12/15/2016 09:37 AM, Mauro Carvalho Chehab wrote:
> 
> [snip]
> 
> > 
> > What happens is that omap3isp driver calls media_device_unregister()
> > too early. Right now, it is called at omap3isp_video_device_release(),
> > with happens when a driver unbind is ordered by userspace, and not after
> > the last usage of all /dev/video?? devices.
> > 
> > There are two possible fixes:
> > 
> > 1) at omap3isp_video_device_release(), streamoff all streams and mark
> > that the media device will be gone.

I actually meant to say: isp_unregister_entities() here.

> > 
> > 2) instead of using video_device_release_empty for the video->video.release,
> > create a omap3isp_video_device_release() that will call
> > media_device_unregister() when destroying the last /dev/video?? devnode.
> >  
> 
> There's also option (3), to have a proper refcounting to make sure that
> the media device node is not freed until all references to it are gone.

Yes, that's another alternative.

> I understand that's what Sakari's RFC patches do. I'll try to make some
> time tomorrow to test and review his patches.

The biggest problem with Sakari's patches is that it starts by 
reverting 3 patches, and this will cause regressions on existing
devices.

Development should be incremental.

I didn't review carefully his series (as it started the wrong way),
but I guess there's another problem on it: as OMAP3 remove entities
at isp_unregister_entities(), while adding a kref to media_device
will prevent an oops, the streamoff logic won't work anymore, as
the entities that were supposed to be at the graph will have been
removed by then.

Ok, we can roll the snow ball and add kref's to entities and links,
but IMHO, we're trying to kill a fly with a death star: instead,
the better is to just fix the driver in a way that it would be
streaming off everything at isp_unregister_entities(), before
dropping the entities and the media controller.

Regards,
Mauro