Re: [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Laurent,

On 2021-07-19 16:15:45 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Jul 19, 2021 at 02:26:36PM +0200, Niklas Söderlund wrote:
> > On 2021-07-19 14:10:39 +0200, Jacopo Mondi wrote:
> > > On Thu, Jul 08, 2021 at 11:55:50AM +0200, Niklas Söderlund wrote:
> > > > There is no need to protect 'cfg_fmt' in get_fmt() and set_fmt() as the
> > > > core protects these callbacks. As this is the only usage of the mutex it
> > > > can be removed.
> > > 
> > > You know, I tried chasing where the vdev->lock used to protect the
> > > subdev's ioctl is set for mex9286 and I wasn't able to find it.
> > > 
> > > Please validate my understanding:
> > > 
> > > - The lock used by the core to protect the set/get format subdev ioctl
> > >   is the one in subdev_do_ioctl_lock()
> > > 
> > >   static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
> > >   {
> > >           struct video_device *vdev = video_devdata(file);
> > >           struct mutex *lock = vdev->lock;
> 
> Let's also note that subdev operations can also be called directly
> within the kernel, in video node-centric setups for instance. There are
> little changes the max9286 driver would be used in such a setup, but the
> .get_fmt() operation can be called during pipeline validation too.
> That's why subdev drivers are supposed to use locks internally.

This is a good point which disregarding of the ioctl lock issue pointed 
out by Jacopo makes this patch bad.

> 
> > > - the max9286 video subdevice node is registered (on R-Car) by
> > >   __v4l2_device_register_subdev_nodes() called by the root notifier
> > >   complete() callback
> > > 
> > > - The video_device created by __v4l2_device_register_subdev_nodes()
> > >   doesn't initialize any lock
> > > 
> > > What am I missing ?
> > 
> > One of the fun idiosyncrasies of V4L2 :-)
> > 
> > The lock comes from and are initialized by the video device used to 
> > register the V4L2 async notifier. Every subdevice created is bound to a 
> > vdev this way, and for example this is the vdev that events get routed 
> > to.
> 
> That doesn't seem right to me, could you point to the corresponding code

I was wrong it's the v4l2_device and not the vdev that is used for 
events. When an sd generates an event with the call chain,

v4l2_subdev_notify_event(priv->sd, ..)
    v4l2_subdev_notify(priv->sd)
        priv->sd->v4l2_dev->notify()


And v4l2_dev is the one passed to v4l2_async_notifier_register() and set 
in the async logic by

v4l2_async_register_subdev() OR v4l2_async_notifier_try_all_subdevs()
    v4l2_async_match_notify()
        v4l2_device_register_subdev()
            sd->v4l2_dev = v4l2_dev


> ?
> 
> > I assume this dates back pre the media-graph where every subdevice could 
> > be associated with a single vdev at probe time. With the media graph 
> > this makes little sens and IMHO should really be reworked. I tried once 
> > but it turned out to be a lot of work that I did not have time for at 
> > the time.
> 
> The video_device above is the one corresponding to the V4L2 subdev
> device node (when the MC API was developed, I attempted to split the
> device node handling out of video_device, to avoid embedding a full
> video_device in v4l2_subdev, but that was rejected).

I'm sorry that got rejected. If you had gotten your way I wound not have 
made the mistake and looked the fool by mixing between vdev and v4l2_dev 
in this mail :-)

> 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 10 ----------
> > > >  1 file changed, 10 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index 1aa2c58fd38c5d2b..b1d11a50d6e53ecc 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -18,7 +18,6 @@
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/i2c-mux.h>
> > > >  #include <linux/module.h>
> > > > -#include <linux/mutex.h>
> > > >  #include <linux/of_graph.h>
> > > >  #include <linux/regulator/consumer.h>
> > > >  #include <linux/slab.h>
> > > > @@ -173,9 +172,6 @@ struct max9286_priv {
> > > >
> > > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > >
> > > > -	/* Protects controls and fmt structures */
> > > > -	struct mutex mutex;
> > > > -
> > > >  	unsigned int nsources;
> > > >  	unsigned int source_mask;
> > > >  	unsigned int route_mask;
> > > > @@ -768,9 +764,7 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> > > >  	if (!cfg_fmt)
> > > >  		return -EINVAL;
> > > >
> > > > -	mutex_lock(&priv->mutex);
> > > >  	*cfg_fmt = format->format;
> > > > -	mutex_unlock(&priv->mutex);
> 
> On a side note, the usual practice is to use the same lock to prevent
> the active format from being changed during streaming.
> 
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -796,9 +790,7 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > >  	if (!cfg_fmt)
> > > >  		return -EINVAL;
> > > >
> > > > -	mutex_lock(&priv->mutex);
> > > >  	format->format = *cfg_fmt;
> > > > -	mutex_unlock(&priv->mutex);
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -1259,8 +1251,6 @@ static int max9286_probe(struct i2c_client *client)
> > > >  	if (!priv)
> > > >  		return -ENOMEM;
> > > >
> > > > -	mutex_init(&priv->mutex);
> > > > -
> > > >  	priv->client = client;
> > > >  	i2c_set_clientdata(client, priv);
> > > >
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund