Hi Laurent
On Mon, 22 Apr 2013, Laurent Pinchart wrote:
> Hi Guennadi and Sylwester,
>
> On Monday 15 April 2013 13:57:17 Sylwester Nawrocki wrote:
> > On 04/12/2013 05:40 PM, Guennadi Liakhovetski wrote:
[snip]
> > > +
> > > + 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@xxxxxxxxxxxxxxx/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.
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().
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
