[v2] media: V4L2: add temporary clock helpers

Commit Message

Guennadi Liakhovetski Oct. 30, 2012, 2:18 p.m. UTC

Typical video devices like camera sensors require an external clock source.
Many such devices cannot even access their hardware registers without a
running clock. These clock sources should be controlled by their consumers.
This should be performed, using the generic clock framework. Unfortunately
so far only very few systems have been ported to that framework. This patch
adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
Platforms, adopting the clock API, should switch to using it. Eventually
this temporary API should be removed.

Signed-off-by: Guennadi Liakhovetski <[e-mail]>
---

v2: integrated most fixes from Sylwester & Laurent,

(1) do not register identical clocks
(2) add clock refcounting
(3) more robust locking
(4) duplicate strings to prevent dereferencing invalid memory
(5) add a private data pointer
(6) return an error in get_rate() / set_rate() if clock isn't enabled
(7) support .id=NULL per subdevice

Thanks to all reviewers!

 drivers/media/v4l2-core/Makefile   |    2 +-
 drivers/media/v4l2-core/v4l2-clk.c |  169 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-clk.h           |   51 +++++++++++
 3 files changed, 221 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 create mode 100644 include/media/v4l2-clk.h

Comments

Laurent Pinchart Oct. 31, 2012, 9:15 a.m. UTC | #1

Hi Guennadi,

Thanks for the patch.

On Tuesday 30 October 2012 15:18:38 Guennadi Liakhovetski wrote:
> Typical video devices like camera sensors require an external clock source.
> Many such devices cannot even access their hardware registers without a
> running clock. These clock sources should be controlled by their consumers.
> This should be performed, using the generic clock framework. Unfortunately
> so far only very few systems have been ported to that framework. This patch
> adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> Platforms, adopting the clock API, should switch to using it. Eventually
> this temporary API should be removed.
> 
> Signed-off-by: Guennadi Liakhovetski <[e-mail]>
> ---
> 
> v2: integrated most fixes from Sylwester & Laurent,
> 
> (1) do not register identical clocks
> (2) add clock refcounting
> (3) more robust locking
> (4) duplicate strings to prevent dereferencing invalid memory
> (5) add a private data pointer
> (6) return an error in get_rate() / set_rate() if clock isn't enabled
> (7) support .id=NULL per subdevice
> 
> Thanks to all reviewers!
> 
>  drivers/media/v4l2-core/Makefile   |    2 +-
>  drivers/media/v4l2-core/v4l2-clk.c |  169 +++++++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h           |   51 +++++++++++
>  3 files changed, 221 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>  create mode 100644 include/media/v4l2-clk.h
> 
> diff --git a/drivers/media/v4l2-core/Makefile
> b/drivers/media/v4l2-core/Makefile index 00f64d6..cb5fede 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -5,7 +5,7 @@
>  tuner-objs	:=	tuner-core.o
> 
>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> -			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> +			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>  ifeq ($(CONFIG_COMPAT),y)
>    videodev-objs += v4l2-compat-ioctl32.o
>  endif
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> b/drivers/media/v4l2-core/v4l2-clk.c new file mode 100644
> index 0000000..2496807
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,169 @@
> +/*
> + * V4L2 clock service
> + *
> + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/errno.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/string.h>
> +
> +#include <media/v4l2-clk.h>
> +#include <media/v4l2-subdev.h>
> +
> +static DEFINE_MUTEX(clk_lock);
> +static LIST_HEAD(v4l2_clk);

As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?

> +static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
> +{
> +	struct v4l2_clk *clk;
> +
> +	list_for_each_entry(clk, &v4l2_clk, list) {
> +		if (strcmp(dev_id, clk->dev_id))
> +			continue;
> +
> +		if (!id || !clk->id || !strcmp(clk->id, id))

If id != NULL and clk->id == NULL, the "unnamed" clock will be returned even 
though the caller requests a named clock. Isn't that a mistake ?

> +			return clk;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> +{
> +	struct v4l2_clk *clk;
> +
> +	mutex_lock(&clk_lock);
> +	clk = v4l2_clk_find(sd->name, id);
> +
> +	if (!IS_ERR(clk) && !try_module_get(clk->ops->owner))
> +		clk = ERR_PTR(-ENODEV);
> +	mutex_unlock(&clk_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_get);
> +
> +void v4l2_clk_put(struct v4l2_clk *clk)
> +{
> +	if (!IS_ERR(clk))
> +		module_put(clk->ops->owner);
> +}
> +EXPORT_SYMBOL(v4l2_clk_put);
> +
> +int v4l2_clk_enable(struct v4l2_clk *clk)
> +{
> +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> +		int ret = clk->ops->enable(clk);
> +		if (ret < 0)
> +			atomic_dec(&clk->enable);
> +		return ret;
> +	}

I think you need a spinlock here instead of atomic operations. You could get 
preempted after atomic_inc_return() and before clk->ops->enable() by another 
process that would call v4l2_clk_enable(). The function would return with 
enabling the clock.

One solution would be to add a spinlock to struct v4l2_clk and modify the 
enable field from atomic_t to plain unsigned int.

> +	return 0;
> +}
> +EXPORT_SYMBOL(v4l2_clk_enable);
> +
> +void v4l2_clk_disable(struct v4l2_clk *clk)
> +{
> +	int enable = atomic_dec_return(&clk->enable);
> +
> +	if (WARN(enable < 0, "Unbalanced %s()!\n", __func__)) {

Could you add the clock name to the debug message ?

> +		atomic_inc(&clk->enable);
> +		return;
> +	}
> +
> +	if (!enable && clk->ops->disable)
> +		clk->ops->disable(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_disable);
> +
> +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> +{
> +	if (!atomic_read(&clk->enable))
> +		return -EINVAL;
> +
> +	if (!clk->ops->get_rate)
> +		return -ENOSYS;
> +
> +	return clk->ops->get_rate(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_get_rate);
> +
> +int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> +{
> +	if (!atomic_read(&clk->enable))
> +		return -EINVAL;

Setting (and thus getting) the rate of a disabled clock should be valid, 
otherwise you'll have to enable the clock with an unknown rate first and then 
modify the rate.

> +	if (!clk->ops->set_rate)
> +		return -ENOSYS;
> +
> +	return clk->ops->set_rate(clk, rate);
> +}
> +EXPORT_SYMBOL(v4l2_clk_set_rate);
> +
> +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> +				   const char *dev_id,
> +				   const char *id, void *priv)
> +{
> +	struct v4l2_clk *clk;
> +
> +	if (!ops || !dev_id)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&clk_lock);
> +	clk = v4l2_clk_find(dev_id, id);
> +
> +	if (!IS_ERR(clk)) {
> +		clk = ERR_PTR(-EEXIST);
> +		goto out;
> +	}
> +
> +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +	if (!clk) {
> +		clk = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
> +
> +	clk->ops = ops;
> +	clk->id = kstrdup(id, GFP_KERNEL);
> +	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> +	clk->priv = priv;
> +	atomic_set(&clk->enable, 0);
> +
> +	list_add_tail(&clk->list, &v4l2_clk);

I might have lived the kzalloc + init code above out of the mutex-protected 
area to lower the possible mutex contention time. That would optimize the non-
error code path. Something like

	kzalloc clk
	if (failed)
		return -ENOMEM
	init clk
	if (failed)
		return -ENOMEM
	mutex_lock
	find existing clock
	if (!found)
		add to v4l2_clk list
	else
		ret = -EEXIST
	mutex_unlock
	return ret

> +out:
> +	if (!IS_ERR(clk) && ((id && !clk->id) || !clk->dev_id)) {
> +		list_del(&clk->list);
> +		kfree(clk->id);
> +		kfree(clk->dev_id);
> +		kfree(clk);
> +		clk = ERR_PTR(-ENOMEM);
> +	}
> +
> +	mutex_unlock(&clk_lock);
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL(v4l2_clk_register);
> +
> +void v4l2_clk_unregister(struct v4l2_clk *clk)
> +{
> +	WARN(atomic_read(&clk->enable),
> +	     "Unregistering still enabled %s:%s clock!\n", clk->dev_id, clk->id);

Shouldn't this warning be based on a get/put refcount instead of an enable 
refcount ?

I would also turn it into a BUG_ON. The kernel will crash later anyway when 
the clock user will try to disable the clock, as you free the clock object 
here.

> +	mutex_lock(&clk_lock);
> +	list_del(&clk->list);
> +	mutex_unlock(&clk_lock);
> +
> +	kfree(clk->id);
> +	kfree(clk->dev_id);
> +	kfree(clk);
> +}
> +EXPORT_SYMBOL(v4l2_clk_unregister);
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> new file mode 100644
> index 0000000..f70664b
> --- /dev/null
> +++ b/include/media/v4l2-clk.h
> @@ -0,0 +1,51 @@
> +/*
> + * V4L2 clock service
> + *
> + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * ATTENTION: This is a temporary API and it shall be replaced by the
> generic + * clock API, when the latter becomes widely available.
> + */
> +
> +#ifndef MEDIA_V4L2_CLK_H
> +#define MEDIA_V4L2_CLK_H
> +
> +#include <linux/atomic.h>
> +#include <linux/list.h>
> +
> +struct module;
> +struct v4l2_subdev;
> +
> +struct v4l2_clk {
> +	struct list_head list;
> +	const struct v4l2_clk_ops *ops;
> +	char *dev_id;
> +	const char *id;

Is there any reason to have a const id and an unconst dev_id ?

> +	atomic_t enable;
> +	void *priv;
> +};
> +
> +struct v4l2_clk_ops {
> +	struct module	*owner;
> +	int		(*enable)(struct v4l2_clk *clk);
> +	void		(*disable)(struct v4l2_clk *clk);
> +	unsigned long	(*get_rate)(struct v4l2_clk *clk);
> +	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
> +};
> +
> +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> +				   const char *dev_name,
> +				   const char *name, void *priv);
> +void v4l2_clk_unregister(struct v4l2_clk *clk);
> +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id);
> +void v4l2_clk_put(struct v4l2_clk *clk);
> +int v4l2_clk_enable(struct v4l2_clk *clk);
> +void v4l2_clk_disable(struct v4l2_clk *clk);
> +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
> +int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
> +
> +#endif

