| Message ID | 20210708095550.682465-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt | expand |
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 ? Thanks j > > 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); > > 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 >
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@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); > > > > 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 > >
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. > > - 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 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). > > > 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); > > >
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
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);
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. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/i2c/max9286.c | 10 ---------- 1 file changed, 10 deletions(-)