Re: [PATCH 02/13] v4l: vsp1: Protect against race conditions between get and set format

[Laurent Pinchart]

Hi Niklas,

Thank you for the review.

On Wednesday 14 Sep 2016 20:23:18 Niklas Söderlund wrote:
> On 2016-09-14 02:16:55 +0300, Laurent Pinchart wrote:
> > The subdev userspace API isn't serialized in the core, serialize access
> > to formats and selection rectangles in the driver.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_bru.c| 28 +++-
> >  drivers/media/platform/vsp1/vsp1_clu.c| 15 ---
> >  drivers/media/platform/vsp1/vsp1_entity.c | 22 +---
> >  drivers/media/platform/vsp1/vsp1_entity.h |  4 ++-
> >  drivers/media/platform/vsp1/vsp1_hsit.c   | 15 ---
> >  drivers/media/platform/vsp1/vsp1_lif.c| 15 ---
> >  drivers/media/platform/vsp1/vsp1_lut.c| 15 ---
> >  drivers/media/platform/vsp1/vsp1_rwpf.c   | 44 +++---
> >  drivers/media/platform/vsp1/vsp1_sru.c| 26 +-
> >  drivers/media/platform/vsp1/vsp1_uds.c| 26 +-
> >  10 files changed, 161 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_bru.c
> > b/drivers/media/platform/vsp1/vsp1_bru.c index 8268b87727a7..26b9e2282a41
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_bru.c
> > +++ b/drivers/media/platform/vsp1/vsp1_bru.c
> > @@ -142,10 +142,15 @@ static int bru_set_format(struct v4l2_subdev
> > *subdev,
> > 
> > struct vsp1_bru *bru = to_bru(subdev);
> > struct v4l2_subdev_pad_config *config;
> > struct v4l2_mbus_framefmt *format;
> > 
> > +   int ret = 0;
> > +
> > +   mutex_lock(>entity.lock);
> > 
> > config = vsp1_entity_get_pad_config(>entity, cfg, fmt->which);
> > 
> > -   if (!config)
> > -   return -EINVAL;
> > +   if (!config) {
> > +   goto done;
> > +   ret = -EINVAL;
> 
> This looks funny to me, you probably intended to do that in the other
> order right?

Oops, good catch !

> If you fix this feel free to add my:
> 
> Acked-by: Niklas Söderlund 

Fixed and applied your ack (with +renesas as mentioned in your other email).

> > +   }
> > 
> > bru_try_format(bru, config, fmt->pad, >format);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/13] v4l: vsp1: Protect against race conditions between get and set format

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2016-09-14 02:16:55 +0300, Laurent Pinchart wrote:
> The subdev userspace API isn't serialized in the core, serialize access
> to formats and selection rectangles in the driver.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/platform/vsp1/vsp1_bru.c| 28 +++-
>  drivers/media/platform/vsp1/vsp1_clu.c| 15 ---
>  drivers/media/platform/vsp1/vsp1_entity.c | 22 +---
>  drivers/media/platform/vsp1/vsp1_entity.h |  4 ++-
>  drivers/media/platform/vsp1/vsp1_hsit.c   | 15 ---
>  drivers/media/platform/vsp1/vsp1_lif.c| 15 ---
>  drivers/media/platform/vsp1/vsp1_lut.c| 15 ---
>  drivers/media/platform/vsp1/vsp1_rwpf.c   | 44 
> +++
>  drivers/media/platform/vsp1/vsp1_sru.c| 26 +-
>  drivers/media/platform/vsp1/vsp1_uds.c| 26 +-
>  10 files changed, 161 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
> b/drivers/media/platform/vsp1/vsp1_bru.c
> index 8268b87727a7..26b9e2282a41 100644
> --- a/drivers/media/platform/vsp1/vsp1_bru.c
> +++ b/drivers/media/platform/vsp1/vsp1_bru.c
> @@ -142,10 +142,15 @@ static int bru_set_format(struct v4l2_subdev *subdev,
>   struct vsp1_bru *bru = to_bru(subdev);
>   struct v4l2_subdev_pad_config *config;
>   struct v4l2_mbus_framefmt *format;
> + int ret = 0;
> +
> + mutex_lock(>entity.lock);
>  
>   config = vsp1_entity_get_pad_config(>entity, cfg, fmt->which);
> - if (!config)
> - return -EINVAL;
> + if (!config) {
> + goto done;
> + ret = -EINVAL;

This looks funny to me, you probably intended to do that in the other 
order right? If you fix this feel free to add my:

Acked-by: Niklas Söderlund 

> + }
>  
>   bru_try_format(bru, config, fmt->pad, >format);
>  
> @@ -174,7 +179,9 @@ static int bru_set_format(struct v4l2_subdev *subdev,
>   }
>   }
>  
> - return 0;
> +done:
> + mutex_unlock(>entity.lock);
> + return ret;
>  }
>  
>  static int bru_get_selection(struct v4l2_subdev *subdev,
> @@ -201,7 +208,9 @@ static int bru_get_selection(struct v4l2_subdev *subdev,
>   if (!config)
>   return -EINVAL;
>  
> + mutex_lock(>entity.lock);
>   sel->r = *bru_get_compose(bru, config, sel->pad);
> + mutex_unlock(>entity.lock);
>   return 0;
>  
>   default:
> @@ -217,6 +226,7 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
>   struct v4l2_subdev_pad_config *config;
>   struct v4l2_mbus_framefmt *format;
>   struct v4l2_rect *compose;
> + int ret = 0;
>  
>   if (sel->pad == bru->entity.source_pad)
>   return -EINVAL;
> @@ -224,9 +234,13 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
>   if (sel->target != V4L2_SEL_TGT_COMPOSE)
>   return -EINVAL;
>  
> + mutex_lock(>entity.lock);
> +
>   config = vsp1_entity_get_pad_config(>entity, cfg, sel->which);
> - if (!config)
> - return -EINVAL;
> + if (!config) {
> + ret = -EINVAL;
> + goto done;
> + }
>  
>   /* The compose rectangle top left corner must be inside the output
>* frame.
> @@ -246,7 +260,9 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
>   compose = bru_get_compose(bru, config, sel->pad);
>   *compose = sel->r;
>  
> - return 0;
> +done:
> + mutex_unlock(>entity.lock);
> + return ret;
>  }
>  
>  static const struct v4l2_subdev_pad_ops bru_pad_ops = {
> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
> b/drivers/media/platform/vsp1/vsp1_clu.c
> index b63d2dbe5ea3..e1fd03811dda 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -148,10 +148,15 @@ static int clu_set_format(struct v4l2_subdev *subdev,
>   struct vsp1_clu *clu = to_clu(subdev);
>   struct v4l2_subdev_pad_config *config;
>   struct v4l2_mbus_framefmt *format;
> + int ret = 0;
> +
> + mutex_lock(>entity.lock);
>  
>   config = vsp1_entity_get_pad_config(>entity, cfg, fmt->which);
> - if (!config)
> - return -EINVAL;
> + if (!config) {
> + ret = -EINVAL;
> + goto done;
> + }
>  
>   /* Default to YUV if the requested format is not supported. */
>   if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
> @@ -164,7 +169,7 @@ static int clu_set_format(struct v4l2_subdev *subdev,
>   if (fmt->pad == CLU_PAD_SOURCE) {
>   /* The CLU output format can't be modified. */
>   fmt->format = *format;
> - return 0;
> + goto done;
>   }
>  
>   format->code = fmt->format.code;
> @@ -182,7 +187,9 @@ static int 

[PATCH 02/13] v4l: vsp1: Protect against race conditions between get and set format

[Laurent Pinchart]

The subdev userspace API isn't serialized in the core, serialize access
to formats and selection rectangles in the driver.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/platform/vsp1/vsp1_bru.c| 28 +++-
 drivers/media/platform/vsp1/vsp1_clu.c| 15 ---
 drivers/media/platform/vsp1/vsp1_entity.c | 22 +---
 drivers/media/platform/vsp1/vsp1_entity.h |  4 ++-
 drivers/media/platform/vsp1/vsp1_hsit.c   | 15 ---
 drivers/media/platform/vsp1/vsp1_lif.c| 15 ---
 drivers/media/platform/vsp1/vsp1_lut.c| 15 ---
 drivers/media/platform/vsp1/vsp1_rwpf.c   | 44 +++
 drivers/media/platform/vsp1/vsp1_sru.c| 26 +-
 drivers/media/platform/vsp1/vsp1_uds.c| 26 +-
 10 files changed, 161 insertions(+), 49 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
b/drivers/media/platform/vsp1/vsp1_bru.c
index 8268b87727a7..26b9e2282a41 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -142,10 +142,15 @@ static int bru_set_format(struct v4l2_subdev *subdev,
struct vsp1_bru *bru = to_bru(subdev);
struct v4l2_subdev_pad_config *config;
struct v4l2_mbus_framefmt *format;
+   int ret = 0;
+
+   mutex_lock(>entity.lock);
 
config = vsp1_entity_get_pad_config(>entity, cfg, fmt->which);
-   if (!config)
-   return -EINVAL;
+   if (!config) {
+   goto done;
+   ret = -EINVAL;
+   }
 
bru_try_format(bru, config, fmt->pad, >format);
 
@@ -174,7 +179,9 @@ static int bru_set_format(struct v4l2_subdev *subdev,
}
}
 
-   return 0;
+done:
+   mutex_unlock(>entity.lock);
+   return ret;
 }
 
 static int bru_get_selection(struct v4l2_subdev *subdev,
@@ -201,7 +208,9 @@ static int bru_get_selection(struct v4l2_subdev *subdev,
if (!config)
return -EINVAL;
 
+   mutex_lock(>entity.lock);
sel->r = *bru_get_compose(bru, config, sel->pad);
+   mutex_unlock(>entity.lock);
return 0;
 
default:
@@ -217,6 +226,7 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
struct v4l2_subdev_pad_config *config;
struct v4l2_mbus_framefmt *format;
struct v4l2_rect *compose;
+   int ret = 0;
 
if (sel->pad == bru->entity.source_pad)
return -EINVAL;
@@ -224,9 +234,13 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
if (sel->target != V4L2_SEL_TGT_COMPOSE)
return -EINVAL;
 
+   mutex_lock(>entity.lock);
+
config = vsp1_entity_get_pad_config(>entity, cfg, sel->which);
-   if (!config)
-   return -EINVAL;
+   if (!config) {
+   ret = -EINVAL;
+   goto done;
+   }
 
/* The compose rectangle top left corner must be inside the output
 * frame.
@@ -246,7 +260,9 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
compose = bru_get_compose(bru, config, sel->pad);
*compose = sel->r;
 
-   return 0;
+done:
+   mutex_unlock(>entity.lock);
+   return ret;
 }
 
 static const struct v4l2_subdev_pad_ops bru_pad_ops = {
diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index b63d2dbe5ea3..e1fd03811dda 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -148,10 +148,15 @@ static int clu_set_format(struct v4l2_subdev *subdev,
struct vsp1_clu *clu = to_clu(subdev);
struct v4l2_subdev_pad_config *config;
struct v4l2_mbus_framefmt *format;
+   int ret = 0;
+
+   mutex_lock(>entity.lock);
 
config = vsp1_entity_get_pad_config(>entity, cfg, fmt->which);
-   if (!config)
-   return -EINVAL;
+   if (!config) {
+   ret = -EINVAL;
+   goto done;
+   }
 
/* Default to YUV if the requested format is not supported. */
if (fmt->format.code != MEDIA_BUS_FMT_ARGB_1X32 &&
@@ -164,7 +169,7 @@ static int clu_set_format(struct v4l2_subdev *subdev,
if (fmt->pad == CLU_PAD_SOURCE) {
/* The CLU output format can't be modified. */
fmt->format = *format;
-   return 0;
+   goto done;
}
 
format->code = fmt->format.code;
@@ -182,7 +187,9 @@ static int clu_set_format(struct v4l2_subdev *subdev,
CLU_PAD_SOURCE);
*format = fmt->format;
 
-   return 0;
+done:
+   mutex_unlock(>entity.lock);
+   return ret;
 }
 
 /* 
-
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c 
b/drivers/media/platform/vsp1/vsp1_entity.c
index