Re: [PATCH v2 5/6] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Mon, Dec 20, 2021 at 02:51:32PM +0200, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Dec 17, 2021 at 03:50:21PM +0200, Tomi Valkeinen wrote:
> > All suitable subdev ops are now passed either the TRY or the ACTIVE
> > state by the v4l2 core. However, other subdev drivers can still call the
> > ops passing NULL as the state, implying the active case.
> > 
> > For all current upstream drivers this doesn't matter, as they do not
> > expect to get a valid state for ACTIVE case. But future drivers which
> > support multiplexed streaming and routing will depend on getting a state
> > for both active and try cases.
> > 
> > For new drivers we can mandate that the pipelines where the drivers are
> > used need to pass the state properly, or preferably, not call such
> > subdev ops at all.
> > 
> > However, if an existing subdev driver is changed to support multiplexed
> > streams, the driver has to consider cases where its ops will be called
> > with NULL state. The problem can easily be solved by using the
> > v4l2_subdev_lock_and_return_state() helper, introduced here.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 867e19ef80bd..d6c7db1d37e4 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1321,4 +1321,40 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> >   */
> >  void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> >  
> > +/**
> > + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
> > + * @sd: subdevice
> > + * @state: subdevice state as passed to the subdev op
> > + *
> > + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
> > + * NULL as the state parameter, as subdevs always used to have their active
> > + * state stored privately.
> > + *
> > + * However, newer state-aware subdev drivers, which store their active state in
> > + * a common place, subdev->active_state, expect to always get a proper state as
> > + * a parameter.
> > + *
> > + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
> > + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
> > + * issue by using subdev->active_state in case the passed state is NULL.
> > + *
> > + * This is a temporary helper function, and should be removed when we can ensure
> > + * that all drivers pass proper state when calling other subdevs.
> > + */
> > +static inline struct v4l2_subdev_state *
> > +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
> > +				  struct v4l2_subdev_state *state)
> > +{
> > +	if (!state)
> > +		dev_notice_once(sd->dev,
> 
> I thought about this a little more.
> 
> This currently warns once when there's something that calls the function
> with NULL state.
> 
> How about going a bit further, moving this to a macro so all instances will
> be covered, and then using WARN_ON_ONCE() so there would be a clear
> indication where the warning comes from?

Actually --- please ignore this. The function is already static inline in
the header and prints the device name. That should be enough to point out
the culprit.

-- 
Sakari Ailus