diff mbox series

media: v4l: subdev: Move out subdev state lock macros outside CONFIG_MEDIA_CONTROLLER

Message ID 20231208-v4l2-state-mc-fix-v1-1-a0c8162557c6@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: v4l: subdev: Move out subdev state lock macros outside CONFIG_MEDIA_CONTROLLER | expand

Commit Message

Tomi Valkeinen Dec. 8, 2023, 4:15 p.m. UTC 
The subdev state locking macros and macros to get the active state are
currently behind CONFIG_MEDIA_CONTROLLER. This makes sense, as there can
be no subdev state without MC.

However, we have code paths common to MC and non-MC cases which call
subdev operations that have subdev state as a parameter. In the non-MC
case the state parameter would always be NULL.

Thus it makes sense to allow, e.g.:

v4l2_subdev_call_state_active(sd, pad, get_fmt, fmt)

which for non-MC case would call the subdev passing NULL as the state.

This currently fails:
https://lore.kernel.org/oe-kbuild-all/202312061101.PLrz5NnJ-lkp@intel.com/

Fix the issue by moving the related macros to be outside
CONFIG_MEDIA_CONTROLLER. The v4l2_subdev_lock_state() and
v4l2_subdev_unlock_state() macros will crash if given NULL as the state,
but the other macros behave correctly even when there's no active state,
and they will only call the lock/unlock macros if there is a state.

An alternative fix would be to make another version of
v4l2_subdev_call_state_try() with ifdefs, which would not use any state
macros and would always pass NULL as the state. But having two version
of a macro/function is always more confusing than having just one, so I
went this way.

So, this fixes the v4l2_subdev_call_state_active() macro. But we also
have v4l2_subdev_call_state_try(). It would be possible to fix that
macro by additionally creating "no-op" variants of the state alloc and
free functions. However, v4l2_subdev_call_state_try() is only used by a
single driver (stm32-dcmi), which selects MC, and the macro is supposed
to be removed as soon as the users have been converted away from the
macro. Thus I have not touched the state alloc/free functions, and I
think it makes sense to keep alloc/free functions available only if
there's actually something that can be allocated or freed.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 include/media/v4l2-subdev.h | 166 ++++++++++++++++++++++----------------------
 1 file changed, 83 insertions(+), 83 deletions(-)


---
base-commit: e55a9482888da73eeadde5f13ef8bafce68a38ed
change-id: 20231208-v4l2-state-mc-fix-b4f0ebfbb2bf

Best regards,

Comments