Guennadi Liakhovetski Oct. 31, 2012, 1:02 p.m. UTC | #2

Hi Laurent

Thanks for the review

On Wed, 31 Oct 2012, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the patch.
> 
> On Tuesday 30 October 2012 15:18:38 Guennadi Liakhovetski wrote:
> > Typical video devices like camera sensors require an external clock source.
> > Many such devices cannot even access their hardware registers without a
> > running clock. These clock sources should be controlled by their consumers.
> > This should be performed, using the generic clock framework. Unfortunately
> > so far only very few systems have been ported to that framework. This patch
> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> > Platforms, adopting the clock API, should switch to using it. Eventually
> > this temporary API should be removed.
> > 
> > Signed-off-by: Guennadi Liakhovetski <[e-mail]>
> > ---
> > 
> > v2: integrated most fixes from Sylwester & Laurent,
> > 
> > (1) do not register identical clocks
> > (2) add clock refcounting
> > (3) more robust locking
> > (4) duplicate strings to prevent dereferencing invalid memory
> > (5) add a private data pointer
> > (6) return an error in get_rate() / set_rate() if clock isn't enabled
> > (7) support .id=NULL per subdevice
> > 
> > Thanks to all reviewers!
> > 
> >  drivers/media/v4l2-core/Makefile   |    2 +-
> >  drivers/media/v4l2-core/v4l2-clk.c |  169 +++++++++++++++++++++++++++++++++
> >  include/media/v4l2-clk.h           |   51 +++++++++++
> >  3 files changed, 221 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >  create mode 100644 include/media/v4l2-clk.h
> > 
> > diff --git a/drivers/media/v4l2-core/Makefile
> > b/drivers/media/v4l2-core/Makefile index 00f64d6..cb5fede 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -5,7 +5,7 @@
> >  tuner-objs	:=	tuner-core.o
> > 
> >  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> > -			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> > +			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> >  ifeq ($(CONFIG_COMPAT),y)
> >    videodev-objs += v4l2-compat-ioctl32.o
> >  endif
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c new file mode 100644
> > index 0000000..2496807
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -0,0 +1,169 @@
> > +/*
> > + * V4L2 clock service
> > + *
> > + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/errno.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/string.h>
> > +
> > +#include <media/v4l2-clk.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +static DEFINE_MUTEX(clk_lock);
> > +static LIST_HEAD(v4l2_clk);
> 
> As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?

Don't you think naming of a static variable isn't important enough? ;-) I 
think code authors should have enough freedom to at least pick up single 
vs. plural form:-) "clks" is too many consonants for my taste, if it were 
anything important I'd rather agree to "clk_head" or "clk_list" or 
something similar.

> > +static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
> > +{
> > +	struct v4l2_clk *clk;
> > +
> > +	list_for_each_entry(clk, &v4l2_clk, list) {
> > +		if (strcmp(dev_id, clk->dev_id))
> > +			continue;
> > +
> > +		if (!id || !clk->id || !strcmp(clk->id, id))
> 
> If id != NULL and clk->id == NULL, the "unnamed" clock will be returned even 
> though the caller requests a named clock. Isn't that a mistake ?

If clk->id == NULL this means it's the only clock with this dev_id. We 
definitely don't want to allow multiple clocks on one subdev, of which one 
has clk->id == NULL. If we don't return a valid pointer here, 
v4l2_clk_register() will decide, that there's no conflict and register 
this clock, which would be an error. As for v4l2_clk_get() - not sure in 
fact. Looking at drivers/clk/clkdev.c::clk_find() if you call 
clk_get(dev, "con-id") and you've got a clock lookup entry registered with 
matching device ID and NULL connection ID, it will match. So, I don't 
think it's too important, we can choose either way.

> > +			return clk;
> > +	}
> > +
> > +	return ERR_PTR(-ENODEV);
> > +}
> > +
> > +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
> > +{
> > +	struct v4l2_clk *clk;
> > +
> > +	mutex_lock(&clk_lock);
> > +	clk = v4l2_clk_find(sd->name, id);
> > +
> > +	if (!IS_ERR(clk) && !try_module_get(clk->ops->owner))
> > +		clk = ERR_PTR(-ENODEV);
> > +	mutex_unlock(&clk_lock);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_get);
> > +
> > +void v4l2_clk_put(struct v4l2_clk *clk)
> > +{
> > +	if (!IS_ERR(clk))
> > +		module_put(clk->ops->owner);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_put);
> > +
> > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > +{
> > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > +		int ret = clk->ops->enable(clk);
> > +		if (ret < 0)
> > +			atomic_dec(&clk->enable);
> > +		return ret;
> > +	}
> 
> I think you need a spinlock here instead of atomic operations. You could get 
> preempted after atomic_inc_return() and before clk->ops->enable() by another 
> process that would call v4l2_clk_enable(). The function would return with 
> enabling the clock.

Sorry, what's the problem then? "Our" instance will succeed and call 
->enable() and the preempting instance will see the enable count > 1 and 
just return.

> One solution would be to add a spinlock to struct v4l2_clk and modify the 
> enable field from atomic_t to plain unsigned int.
> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_enable);
> > +
> > +void v4l2_clk_disable(struct v4l2_clk *clk)
> > +{
> > +	int enable = atomic_dec_return(&clk->enable);
> > +
> > +	if (WARN(enable < 0, "Unbalanced %s()!\n", __func__)) {
> 
> Could you add the clock name to the debug message ?

You mean device / connection IDs? Could do, yes.

> > +		atomic_inc(&clk->enable);
> > +		return;
> > +	}
> > +
> > +	if (!enable && clk->ops->disable)
> > +		clk->ops->disable(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_disable);
> > +
> > +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> > +{
> > +	if (!atomic_read(&clk->enable))
> > +		return -EINVAL;
> > +
> > +	if (!clk->ops->get_rate)
> > +		return -ENOSYS;
> > +
> > +	return clk->ops->get_rate(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_get_rate);
> > +
> > +int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> > +{
> > +	if (!atomic_read(&clk->enable))
> > +		return -EINVAL;
> 
> Setting (and thus getting) the rate of a disabled clock should be valid, 
> otherwise you'll have to enable the clock with an unknown rate first and then 
> modify the rate.

You're right, will fix.

> > +	if (!clk->ops->set_rate)
> > +		return -ENOSYS;
> > +
> > +	return clk->ops->set_rate(clk, rate);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_set_rate);
> > +
> > +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> > +				   const char *dev_id,
> > +				   const char *id, void *priv)
> > +{
> > +	struct v4l2_clk *clk;
> > +
> > +	if (!ops || !dev_id)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	mutex_lock(&clk_lock);
> > +	clk = v4l2_clk_find(dev_id, id);
> > +
> > +	if (!IS_ERR(clk)) {
> > +		clk = ERR_PTR(-EEXIST);
> > +		goto out;
> > +	}
> > +
> > +	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> > +	if (!clk) {
> > +		clk = ERR_PTR(-ENOMEM);
> > +		goto out;
> > +	}
> > +
> > +	clk->ops = ops;
> > +	clk->id = kstrdup(id, GFP_KERNEL);
> > +	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> > +	clk->priv = priv;
> > +	atomic_set(&clk->enable, 0);
> > +
> > +	list_add_tail(&clk->list, &v4l2_clk);
> 
> I might have lived the kzalloc + init code above out of the mutex-protected 
> area to lower the possible mutex contention time. That would optimize the non-
> error code path. Something like
> 
> 	kzalloc clk
> 	if (failed)
> 		return -ENOMEM
> 	init clk
> 	if (failed)
> 		return -ENOMEM
> 	mutex_lock
> 	find existing clock
> 	if (!found)
> 		add to v4l2_clk list
> 	else
> 		ret = -EEXIST
> 	mutex_unlock
> 	return ret

Well, you have to call v4l2_clk_find() locked, that's right. And then, if 
the entry is free, you have to fill it in under the lock too. But, if any 
of 3 allocations fail or if the entry is busy you'd have to free all the 
memory, that you allocated so far. So, don't think there's a huge 
difference, but yes, holding the lock a bit shorter is good, will see if 
changing this becomes much uglier:-)

> > +out:
> > +	if (!IS_ERR(clk) && ((id && !clk->id) || !clk->dev_id)) {
> > +		list_del(&clk->list);
> > +		kfree(clk->id);
> > +		kfree(clk->dev_id);
> > +		kfree(clk);
> > +		clk = ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	mutex_unlock(&clk_lock);
> > +
> > +	return clk;
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_register);
> > +
> > +void v4l2_clk_unregister(struct v4l2_clk *clk)
> > +{
> > +	WARN(atomic_read(&clk->enable),
> > +	     "Unregistering still enabled %s:%s clock!\n", clk->dev_id, clk->id);
> 
> Shouldn't this warning be based on a get/put refcount instead of an enable 
> refcount ?

In principle - yes, so, you vote for one more counter?...

> I would also turn it into a BUG_ON. The kernel will crash later anyway when 
> the clock user will try to disable the clock, as you free the clock object 
> here.

s/when/if/ ;-) With BUG_ON() you, probably, only get one stack dump here, 
with WARN() you get both - one with the _unregister() stack and one with 
the _disable() and / or _put() stack... Don't you think the latter option 
is more informative?

> > +	mutex_lock(&clk_lock);
> > +	list_del(&clk->list);
> > +	mutex_unlock(&clk_lock);
> > +
> > +	kfree(clk->id);
> > +	kfree(clk->dev_id);
> > +	kfree(clk);
> > +}
> > +EXPORT_SYMBOL(v4l2_clk_unregister);
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > new file mode 100644
> > index 0000000..f70664b
> > --- /dev/null
> > +++ b/include/media/v4l2-clk.h
> > @@ -0,0 +1,51 @@
> > +/*
> > + * V4L2 clock service
> > + *
> > + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * ATTENTION: This is a temporary API and it shall be replaced by the
> > generic + * clock API, when the latter becomes widely available.
> > + */
> > +
> > +#ifndef MEDIA_V4L2_CLK_H
> > +#define MEDIA_V4L2_CLK_H
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/list.h>
> > +
> > +struct module;
> > +struct v4l2_subdev;
> > +
> > +struct v4l2_clk {
> > +	struct list_head list;
> > +	const struct v4l2_clk_ops *ops;
> > +	char *dev_id;
> > +	const char *id;
> 
> Is there any reason to have a const id and an unconst dev_id ?

Unlikely:-)

