Re: [PATCH v5 18/35] v4l: Allow changing control handler lock

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Sakari,

On 03/06/2012 05:32 PM, Sakari Ailus wrote:
> Allow choosing the lock used by the control handler. This may be handy
> sometimes when a driver providing multiple subdevs does not want to use
> several locks to serialise its functions.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/adp1653.c    |    8 +++---
>  drivers/media/video/v4l2-ctrls.c |   39 +++++++++++++++++++------------------
>  drivers/media/video/vivi.c       |    4 +-
>  include/media/v4l2-ctrls.h       |    9 +++++--
>  4 files changed, 32 insertions(+), 28 deletions(-)
...
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 533315b..71abac0 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -168,7 +168,9 @@ struct v4l2_ctrl_ref {
>  /** struct v4l2_ctrl_handler - The control handler keeps track of all the
>    * controls: both the controls owned by the handler and those inherited
>    * from other handlers.
> +  * @_lock:	Default for "lock".
>    * @lock:	Lock to control access to this handler and its controls.
> +  *		May be replaced by the user right after init.
>    * @ctrls:	The list of controls owned by this handler.
>    * @ctrl_refs:	The list of control references.
>    * @cached:	The last found control reference. It is common that the same
> @@ -179,7 +181,8 @@ struct v4l2_ctrl_ref {
>    * @error:	The error code of the first failed control addition.
>    */
>  struct v4l2_ctrl_handler {
> -	struct mutex lock;
> +	struct mutex _lock;
> +	struct mutex *lock;

I think we have an issue here. All drivers that reference ctrl_handler.lock
directly need to be updated, since the 'lock' member of struct v4l2_ctrl_handler
is no a pointer. So insted 

mutex_lock(&ctrl_handler.lock);

they should now do

mutex_lock(ctrl_handler.lock);

Or am I missing something ?

For example, I'm getting following error:

drivers/media/video/s5p-fimc/fimc-core.c: In function ‘fimc_ctrls_activate’:
drivers/media/video/s5p-fimc/fimc-core.c:678: warning: passing argument 1 of ‘mutex_lock’ from incompatible pointer type
include/linux/mutex.h:152: note: expected ‘struct mutex *’ but argument is of type ‘struct mutex **’
drivers/media/video/s5p-fimc/fimc-core.c:697: warning: passing argument 1 of ‘mutex_unlock’ from incompatible pointer type
include/linux/mutex.h:169: note: expected ‘struct mutex *’ but argument is of type ‘struct mutex **’

AFAICT only vivi and s5p-fimc drivers use the control handler lock 
directly, so I can prepare a patch updating those drivers.

>  	struct list_head ctrls;
>  	struct list_head ctrl_refs;
>  	struct v4l2_ctrl_ref *cached;
> @@ -456,7 +459,7 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
>    */
>  static inline void v4l2_ctrl_lock(struct v4l2_ctrl *ctrl)
>  {
> -	mutex_lock(&ctrl->handler->lock);
> +	mutex_lock(ctrl->handler->lock);
>  }
>  
>  /** v4l2_ctrl_lock() - Helper function to unlock the handler
> @@ -465,7 +468,7 @@ static inline void v4l2_ctrl_lock(struct v4l2_ctrl *ctrl)
>    */
>  static inline void v4l2_ctrl_unlock(struct v4l2_ctrl *ctrl)
>  {
> -	mutex_unlock(&ctrl->handler->lock);
> +	mutex_unlock(ctrl->handler->lock);
>  }
>  
>  /** v4l2_ctrl_g_ctrl() - Helper function to get the control's value from within a driver.

--

Regards,
Sylwester