Laurent Pinchart Dec. 8, 2023, 4:33 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Fri, Dec 08, 2023 at 06:15:15PM +0200, Tomi Valkeinen wrote:
> The subdev state locking macros and macros to get the active state are
> currently behind CONFIG_MEDIA_CONTROLLER. This makes sense, as there can
> be no subdev state without MC.
> 
> However, we have code paths common to MC and non-MC cases which call
> subdev operations that have subdev state as a parameter. In the non-MC
> case the state parameter would always be NULL.
> 
> Thus it makes sense to allow, e.g.:
> 
> v4l2_subdev_call_state_active(sd, pad, get_fmt, fmt)
> 
> which for non-MC case would call the subdev passing NULL as the state.
> 
> This currently fails:
> https://lore.kernel.org/oe-kbuild-all/202312061101.PLrz5NnJ-lkp@intel.com/
> 
> Fix the issue by moving the related macros to be outside
> CONFIG_MEDIA_CONTROLLER. The v4l2_subdev_lock_state() and
> v4l2_subdev_unlock_state() macros will crash if given NULL as the state,
> but the other macros behave correctly even when there's no active state,
> and they will only call the lock/unlock macros if there is a state.
> 
> An alternative fix would be to make another version of
> v4l2_subdev_call_state_try() with ifdefs, which would not use any state
> macros and would always pass NULL as the state. But having two version
> of a macro/function is always more confusing than having just one, so I
> went this way.
> 
> So, this fixes the v4l2_subdev_call_state_active() macro. But we also
> have v4l2_subdev_call_state_try(). It would be possible to fix that
> macro by additionally creating "no-op" variants of the state alloc and
> free functions. However, v4l2_subdev_call_state_try() is only used by a
> single driver (stm32-dcmi), which selects MC, and the macro is supposed
> to be removed as soon as the users have been converted away from the
> macro. Thus I have not touched the state alloc/free functions, and I
> think it makes sense to keep alloc/free functions available only if
> there's actually something that can be allocated or freed.

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  include/media/v4l2-subdev.h | 166 ++++++++++++++++++++++----------------------
>  1 file changed, 83 insertions(+), 83 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8b08f6640dee..61024a74ce38 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1311,89 +1311,6 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>   */
>  void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
>  
> -/**
> - * v4l2_subdev_lock_state() - Locks the subdev state
> - * @state: The subdevice state
> - *
> - * Locks the given subdev state.
> - *
> - * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> - */
> -static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> -{
> -	mutex_lock(state->lock);
> -}
> -
> -/**
> - * v4l2_subdev_unlock_state() - Unlocks the subdev state
> - * @state: The subdevice state
> - *
> - * Unlocks the given subdev state.
> - */
> -static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> -{
> -	mutex_unlock(state->lock);
> -}
> -
> -/**
> - * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
> - *					     is unlocked and returns it
> - * @sd: The subdevice
> - *
> - * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state. If the state is not NULL, calls
> - * lockdep_assert_not_held() to issue a warning if the state is locked.
> - *
> - * This function is to be used e.g. when getting the active state for the sole
> - * purpose of passing it forward, without accessing the state fields.
> - */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_get_unlocked_active_state(struct v4l2_subdev *sd)
> -{
> -	if (sd->active_state)
> -		lockdep_assert_not_held(sd->active_state->lock);
> -	return sd->active_state;
> -}
> -
> -/**
> - * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
> - *					   is locked and returns it
> - *
> - * @sd: The subdevice
> - *
> - * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state. If the state is not NULL, calls lockdep_assert_held()
> - * to issue a warning if the state is not locked.
> - *
> - * This function is to be used when the caller knows that the active state is
> - * already locked.
> - */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
> -{
> -	if (sd->active_state)
> -		lockdep_assert_held(sd->active_state->lock);
> -	return sd->active_state;
> -}
> -
> -/**
> - * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
> - *					     state for the subdevice
> - * @sd: The subdevice
> - *
> - * Returns the locked active state for the subdevice, or NULL if the subdev
> - * does not support active state.
> - *
> - * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> - */
> -static inline struct v4l2_subdev_state *
> -v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> -{
> -	if (sd->active_state)
> -		v4l2_subdev_lock_state(sd->active_state);
> -	return sd->active_state;
> -}
> -
>  /*
>   * A macro to generate the macro or function name for sub-devices state access
>   * wrapper macros below.
> @@ -1738,6 +1655,89 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
>  
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
> +/**
> + * v4l2_subdev_lock_state() - Locks the subdev state
> + * @state: The subdevice state
> + *
> + * Locks the given subdev state.
> + *
> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_lock(state->lock);
> +}
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> + * @state: The subdevice state
> + *
> + * Unlocks the given subdev state.
> + */
> +static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_unlock(state->lock);
> +}
> +
> +/**
> + * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
> + *					     is unlocked and returns it
> + * @sd: The subdevice
> + *
> + * Returns the active state for the subdevice, or NULL if the subdev does not
> + * support active state. If the state is not NULL, calls
> + * lockdep_assert_not_held() to issue a warning if the state is locked.
> + *
> + * This function is to be used e.g. when getting the active state for the sole
> + * purpose of passing it forward, without accessing the state fields.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_unlocked_active_state(struct v4l2_subdev *sd)
> +{
> +	if (sd->active_state)
> +		lockdep_assert_not_held(sd->active_state->lock);
> +	return sd->active_state;
> +}
> +
> +/**
> + * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
> + *					   is locked and returns it
> + *
> + * @sd: The subdevice
> + *
> + * Returns the active state for the subdevice, or NULL if the subdev does not
> + * support active state. If the state is not NULL, calls lockdep_assert_held()
> + * to issue a warning if the state is not locked.
> + *
> + * This function is to be used when the caller knows that the active state is
> + * already locked.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
> +{
> +	if (sd->active_state)
> +		lockdep_assert_held(sd->active_state->lock);
> +	return sd->active_state;
> +}
> +
> +/**
> + * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
> + *					     state for the subdevice
> + * @sd: The subdevice
> + *
> + * Returns the locked active state for the subdevice, or NULL if the subdev
> + * does not support active state.
> + *
> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> +{
> +	if (sd->active_state)
> +		v4l2_subdev_lock_state(sd->active_state);
> +	return sd->active_state;
> +}
> +
>  /**
>   * v4l2_subdev_init - initializes the sub-device struct
>   *
> 
> ---
> base-commit: e55a9482888da73eeadde5f13ef8bafce68a38ed
> change-id: 20231208-v4l2-state-mc-fix-b4f0ebfbb2bf
diff mbox series

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8b08f6640dee..61024a74ce38 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1311,89 +1311,6 @@  int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
  */
 void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
 
