Hi Jacopo,
On 2021-07-19 14:10:39 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> 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;
>
> - 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.
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.
>
> Thanks
> j
>
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > 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);
> >
> > 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);
> >
> > --
> > 2.32.0
> >
--
Regards,
Niklas Söderlund
