* [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt
@ 2021-07-08 9:55 Niklas Söderlund
2021-07-19 12:10 ` Jacopo Mondi
0 siblings, 1 reply; 5+ messages in thread
From: Niklas Söderlund @ 2021-07-08 9:55 UTC (permalink / raw)
To: Jacopo Mondi, Kieran Bingham, Laurent Pinchart, linux-media
Cc: linux-renesas-soc, Niklas Söderlund
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt
2021-07-08 9:55 [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt Niklas Söderlund
@ 2021-07-19 12:10 ` Jacopo Mondi
2021-07-19 12:26 ` Niklas Söderlund
0 siblings, 1 reply; 5+ messages in thread
From: Jacopo Mondi @ 2021-07-19 12:10 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart, linux-media,
linux-renesas-soc
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt
2021-07-19 12:10 ` Jacopo Mondi
@ 2021-07-19 12:26 ` Niklas Söderlund
2021-07-19 13:15 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Niklas Söderlund @ 2021-07-19 12:26 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Jacopo Mondi, Kieran Bingham, Laurent Pinchart, linux-media,
linux-renesas-soc
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
> >
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt
2021-07-19 12:26 ` Niklas Söderlund
@ 2021-07-19 13:15 ` Laurent Pinchart
2021-07-19 13:50 ` Niklas Söderlund
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2021-07-19 13:15 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Jacopo Mondi, Jacopo Mondi, Kieran Bingham, linux-media,
linux-renesas-soc
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);
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt
2021-07-19 13:15 ` Laurent Pinchart
@ 2021-07-19 13:50 ` Niklas Söderlund
0 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2021-07-19 13:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, Jacopo Mondi, Kieran Bingham, linux-media,
linux-renesas-soc
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-19 13:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08 9:55 [PATCH] media: i2c: max9286: Remove unneeded mutex for get_fmt and set_fmt Niklas Söderlund
2021-07-19 12:10 ` Jacopo Mondi
2021-07-19 12:26 ` Niklas Söderlund
2021-07-19 13:15 ` Laurent Pinchart
2021-07-19 13:50 ` Niklas Söderlund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).