-/**
- * v4l2_subdev_lock_state() - Locks the subdev state
- * @state: The subdevice state
- *
- * Locks the given subdev state.
- *
- * The state must be unlocked with v4l2_subdev_unlock_state() after use.
- */
-static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
-{
-	mutex_lock(state->lock);
-}
-
-/**
- * v4l2_subdev_unlock_state() - Unlocks the subdev state
- * @state: The subdevice state
- *
- * Unlocks the given subdev state.
- */
-static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
-{
-	mutex_unlock(state->lock);
-}
-
-/**
- * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
- *					     is unlocked and returns it
- * @sd: The subdevice
- *
- * Returns the active state for the subdevice, or NULL if the subdev does not
- * support active state. If the state is not NULL, calls
- * lockdep_assert_not_held() to issue a warning if the state is locked.
- *
- * This function is to be used e.g. when getting the active state for the sole
- * purpose of passing it forward, without accessing the state fields.
- */
-static inline struct v4l2_subdev_state *
-v4l2_subdev_get_unlocked_active_state(struct v4l2_subdev *sd)
-{
-	if (sd->active_state)
-		lockdep_assert_not_held(sd->active_state->lock);
-	return sd->active_state;
-}
-
-/**
- * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
- *					   is locked and returns it
- *
- * @sd: The subdevice
- *
- * Returns the active state for the subdevice, or NULL if the subdev does not
- * support active state. If the state is not NULL, calls lockdep_assert_held()
- * to issue a warning if the state is not locked.
- *
- * This function is to be used when the caller knows that the active state is
- * already locked.
- */
-static inline struct v4l2_subdev_state *
-v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
-{
-	if (sd->active_state)
-		lockdep_assert_held(sd->active_state->lock);
-	return sd->active_state;
-}
-
-/**
- * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
- *					     state for the subdevice
- * @sd: The subdevice
- *
- * Returns the locked active state for the subdevice, or NULL if the subdev
- * does not support active state.
- *
- * The state must be unlocked with v4l2_subdev_unlock_state() after use.
- */
-static inline struct v4l2_subdev_state *
-v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
-{
-	if (sd->active_state)
-		v4l2_subdev_lock_state(sd->active_state);
-	return sd->active_state;
-}
-
 /*
  * A macro to generate the macro or function name for sub-devices state access
  * wrapper macros below.
@@ -1738,6 +1655,89 @@  int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
 
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
+/**
+ * v4l2_subdev_lock_state() - Locks the subdev state
+ * @state: The subdevice state
+ *
+ * Locks the given subdev state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
+{
+	mutex_lock(state->lock);
+}
+
+/**
+ * v4l2_subdev_unlock_state() - Unlocks the subdev state
+ * @state: The subdevice state
+ *
+ * Unlocks the given subdev state.
+ */
+static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
+{
+	mutex_unlock(state->lock);
+}
+
+/**
+ * v4l2_subdev_get_unlocked_active_state() - Checks that the active subdev state
+ *					     is unlocked and returns it
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state. If the state is not NULL, calls
+ * lockdep_assert_not_held() to issue a warning if the state is locked.
+ *
+ * This function is to be used e.g. when getting the active state for the sole
+ * purpose of passing it forward, without accessing the state fields.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_unlocked_active_state(struct v4l2_subdev *sd)
+{
+	if (sd->active_state)
+		lockdep_assert_not_held(sd->active_state->lock);
+	return sd->active_state;
+}
+
+/**
+ * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
+ *					   is locked and returns it
+ *
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state. If the state is not NULL, calls lockdep_assert_held()
+ * to issue a warning if the state is not locked.
+ *
+ * This function is to be used when the caller knows that the active state is
+ * already locked.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
+{
+	if (sd->active_state)
+		lockdep_assert_held(sd->active_state->lock);
+	return sd->active_state;
+}
+
+/**
+ * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
+ *					     state for the subdevice
+ * @sd: The subdevice
+ *
+ * Returns the locked active state for the subdevice, or NULL if the subdev
+ * does not support active state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
+{
+	if (sd->active_state)
+		v4l2_subdev_lock_state(sd->active_state);
+	return sd->active_state;
+}
+
 /**
  * v4l2_subdev_init - initializes the sub-device struct
  *