Re: [PATCH v9 02/20] V4L2: support asynchronous subdevice registration

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Guennadi,

On 04/23/2013 03:01 PM, Guennadi Liakhovetski wrote:
> On Mon, 22 Apr 2013, Laurent Pinchart wrote:
>> On Monday 15 April 2013 13:57:17 Sylwester Nawrocki wrote:
>>> On 04/12/2013 05:40 PM, Guennadi Liakhovetski wrote:
>>>> +
>>>> +		if (notifier->unbind)
>>>> +			notifier->unbind(notifier, asdl);
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&list_lock);
>>>> +
>>>> +	if (dev) {
>>>> +		while (i--) {
>>>> +			if (dev[i]&&  device_attach(dev[i])<  0)
>>
>> This is my last major pain point.
>>
>> To avoid race conditions we need circular references (see
>>http://www.mail-archive.com/linux-media@vger.kernel.org/msg61092.html).
>>We will thus need a
>> way to break the circle by explictly requesting the subdev to release its
>> resources. I'm afraid I have no well-designed solution for that at the moment.
>
> I think we really can design the framework to allow a _safe_ unloading of
> the bridge driver. An rmmod run is not an immediate death - we have time
> to clean up and release all resources properly. As an example, I just had
> a network interface running, but rmmod-ing of one of hardware drivers just
> safely destroyed the interface. In our case, rmmod<bridge>  should just
> signal the subdevice to release the clock reference. Whether we have the
> required - is a separate question.

It sounds like a reasonable requirements.

> Currently a call to v4l2_clk_get() increments the clock owner use-count.
> This isn't a problem for soc-camera, since there the soc-camera core owns
> the clock. For other bridge drivers they would probably own the clock
> themselves, so, incrementing their use-count would block their modules in
> memory. To avoid that we have to remove that use-count incrementing.
>
> The crash, described in the referenced mail can happen if the subdevice
> driver calls (typically) v4l2_clk_enable() on a clock, that's already been
> freed. Wouldn't a locked look-up in the global clock list in v4l2-clk.c
> prevent such a crash? E.g.
>
> int v4l2_clk_enable(struct v4l2_clk *clk)
> {
> 	struct v4l2_clk *tmp;
> 	int ret = -ENODEV;
>
> 	mutex_lock(&clk_lock);
> 	list_for_each_entry(tmp,&clk_list, list)
> 		if (tmp = clk) {
> 			ret = !try_module_get(clk->ops->owner);
> 			if (ret)
> 				ret = -EFAULT;
> 			break;
> 		}
> 	mutex_unlock(&clk_lock);
>
> 	if (ret<  0)
> 		return ret;
>
> 	...
> }
>
> We'd have to do a similar locked look-up in v4l2_clk_put().

Sounds good. This way it will not be possible to unload modules when 
clock is
enabled, which is expected. And it seems straightforward to ensure 
clk_prepare/
clk_unprepare, clk_enable/clk_disable are properly balanced. If module 
is gone
before subdev driver calls v4l2_clk_put() the clock provider module will 
have
to ensure any source clocks it uses are properly released 
(clk_unprepare/clk_put).

I'm looking forward to try your v10. :-)

Regards,
Sylwester