Thanks
Guennadi

> > +	atomic_t enable;
> > +	void *priv;
> > +};
> > +
> > +struct v4l2_clk_ops {
> > +	struct module	*owner;
> > +	int		(*enable)(struct v4l2_clk *clk);
> > +	void		(*disable)(struct v4l2_clk *clk);
> > +	unsigned long	(*get_rate)(struct v4l2_clk *clk);
> > +	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
> > +};
> > +
> > +struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> > +				   const char *dev_name,
> > +				   const char *name, void *priv);
> > +void v4l2_clk_unregister(struct v4l2_clk *clk);
> > +struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id);
> > +void v4l2_clk_put(struct v4l2_clk *clk);
> > +int v4l2_clk_enable(struct v4l2_clk *clk);
> > +void v4l2_clk_disable(struct v4l2_clk *clk);
> > +unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
> > +int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
> > +
> > +#endif
> -- 
> Regards,
> 
> Laurent Pinchart

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus Nov. 11, 2012, 10:33 p.m. UTC | #3

Hi Guennadi,

Thanks for the patch!

On Wed, Oct 31, 2012 at 02:02:54PM +0100, Guennadi Liakhovetski wrote:
> On Wed, 31 Oct 2012, Laurent Pinchart wrote:
...
> > > +#include <linux/atomic.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/string.h>
> > > +
> > > +#include <media/v4l2-clk.h>
> > > +#include <media/v4l2-subdev.h>
> > > +
> > > +static DEFINE_MUTEX(clk_lock);
> > > +static LIST_HEAD(v4l2_clk);
> > 
> > As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> 
> Don't you think naming of a static variable isn't important enough? ;-) I 
> think code authors should have enough freedom to at least pick up single 
> vs. plural form:-) "clks" is too many consonants for my taste, if it were 
> anything important I'd rather agree to "clk_head" or "clk_list" or 
> something similar.

