Moi,
On Mon, Oct 09, 2023 at 01:34:00PM +0300, Tomi Valkeinen wrote:
> On 03/10/2023 15:07, Sakari Ailus wrote:
> > Add two new functions, v4l2_subdev_lock_states() and
> > v4l2_subdev_unclock_states(), to acquire and release the state of two
> > sub-devices. They differ from calling v4l2_subdev_{un,}lock_state() so
> > that if the two states share the same lock, the lock is acquired only
> > once.
> >
> > Also use the new functions in v4l2_subdev_link_validate().
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 12 +++-----
> > include/media/v4l2-subdev.h | 40 +++++++++++++++++++++++++++
> > 2 files changed, 44 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 854f9d4db923..df9a1ae65410 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1377,17 +1377,13 @@ int v4l2_subdev_link_validate(struct media_link *link)
> > states_locked = sink_state && source_state;
> > - if (states_locked) {
> > - v4l2_subdev_lock_state(sink_state);
> > - v4l2_subdev_lock_state(source_state);
> > - }
> > + if (states_locked)
> > + v4l2_subdev_lock_states(sink_state, source_state);
> > ret = v4l2_subdev_link_validate_locked(link, states_locked);
> > - if (states_locked) {
> > - v4l2_subdev_unlock_state(sink_state);
> > - v4l2_subdev_unlock_state(source_state);
> > - }
> > + if (states_locked)
> > + v4l2_subdev_unlock_states(sink_state, source_state);
> > return ret;
> > }
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 7c34243ffed9..e49e8af2fb52 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1418,6 +1418,46 @@ static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> > mutex_unlock(state->lock);
> > }
> > +/**
> > + * v4l2_subdev_lock_states - Lock two sub-device states
> > + * @state1: One subdevice state
> > + * @state2: The other subdevice state
> > + *
> > + * Locks the state of two sub-devices.
> > + *
> > + * The states must be unlocked with v4l2_subdev_unlock_states() after use.
> > + *
> > + * This differs from calling v4l2_subdev_lock_state() on both states so that if
> > + * the states share the same lock, the lock is acquired only once (so no
> > + * deadlock occurs). Note that it must be ensured the locks must always be
> > + * acquired in the same order.
>
> Either s/must/will/ or maybe "note that the locks must always be
> acquired...".
How about:
The caller is responsible for ensuring the locks will always be acquired in
the same order.
>
> Maybe it should be stated that state1 and state2 have to be from subdevices
> that are connected via a media link and something like "the sink subdevice
> must always be state 1, and the source subdevice must always be state2".
That's less generic than the text in the patch. But in practice I presume
this would be the case --- this is intended to be used in link validation.
--
Regards,
Sakari Ailus