clk_list makes sense IMO since the clk_ prefis is the same.

...

> > > +void v4l2_clk_put(struct v4l2_clk *clk)
> > > +{
> > > +	if (!IS_ERR(clk))
> > > +		module_put(clk->ops->owner);
> > > +}
> > > +EXPORT_SYMBOL(v4l2_clk_put);
> > > +
> > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > +{
> > > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > > +		int ret = clk->ops->enable(clk);
> > > +		if (ret < 0)
> > > +			atomic_dec(&clk->enable);
> > > +		return ret;
> > > +	}
> > 
> > I think you need a spinlock here instead of atomic operations. You could get 
> > preempted after atomic_inc_return() and before clk->ops->enable() by another 
> > process that would call v4l2_clk_enable(). The function would return with 
> > enabling the clock.
> 
> Sorry, what's the problem then? "Our" instance will succeed and call 
> ->enable() and the preempting instance will see the enable count > 1 and 
> just return.

The clock is guaranteed to be enabled only after the call has returned. The
second caller of v4lw_clk_enable() thus may proceed without the clock being
enabled.

In principle enable() might also want to sleep, so how about using a mutex
for the purpose instead of a spinlock?

...

> > > +struct v4l2_clk_ops {
> > > +	struct module	*owner;
> > > +	int		(*enable)(struct v4l2_clk *clk);
> > > +	void		(*disable)(struct v4l2_clk *clk);
> > > +	unsigned long	(*get_rate)(struct v4l2_clk *clk);
> > > +	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);

How about unsigned long hz?

Laurent Pinchart Nov. 12, 2012, 11:04 a.m. UTC | #4

Hi Guennadi,

On Wednesday 31 October 2012 14:02:54 Guennadi Liakhovetski wrote:
> On Wed, 31 Oct 2012, Laurent Pinchart wrote:
> > On Tuesday 30 October 2012 15:18:38 Guennadi Liakhovetski wrote:
> > > Typical video devices like camera sensors require an external clock
> > > source. Many such devices cannot even access their hardware registers
> > > without a running clock. These clock sources should be controlled by
> > > their consumers. This should be performed, using the generic clock
> > > framework. Unfortunately so far only very few systems have been ported
> > > to that framework. This patch adds a set of temporary helpers, mimicking
> > > the generic clock API, to V4L2. Platforms, adopting the clock API,
> > > should switch to using it. Eventually this temporary API should be
> > > removed.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <[e-mail]>
> > > ---
> > > 
> > > v2: integrated most fixes from Sylwester & Laurent,
> > > 
> > > (1) do not register identical clocks
> > > (2) add clock refcounting
> > > (3) more robust locking
> > > (4) duplicate strings to prevent dereferencing invalid memory
> > > (5) add a private data pointer
> > > (6) return an error in get_rate() / set_rate() if clock isn't enabled
> > > (7) support .id=NULL per subdevice
> > > 
> > > Thanks to all reviewers!
> > > 
> > >  drivers/media/v4l2-core/Makefile   |    2 +-
> > >  drivers/media/v4l2-core/v4l2-clk.c |  169 +++++++++++++++++++++++++++++
> > >  include/media/v4l2-clk.h           |   51 +++++++++++
> > >  3 files changed, 221 insertions(+), 1 deletions(-)
> > >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> > >  create mode 100644 include/media/v4l2-clk.h

[snip]

> > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > +{
> > > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > > +		int ret = clk->ops->enable(clk);
> > > +		if (ret < 0)
> > > +			atomic_dec(&clk->enable);
> > > +		return ret;
> > > +	}
> > 
> > I think you need a spinlock here instead of atomic operations. You could
> > get preempted after atomic_inc_return() and before clk->ops->enable() by
> > another process that would call v4l2_clk_enable(). The function would
> > return with enabling the clock.
> 
> Sorry, what's the problem then? "Our" instance will succeed and call
> ->enable() and the preempting instance will see the enable count > 1 and
> just return.

CPU 0                              CPU 1
-----------------------------------------------------------------------------

v4l2_clk_enable()
atomic_inc_return() returns 1

                                   v4l2_clk_enable()
                                   atomic_inc_return() returns 2
                                   returns 0 to caller, clock is not enabled
                                   caller uses the clock and fails

clk->ops->enable()

This could also happen on a non-SMP system if the first task is interrupted 
between the atomic_inc_return() and clk->ops->enable() calls.

> > One solution would be to add a spinlock to struct v4l2_clk and modify the
> > enable field from atomic_t to plain unsigned int.
> > 
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(v4l2_clk_enable);
> > > +
> > > +void v4l2_clk_disable(struct v4l2_clk *clk)
> > > +{
> > > +	int enable = atomic_dec_return(&clk->enable);
> > > +
> > > +	if (WARN(enable < 0, "Unbalanced %s()!\n", __func__)) {
> > 
> > Could you add the clock name to the debug message ?
> 
> You mean device / connection IDs? Could do, yes.

Yes.

> > > +		atomic_inc(&clk->enable);
> > > +		return;
> > > +	}
> > > +
> > > +	if (!enable && clk->ops->disable)
> > > +		clk->ops->disable(clk);
> > > +}
> > > +EXPORT_SYMBOL(v4l2_clk_disable);

[snip]

> > > +void v4l2_clk_unregister(struct v4l2_clk *clk)
> > > +{
> > > +	WARN(atomic_read(&clk->enable),
> > > +	     "Unregistering still enabled %s:%s clock!\n", clk->dev_id,
> > > clk->id);
> > 
> > Shouldn't this warning be based on a get/put refcount instead of an enable
> > refcount ?
> 
> In principle - yes, so, you vote for one more counter?...

Either one more counter, or dropping the warning.

> > I would also turn it into a BUG_ON. The kernel will crash later anyway
> > when the clock user will try to disable the clock, as you free the clock
> > object here.
> 
> s/when/if/ ;-) With BUG_ON() you, probably, only get one stack dump here,
> with WARN() you get both - one with the _unregister() stack and one with
> the _disable() and / or _put() stack... Don't you think the latter option
> is more informative?

What bothers me is that the later disable/put might not crash immediately but 
could lead to memory corruption with potential severe consequences if the 
memory has been reallocated.

> > > +	mutex_lock(&clk_lock);
> > > +	list_del(&clk->list);
> > > +	mutex_unlock(&clk_lock);
> > > +
> > > +	kfree(clk->id);
> > > +	kfree(clk->dev_id);
> > > +	kfree(clk);

Laurent Pinchart Nov. 12, 2012, 11:06 a.m. UTC | #5

Hi Sakari,

On Monday 12 November 2012 00:33:17 Sakari Ailus wrote:
> On Wed, Oct 31, 2012 at 02:02:54PM +0100, Guennadi Liakhovetski wrote:
> > On Wed, 31 Oct 2012, Laurent Pinchart wrote:
> ...
> 
> > > > +#include <linux/atomic.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/string.h>
> > > > +
> > > > +#include <media/v4l2-clk.h>
> > > > +#include <media/v4l2-subdev.h>
> > > > +
> > > > +static DEFINE_MUTEX(clk_lock);
> > > > +static LIST_HEAD(v4l2_clk);
> > > 
> > > As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> > 
> > Don't you think naming of a static variable isn't important enough? ;-) I
> > think code authors should have enough freedom to at least pick up single
> > vs. plural form:-) "clks" is too many consonants for my taste, if it were
> > anything important I'd rather agree to "clk_head" or "clk_list" or
> > something similar.
> 
> clk_list makes sense IMO since the clk_ prefis is the same.
> 
> ...
> 
> > > > +void v4l2_clk_put(struct v4l2_clk *clk)
> > > > +{
> > > > +	if (!IS_ERR(clk))
> > > > +		module_put(clk->ops->owner);
> > > > +}
> > > > +EXPORT_SYMBOL(v4l2_clk_put);
> > > > +
> > > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > > +{
> > > > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > > > +		int ret = clk->ops->enable(clk);
> > > > +		if (ret < 0)
> > > > +			atomic_dec(&clk->enable);
> > > > +		return ret;
> > > > +	}
> > > 
> > > I think you need a spinlock here instead of atomic operations. You could
> > > get preempted after atomic_inc_return() and before clk->ops->enable()
> > > by another process that would call v4l2_clk_enable(). The function
> > > would return with enabling the clock.
> > 
> > Sorry, what's the problem then? "Our" instance will succeed and call
> > ->enable() and the preempting instance will see the enable count > 1 and
> > just return.
> 
> The clock is guaranteed to be enabled only after the call has returned. The
> second caller of v4lw_clk_enable() thus may proceed without the clock being
> enabled.
> 
> In principle enable() might also want to sleep, so how about using a mutex
> for the purpose instead of a spinlock?

If enable() needs to sleep we should split the enable call into prepare and 
enable, like the common clock framework did.

I'm fine with both implementing this now or later when we will have an enable 
call that requires sleeping.

> ...
> 
> > > > +struct v4l2_clk_ops {
> > > > +	struct module	*owner;
> > > > +	int		(*enable)(struct v4l2_clk *clk);
> > > > +	void		(*disable)(struct v4l2_clk *clk);
> > > > +	unsigned long	(*get_rate)(struct v4l2_clk *clk);
> > > > +	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
> 
> How about unsigned long hz?

Sakari Ailus Nov. 12, 2012, 11:37 p.m. UTC | #6

Hi Laurent,

On Mon, Nov 12, 2012 at 12:06:50PM +0100, Laurent Pinchart wrote:
> On Monday 12 November 2012 00:33:17 Sakari Ailus wrote:
> > On Wed, Oct 31, 2012 at 02:02:54PM +0100, Guennadi Liakhovetski wrote:
> > > On Wed, 31 Oct 2012, Laurent Pinchart wrote:
> > ...
> > 
> > > > > +#include <linux/atomic.h>
> > > > > +#include <linux/errno.h>
> > > > > +#include <linux/list.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/mutex.h>
> > > > > +#include <linux/string.h>
> > > > > +
> > > > > +#include <media/v4l2-clk.h>
> > > > > +#include <media/v4l2-subdev.h>
> > > > > +
> > > > > +static DEFINE_MUTEX(clk_lock);
> > > > > +static LIST_HEAD(v4l2_clk);
> > > > 
> > > > As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> > > 
> > > Don't you think naming of a static variable isn't important enough? ;-) I
> > > think code authors should have enough freedom to at least pick up single
> > > vs. plural form:-) "clks" is too many consonants for my taste, if it were
> > > anything important I'd rather agree to "clk_head" or "clk_list" or
> > > something similar.
> > 
> > clk_list makes sense IMO since the clk_ prefis is the same.
> > 
> > ...
> > 
> > > > > +void v4l2_clk_put(struct v4l2_clk *clk)
> > > > > +{
> > > > > +	if (!IS_ERR(clk))
> > > > > +		module_put(clk->ops->owner);
> > > > > +}
> > > > > +EXPORT_SYMBOL(v4l2_clk_put);
> > > > > +
> > > > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > > > +{
> > > > > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > > > > +		int ret = clk->ops->enable(clk);
> > > > > +		if (ret < 0)
> > > > > +			atomic_dec(&clk->enable);
> > > > > +		return ret;
> > > > > +	}
> > > > 
> > > > I think you need a spinlock here instead of atomic operations. You could
> > > > get preempted after atomic_inc_return() and before clk->ops->enable()
> > > > by another process that would call v4l2_clk_enable(). The function
> > > > would return with enabling the clock.
> > > 
> > > Sorry, what's the problem then? "Our" instance will succeed and call
> > > ->enable() and the preempting instance will see the enable count > 1 and
> > > just return.
> > 
> > The clock is guaranteed to be enabled only after the call has returned. The
> > second caller of v4lw_clk_enable() thus may proceed without the clock being
> > enabled.
> > 
> > In principle enable() might also want to sleep, so how about using a mutex
> > for the purpose instead of a spinlock?
> 
> If enable() needs to sleep we should split the enable call into prepare and 
> enable, like the common clock framework did.

I'm pretty sure we won't need to toggle this from interrupt context which is
what the clock framework does, AFAIU. Accessing i2c subdevs mandates
sleeping already.

We might not need to have a mutex either if no driver needs to sleep for
this, still I guess this is more likely. I'm ok with both; just thought to
mention this.

Laurent Pinchart Nov. 14, 2012, 1:06 p.m. UTC | #7

Hi Sakari,

On Tuesday 13 November 2012 01:37:51 Sakari Ailus wrote:
> On Mon, Nov 12, 2012 at 12:06:50PM +0100, Laurent Pinchart wrote:
> > On Monday 12 November 2012 00:33:17 Sakari Ailus wrote:
> > > On Wed, Oct 31, 2012 at 02:02:54PM +0100, Guennadi Liakhovetski wrote:
> > > > On Wed, 31 Oct 2012, Laurent Pinchart wrote:
> > > ...
> > > 
> > > > > > +#include <linux/atomic.h>
> > > > > > +#include <linux/errno.h>
> > > > > > +#include <linux/list.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/mutex.h>
> > > > > > +#include <linux/string.h>
> > > > > > +
> > > > > > +#include <media/v4l2-clk.h>
> > > > > > +#include <media/v4l2-subdev.h>
> > > > > > +
> > > > > > +static DEFINE_MUTEX(clk_lock);
> > > > > > +static LIST_HEAD(v4l2_clk);
> > > > > 
> > > > > As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> > > > 
> > > > Don't you think naming of a static variable isn't important enough?
> > > > ;-) I think code authors should have enough freedom to at least pick
> > > > up single vs. plural form:-) "clks" is too many consonants for my
> > > > taste, if it were anything important I'd rather agree to "clk_head" or
> > > > "clk_list" or something similar.
> > > 
> > > clk_list makes sense IMO since the clk_ prefis is the same.
> > > 
> > > ...
> > > 
> > > > > > +void v4l2_clk_put(struct v4l2_clk *clk)
> > > > > > +{
> > > > > > +	if (!IS_ERR(clk))
> > > > > > +		module_put(clk->ops->owner);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(v4l2_clk_put);
> > > > > > +
> > > > > > +int v4l2_clk_enable(struct v4l2_clk *clk)
> > > > > > +{
> > > > > > +	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
> > > > > > +		int ret = clk->ops->enable(clk);
> > > > > > +		if (ret < 0)
> > > > > > +			atomic_dec(&clk->enable);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > I think you need a spinlock here instead of atomic operations. You
> > > > > could get preempted after atomic_inc_return() and before
> > > > > clk->ops->enable() by another process that would call
> > > > > v4l2_clk_enable(). The function would return with enabling the
> > > > > clock.
> > > > 
> > > > Sorry, what's the problem then? "Our" instance will succeed and call
> > > > ->enable() and the preempting instance will see the enable count > 1
> > > > and just return.
> > > 
> > > The clock is guaranteed to be enabled only after the call has returned.
> > > The second caller of v4lw_clk_enable() thus may proceed without the
> > > clock being enabled.
> > > 
> > > In principle enable() might also want to sleep, so how about using a
> > > mutex for the purpose instead of a spinlock?
> > 
> > If enable() needs to sleep we should split the enable call into prepare
> > and enable, like the common clock framework did.
> 
> I'm pretty sure we won't need to toggle this from interrupt context which is
> what the clock framework does, AFAIU. Accessing i2c subdevs mandates
> sleeping already.
> 
> We might not need to have a mutex either if no driver needs to sleep for
> this, still I guess this is more likely. I'm ok with both; just thought to
> mention this.

Right, I'm fine with a mutex for now, we'll split enable into enable and 
prepare later if needed.

Sylwester Nawrocki Nov. 22, 2012, 10:52 p.m. UTC | #8

Hi All,

On 11/14/2012 02:06 PM, Laurent Pinchart wrote:
...
>>>>>>> +
>>>>>>> +static DEFINE_MUTEX(clk_lock);
>>>>>>> +static LIST_HEAD(v4l2_clk);
>>>>>>
>>>>>> As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
>>>>>
>>>>> Don't you think naming of a static variable isn't important enough?
>>>>> ;-) I think code authors should have enough freedom to at least pick
>>>>> up single vs. plural form:-) "clks" is too many consonants for my
>>>>> taste, if it were anything important I'd rather agree to "clk_head" or
>>>>> "clk_list" or something similar.
>>>>
>>>> clk_list makes sense IMO since the clk_ prefis is the same.

FWIW, clk_list looks fine for me as well.

>>>>>>> +void v4l2_clk_put(struct v4l2_clk *clk)
>>>>>>> +{
>>>>>>> +	if (!IS_ERR(clk))
>>>>>>> +		module_put(clk->ops->owner);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(v4l2_clk_put);
>>>>>>> +
>>>>>>> +int v4l2_clk_enable(struct v4l2_clk *clk)
>>>>>>> +{
>>>>>>> +	if (atomic_inc_return(&clk->enable) == 1&&  clk->ops->enable) {
>>>>>>> +		int ret = clk->ops->enable(clk);
>>>>>>> +		if (ret<  0)
>>>>>>> +			atomic_dec(&clk->enable);
>>>>>>> +		return ret;
>>>>>>> +	}
>>>>>>
>>>>>> I think you need a spinlock here instead of atomic operations. You
>>>>>> could get preempted after atomic_inc_return() and before
>>>>>> clk->ops->enable() by another process that would call
>>>>>> v4l2_clk_enable(). The function would return with enabling the
>>>>>> clock.
>>>>>
>>>>> Sorry, what's the problem then? "Our" instance will succeed and call
>>>>> ->enable() and the preempting instance will see the enable count>  1
>>>>> and just return.
>>>>
>>>> The clock is guaranteed to be enabled only after the call has returned.
>>>> The second caller of v4lw_clk_enable() thus may proceed without the
>>>> clock being enabled.
>>>>
>>>> In principle enable() might also want to sleep, so how about using a
>>>> mutex for the purpose instead of a spinlock?
>>>
>>> If enable() needs to sleep we should split the enable call into prepare
>>> and enable, like the common clock framework did.
>>
>> I'm pretty sure we won't need to toggle this from interrupt context which is
>> what the clock framework does, AFAIU. Accessing i2c subdevs mandates
>> sleeping already.
>>
>> We might not need to have a mutex either if no driver needs to sleep for
>> this, still I guess this is more likely. I'm ok with both; just thought to
>> mention this.
>
> Right, I'm fine with a mutex for now, we'll split enable into enable and
> prepare later if needed.

How about just dropping reference counting from this code entirely ?
What would be use cases for multiple users of a single clock ? E.g. multiple
sensors case where each one uses same clock provided by a host interface ?
If we allow the sensor subdev drivers to be setting the clock frequency and
each sensor uses different frequency, then I can't see how this can work
reliably. I mean it's the clock's provider that should coordinate and
reference count the clock users. If a clock is enabled for one sensor and
some other sensor is attempting to set different frequency then the 
set_rate
callback should return an error. The clock provider will need use 
internally
a lock for the clock anyway, and to track the clock reference count too.
So I'm inclined to leave all this refcounting bits out to individual clock
providers.

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

Sakari Ailus Nov. 25, 2012, 11:04 a.m. UTC | #9

Hi Sylwester,

Sylwester Nawrocki wrote:
> Hi All,
> 
> On 11/14/2012 02:06 PM, Laurent Pinchart wrote:
> ...
>>>>>>>> +
>>>>>>>> +static DEFINE_MUTEX(clk_lock);
>>>>>>>> +static LIST_HEAD(v4l2_clk);
>>>>>>>
>>>>>>> As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
>>>>>>
>>>>>> Don't you think naming of a static variable isn't important enough?
>>>>>> ;-) I think code authors should have enough freedom to at least pick
>>>>>> up single vs. plural form:-) "clks" is too many consonants for my
>>>>>> taste, if it were anything important I'd rather agree to
>>>>>> "clk_head" or
>>>>>> "clk_list" or something similar.
>>>>>
>>>>> clk_list makes sense IMO since the clk_ prefis is the same.
> 
> FWIW, clk_list looks fine for me as well.
> 
>>>>>>>> +void v4l2_clk_put(struct v4l2_clk *clk)
>>>>>>>> +{
>>>>>>>> +    if (!IS_ERR(clk))
>>>>>>>> +        module_put(clk->ops->owner);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(v4l2_clk_put);
>>>>>>>> +
>>>>>>>> +int v4l2_clk_enable(struct v4l2_clk *clk)
>>>>>>>> +{
>>>>>>>> +    if (atomic_inc_return(&clk->enable) == 1&& 
>>>>>>>> clk->ops->enable) {
>>>>>>>> +        int ret = clk->ops->enable(clk);
>>>>>>>> +        if (ret<  0)
>>>>>>>> +            atomic_dec(&clk->enable);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>
>>>>>>> I think you need a spinlock here instead of atomic operations. You
>>>>>>> could get preempted after atomic_inc_return() and before
>>>>>>> clk->ops->enable() by another process that would call
>>>>>>> v4l2_clk_enable(). The function would return with enabling the
>>>>>>> clock.
>>>>>>
>>>>>> Sorry, what's the problem then? "Our" instance will succeed and call
>>>>>> ->enable() and the preempting instance will see the enable count>  1
>>>>>> and just return.
>>>>>
>>>>> The clock is guaranteed to be enabled only after the call has
>>>>> returned.
>>>>> The second caller of v4lw_clk_enable() thus may proceed without the
>>>>> clock being enabled.
>>>>>
>>>>> In principle enable() might also want to sleep, so how about using a
>>>>> mutex for the purpose instead of a spinlock?
>>>>
>>>> If enable() needs to sleep we should split the enable call into prepare
>>>> and enable, like the common clock framework did.
>>>
>>> I'm pretty sure we won't need to toggle this from interrupt context
>>> which is
>>> what the clock framework does, AFAIU. Accessing i2c subdevs mandates
>>> sleeping already.
>>>
>>> We might not need to have a mutex either if no driver needs to sleep for
>>> this, still I guess this is more likely. I'm ok with both; just
>>> thought to
>>> mention this.
>>
>> Right, I'm fine with a mutex for now, we'll split enable into enable and
>> prepare later if needed.
> 
> How about just dropping reference counting from this code entirely ?
> What would be use cases for multiple users of a single clock ? E.g.
> multiple
> sensors case where each one uses same clock provided by a host interface ?
> If we allow the sensor subdev drivers to be setting the clock frequency and
> each sensor uses different frequency, then I can't see how this can work
> reliably. I mean it's the clock's provider that should coordinate and
> reference count the clock users. If a clock is enabled for one sensor and
> some other sensor is attempting to set different frequency then the
> set_rate
> callback should return an error. The clock provider will need use
> internally
> a lock for the clock anyway, and to track the clock reference count too.
> So I'm inclined to leave all this refcounting bits out to individual clock
> providers.

The common clock framework achieves this through notifiers. That'd be
probably overkill in this case.

What comes to the implementation now, would it be enough if changing the
clock rate would work once after the clock first had users, with this
capability renewed once all users are gone?

I wonder if enabling the clock should be allowed at all if the rate
hasn't been explicitly set. I don't know of a sensor driver which would
be able to use a non-predefined clock frequency.

Laurent Pinchart Nov. 27, 2012, 2:18 p.m. UTC | #10

Hello,

On Sunday 25 November 2012 13:04:09 Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> > On 11/14/2012 02:06 PM, Laurent Pinchart wrote:
> > ...
> > 
> >>>>>>>> +
> >>>>>>>> +static DEFINE_MUTEX(clk_lock);
> >>>>>>>> +static LIST_HEAD(v4l2_clk);
> >>>>>>> 
> >>>>>>> As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
> >>>>>> 
> >>>>>> Don't you think naming of a static variable isn't important enough?
> >>>>>> ;-) I think code authors should have enough freedom to at least pick
> >>>>>> up single vs. plural form:-) "clks" is too many consonants for my
> >>>>>> taste, if it were anything important I'd rather agree to
> >>>>>> "clk_head" or
> >>>>>> "clk_list" or something similar.
> >>>>> 
> >>>>> clk_list makes sense IMO since the clk_ prefis is the same.
> > 
> > FWIW, clk_list looks fine for me as well.
> > 
> >>>>>>>> +void v4l2_clk_put(struct v4l2_clk *clk)
> >>>>>>>> +{
> >>>>>>>> +    if (!IS_ERR(clk))
> >>>>>>>> +        module_put(clk->ops->owner);
> >>>>>>>> +}
> >>>>>>>> +EXPORT_SYMBOL(v4l2_clk_put);
> >>>>>>>> +
> >>>>>>>> +int v4l2_clk_enable(struct v4l2_clk *clk)
> >>>>>>>> +{
> >>>>>>>> +    if (atomic_inc_return(&clk->enable) == 1&&
> >>>>>>>> clk->ops->enable) {
> >>>>>>>> +        int ret = clk->ops->enable(clk);
> >>>>>>>> +        if (ret<  0)
> >>>>>>>> +            atomic_dec(&clk->enable);
> >>>>>>>> +        return ret;
> >>>>>>>> +    }
> >>>>>>> 
> >>>>>>> I think you need a spinlock here instead of atomic operations. You
> >>>>>>> could get preempted after atomic_inc_return() and before
> >>>>>>> clk->ops->enable() by another process that would call
> >>>>>>> v4l2_clk_enable(). The function would return with enabling the
> >>>>>>> clock.
> >>>>>> 
> >>>>>> Sorry, what's the problem then? "Our" instance will succeed and call
> >>>>>> ->enable() and the preempting instance will see the enable count>  1
> >>>>>> and just return.
> >>>>> 
> >>>>> The clock is guaranteed to be enabled only after the call has
> >>>>> returned. The second caller of v4lw_clk_enable() thus may proceed
> >>>>> without the clock being enabled.
> >>>>> 
> >>>>> In principle enable() might also want to sleep, so how about using a
> >>>>> mutex for the purpose instead of a spinlock?
> >>>> 
> >>>> If enable() needs to sleep we should split the enable call into prepare
> >>>> and enable, like the common clock framework did.
> >>> 
> >>> I'm pretty sure we won't need to toggle this from interrupt context
> >>> which is what the clock framework does, AFAIU. Accessing i2c subdevs
> >>> mandates sleeping already.
> >>> 
> >>> We might not need to have a mutex either if no driver needs to sleep for
> >>> this, still I guess this is more likely. I'm ok with both; just
> >>> thought to mention this.
> >> 
> >> Right, I'm fine with a mutex for now, we'll split enable into enable and
> >> prepare later if needed.
> > 
> > How about just dropping reference counting from this code entirely ?
> > What would be use cases for multiple users of a single clock ? E.g.
> > multiple sensors case where each one uses same clock provided by a host
> > interface ? If we allow the sensor subdev drivers to be setting the clock
> > frequency and each sensor uses different frequency, then I can't see how
> > this can work reliably. I mean it's the clock's provider that should
> > coordinate and reference count the clock users. If a clock is enabled for
> > one sensor and some other sensor is attempting to set different frequency
> > then the set_rate callback should return an error. The clock provider will
> > need use internally a lock for the clock anyway, and to track the clock
> > reference count too. So I'm inclined to leave all this refcounting bits
> > out to individual clock providers.
> 
> The common clock framework achieves this through notifiers. That'd be
> probably overkill in this case.
> 
> What comes to the implementation now, would it be enough if changing the
> clock rate would work once after the clock first had users, with this
> capability renewed once all users are gone?
> 
> I wonder if enabling the clock should be allowed at all if the rate
> hasn't been explicitly set. I don't know of a sensor driver which would
> be able to use a non-predefined clock frequency.

OK, maybe we should try to refocus here.

The purpose of the V4L2 clock helpers is to get rid of clock-related board 
callbacks (or other direct clock provider-consumer dependencies) without 
waiting for the common clock framework to be available on a particular 
platform. With that in mind, the following problems need to be solved.

- Multiple consumers for a single clock

A video sensor and the associated lens and/or flash controllers could share 
the same input clock. How to arbitrate rate requests from the individual 
consumers is still an open problem, but that's true for the common clock 
framework too. I wouldn't vote against requiring the common clock framework 
for this use case. This would mean that most of the locking and reference 
counting could (but doesn't have to) be dropped from the V4L2 clock helpers 
(reference counting can still be useful for debugging purposes).

- Clock provider/consumer outside of V4L2

If the clock provider isn't a V4L2 device, I think the common clock framework 
must be used by the consumer to access the clock. Similarly, if the provider 
is a V4L2 device and the consumer isn't, the common clock framework must be 
used as well (although I don't think this case will happen in practice, I 
can't really imagine a USB transceiver using the ISP clock for instance).

- Common clock framework fallback

This is a new item. Sensor (or other clock consumer) drivers can't tell 
whether the clock provider exposes its clock(s) through the common clock 
framework or through the V4L2 clock helpers. ISP (or other clock provider) 
drivers, on the other hand, are tied to a platform (or a very limited set of 
platforms) and use the common clock framework if available, or the V4L2 clock 
helpers otherwise. During the transition clock consumers will need to support 
both. For that reason I believe that the V4L2 clock helpers should retrieve 
the requested clock through the common clock framework first, and then look at 
the V4L2 clocks if the clock wasn't available from the common clock framework.

Patch

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 00f64d6..cb5fede 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -5,7 +5,7 @@ 
 tuner-objs	:=	tuner-core.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
-			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
+			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
 ifeq ($(CONFIG_COMPAT),y)
   videodev-objs += v4l2-compat-ioctl32.o
 endif
diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
new file mode 100644
index 0000000..2496807
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -0,0 +1,169 @@ 
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/atomic.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+
+#include <media/v4l2-clk.h>
+#include <media/v4l2-subdev.h>
+
+static DEFINE_MUTEX(clk_lock);
+static LIST_HEAD(v4l2_clk);
+
+static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
+{
+	struct v4l2_clk *clk;
+
+	list_for_each_entry(clk, &v4l2_clk, list) {
+		if (strcmp(dev_id, clk->dev_id))
+			continue;
+
+		if (!id || !clk->id || !strcmp(clk->id, id))
+			return clk;
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id)
+{
+	struct v4l2_clk *clk;
+
+	mutex_lock(&clk_lock);
+	clk = v4l2_clk_find(sd->name, id);
+
+	if (!IS_ERR(clk) && !try_module_get(clk->ops->owner))
+		clk = ERR_PTR(-ENODEV);
+	mutex_unlock(&clk_lock);
+
+	return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_get);
+
+void v4l2_clk_put(struct v4l2_clk *clk)
+{
+	if (!IS_ERR(clk))
+		module_put(clk->ops->owner);
+}
+EXPORT_SYMBOL(v4l2_clk_put);
+
+int v4l2_clk_enable(struct v4l2_clk *clk)
+{
+	if (atomic_inc_return(&clk->enable) == 1 && clk->ops->enable) {
+		int ret = clk->ops->enable(clk);
+		if (ret < 0)
+			atomic_dec(&clk->enable);
+		return ret;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_clk_enable);
+
+void v4l2_clk_disable(struct v4l2_clk *clk)
+{
+	int enable = atomic_dec_return(&clk->enable);
+
+	if (WARN(enable < 0, "Unbalanced %s()!\n", __func__)) {
+		atomic_inc(&clk->enable);
+		return;
+	}
+
+	if (!enable && clk->ops->disable)
+		clk->ops->disable(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_disable);
+
+unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
+{
+	if (!atomic_read(&clk->enable))
+		return -EINVAL;
+
+	if (!clk->ops->get_rate)
+		return -ENOSYS;
+
+	return clk->ops->get_rate(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_get_rate);
+
+int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
+{
+	if (!atomic_read(&clk->enable))
+		return -EINVAL;
+
+	if (!clk->ops->set_rate)
+		return -ENOSYS;
+
+	return clk->ops->set_rate(clk, rate);
+}
+EXPORT_SYMBOL(v4l2_clk_set_rate);
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+				   const char *dev_id,
+				   const char *id, void *priv)
+{
+	struct v4l2_clk *clk;
+
+	if (!ops || !dev_id)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&clk_lock);
+	clk = v4l2_clk_find(dev_id, id);
+
+	if (!IS_ERR(clk)) {
+		clk = ERR_PTR(-EEXIST);
+		goto out;
+	}
+
+	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+	if (!clk) {
+		clk = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
+	clk->ops = ops;
+	clk->id = kstrdup(id, GFP_KERNEL);
+	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
+	clk->priv = priv;
+	atomic_set(&clk->enable, 0);
+
+	list_add_tail(&clk->list, &v4l2_clk);
+out:
+	if (!IS_ERR(clk) && ((id && !clk->id) || !clk->dev_id)) {
+		list_del(&clk->list);
+		kfree(clk->id);
+		kfree(clk->dev_id);
+		kfree(clk);
+		clk = ERR_PTR(-ENOMEM);
+	}
+
+	mutex_unlock(&clk_lock);
+
+	return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_register);
+
+void v4l2_clk_unregister(struct v4l2_clk *clk)
+{
+	WARN(atomic_read(&clk->enable),
+	     "Unregistering still enabled %s:%s clock!\n", clk->dev_id, clk->id);
+
+	mutex_lock(&clk_lock);
+	list_del(&clk->list);
+	mutex_unlock(&clk_lock);
+
+	kfree(clk->id);
+	kfree(clk->dev_id);
+	kfree(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_unregister);
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
new file mode 100644
index 0000000..f70664b
--- /dev/null
+++ b/include/media/v4l2-clk.h
@@ -0,0 +1,51 @@ 
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * ATTENTION: This is a temporary API and it shall be replaced by the generic
+ * clock API, when the latter becomes widely available.
+ */
+
+#ifndef MEDIA_V4L2_CLK_H
+#define MEDIA_V4L2_CLK_H
+
+#include <linux/atomic.h>
+#include <linux/list.h>
+
+struct module;
+struct v4l2_subdev;
+
+struct v4l2_clk {
+	struct list_head list;
+	const struct v4l2_clk_ops *ops;
+	char *dev_id;
+	const char *id;
+	atomic_t enable;
+	void *priv;
+};
+
+struct v4l2_clk_ops {
+	struct module	*owner;
+	int		(*enable)(struct v4l2_clk *clk);
+	void		(*disable)(struct v4l2_clk *clk);
+	unsigned long	(*get_rate)(struct v4l2_clk *clk);
+	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
+};
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+				   const char *dev_name,
+				   const char *name, void *priv);
+void v4l2_clk_unregister(struct v4l2_clk *clk);
+struct v4l2_clk *v4l2_clk_get(struct v4l2_subdev *sd, const char *id);
+void v4l2_clk_put(struct v4l2_clk *clk);
+int v4l2_clk_enable(struct v4l2_clk *clk);
+void v4l2_clk_disable(struct v4l2_clk *clk);
+unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
+int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
+
+#endif