[PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

Any local subdev state should be allocated and free'd using
__v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
takes care of calling .init_cfg() subdev op. Without this,
subdev internal state might be uninitialized by the time
any other subdev op is called.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Philippe CORNU <philippe.cornu@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
---
 drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index c604d672c2156..c68d32931b277 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 			struct dcmi_framesize *sd_framesize)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
 	struct dcmi_framesize sd_fsize;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
 	bool do_crop;
 	int ret;
 
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
@@ -1036,10 +1044,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	/* Update pix regarding to what sensor can do */
 	v4l2_fill_pix_format(pix, &format.format);
@@ -1079,7 +1086,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	if (sd_framesize)
 		*sd_framesize = sd_fsize;
 
-	return 0;
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
@@ -1183,31 +1192,37 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
 				  struct v4l2_pix_format *pix)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
 	int ret;
 
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
-- 
2.35.1

[PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

Any local subdev state should be allocated and free'd using
__v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
takes care of calling .init_cfg() subdev op. Without this,
subdev internal state might be uninitialized by the time
any other subdev op is called.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Philippe CORNU <philippe.cornu@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
---
 drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index c604d672c2156..c68d32931b277 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 			struct dcmi_framesize *sd_framesize)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
 	struct dcmi_framesize sd_fsize;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
 	bool do_crop;
 	int ret;
 
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
@@ -1036,10 +1044,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	/* Update pix regarding to what sensor can do */
 	v4l2_fill_pix_format(pix, &format.format);
@@ -1079,7 +1086,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	if (sd_framesize)
 		*sd_framesize = sd_fsize;
 
-	return 0;
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
@@ -1183,31 +1192,37 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
 				  struct v4l2_pix_format *pix)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
 	int ret;
 
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

Hi Marek, Tomi, Laurent,

On 27/06/2022 19:41, Marek Vasut wrote:
> Any local subdev state should be allocated and free'd using
> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> takes care of calling .init_cfg() subdev op. Without this,
> subdev internal state might be uninitialized by the time
> any other subdev op is called.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alain Volmat <alain.volmat@foss.st.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
> ---
>  drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> index c604d672c2156..c68d32931b277 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  			struct dcmi_framesize *sd_framesize)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
>  	struct dcmi_framesize sd_fsize;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
>  	bool do_crop;
>  	int ret;
>  
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +

I've been reading the discussion for the v1 patch, and I seriously do not like this.

My comments are not specifically for this patch, but for all cases where
__v4l2_subdev_state_alloc is called.

It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
exactly trivial either.

I think a helper function might be beneficial, but the real problem is with the
comment: it does not explain why you shouldn't use it and what needs to be done
to fix it.

My suggestion would be to document that in the kerneldoc for this function in
media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
in the other drivers that use this).

And another question: are more drivers affected by this? Is it possible to
find those and fix them all?

Regards,

	Hans

>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
> @@ -1036,10 +1044,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
>  	if (ret < 0)
> -		return ret;
> +		goto done;
>  
>  	/* Update pix regarding to what sensor can do */
>  	v4l2_fill_pix_format(pix, &format.format);
> @@ -1079,7 +1086,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	if (sd_framesize)
>  		*sd_framesize = sd_fsize;
>  
> -	return 0;
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
> @@ -1183,31 +1192,37 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
>  				  struct v4l2_pix_format *pix)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
>  	int ret;
>  
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

Hi Marek, Tomi, Laurent,

On 27/06/2022 19:41, Marek Vasut wrote:
> Any local subdev state should be allocated and free'd using
> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> takes care of calling .init_cfg() subdev op. Without this,
> subdev internal state might be uninitialized by the time
> any other subdev op is called.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alain Volmat <alain.volmat@foss.st.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
> ---
>  drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> index c604d672c2156..c68d32931b277 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  			struct dcmi_framesize *sd_framesize)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
>  	struct dcmi_framesize sd_fsize;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
>  	bool do_crop;
>  	int ret;
>  
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +

I've been reading the discussion for the v1 patch, and I seriously do not like this.

My comments are not specifically for this patch, but for all cases where
__v4l2_subdev_state_alloc is called.

It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
exactly trivial either.

I think a helper function might be beneficial, but the real problem is with the
comment: it does not explain why you shouldn't use it and what needs to be done
to fix it.

My suggestion would be to document that in the kerneldoc for this function in
media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
in the other drivers that use this).

And another question: are more drivers affected by this? Is it possible to
find those and fix them all?

Regards,

	Hans

>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
> @@ -1036,10 +1044,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
>  	if (ret < 0)
> -		return ret;
> +		goto done;
>  
>  	/* Update pix regarding to what sensor can do */
>  	v4l2_fill_pix_format(pix, &format.format);
> @@ -1079,7 +1086,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	if (sd_framesize)
>  		*sd_framesize = sd_fsize;
>  
> -	return 0;
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
> @@ -1183,31 +1192,37 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
>  				  struct v4l2_pix_format *pix)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
>  	int ret;
>  
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 11:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,

Hi,

[...]

>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> index c604d672c2156..c68d32931b277 100644
>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>   			struct dcmi_framesize *sd_framesize)
>>   {
>>   	const struct dcmi_format *sd_fmt;
>> +	static struct lock_class_key key;
>>   	struct dcmi_framesize sd_fsize;
>>   	struct v4l2_pix_format *pix = &f->fmt.pix;
>> -	struct v4l2_subdev_pad_config pad_cfg;
>> -	struct v4l2_subdev_state pad_state = {
>> -		.pads = &pad_cfg
>> -		};
>> +	struct v4l2_subdev_state *sd_state;
>>   	struct v4l2_subdev_format format = {
>>   		.which = V4L2_SUBDEV_FORMAT_TRY,
>>   	};
>>   	bool do_crop;
>>   	int ret;
>>   
>> +	/*
>> +	 * FIXME: Drop this call, drivers are not supposed to use
>> +	 * __v4l2_subdev_state_alloc().
>> +	 */
>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>> +	if (IS_ERR(sd_state))
>> +		return PTR_ERR(sd_state);
>> +
> 
> I've been reading the discussion for the v1 patch, and I seriously do not like this.
> 
> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.
> 
> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.
> 
> I think a helper function might be beneficial, but the real problem is with the
> comment: it does not explain why you shouldn't use it and what needs to be done
> to fix it.
> 
> My suggestion would be to document that in the kerneldoc for this function in
> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> in the other drivers that use this).

Would it be OK if I left the core rework/documentation to Tomi as a 
subsequent patch to this one ?

> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?

Probably, I only ran into it with the DCMI so far.

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 11:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,

Hi,

[...]

>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> index c604d672c2156..c68d32931b277 100644
>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>   			struct dcmi_framesize *sd_framesize)
>>   {
>>   	const struct dcmi_format *sd_fmt;
>> +	static struct lock_class_key key;
>>   	struct dcmi_framesize sd_fsize;
>>   	struct v4l2_pix_format *pix = &f->fmt.pix;
>> -	struct v4l2_subdev_pad_config pad_cfg;
>> -	struct v4l2_subdev_state pad_state = {
>> -		.pads = &pad_cfg
>> -		};
>> +	struct v4l2_subdev_state *sd_state;
>>   	struct v4l2_subdev_format format = {
>>   		.which = V4L2_SUBDEV_FORMAT_TRY,
>>   	};
>>   	bool do_crop;
>>   	int ret;
>>   
>> +	/*
>> +	 * FIXME: Drop this call, drivers are not supposed to use
>> +	 * __v4l2_subdev_state_alloc().
>> +	 */
>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>> +	if (IS_ERR(sd_state))
>> +		return PTR_ERR(sd_state);
>> +
> 
> I've been reading the discussion for the v1 patch, and I seriously do not like this.
> 
> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.
> 
> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.
> 
> I think a helper function might be beneficial, but the real problem is with the
> comment: it does not explain why you shouldn't use it and what needs to be done
> to fix it.
> 
> My suggestion would be to document that in the kerneldoc for this function in
> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> in the other drivers that use this).

Would it be OK if I left the core rework/documentation to Tomi as a 
subsequent patch to this one ?

> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?

Probably, I only ran into it with the DCMI so far.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

On 29/06/2022 13:04, Marek Vasut wrote:
> On 6/29/22 11:41, Hans Verkuil wrote:
>> Hi Marek, Tomi, Laurent,
> 
> Hi,
> 
> [...]
> 
>>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> index c604d672c2156..c68d32931b277 100644
>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>               struct dcmi_framesize *sd_framesize)
>>>   {
>>>       const struct dcmi_format *sd_fmt;
>>> +    static struct lock_class_key key;
>>>       struct dcmi_framesize sd_fsize;
>>>       struct v4l2_pix_format *pix = &f->fmt.pix;
>>> -    struct v4l2_subdev_pad_config pad_cfg;
>>> -    struct v4l2_subdev_state pad_state = {
>>> -        .pads = &pad_cfg
>>> -        };
>>> +    struct v4l2_subdev_state *sd_state;
>>>       struct v4l2_subdev_format format = {
>>>           .which = V4L2_SUBDEV_FORMAT_TRY,
>>>       };
>>>       bool do_crop;
>>>       int ret;
>>>   +    /*
>>> +     * FIXME: Drop this call, drivers are not supposed to use
>>> +     * __v4l2_subdev_state_alloc().
>>> +     */
>>> +    sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>> +    if (IS_ERR(sd_state))
>>> +        return PTR_ERR(sd_state);
>>> +
>>
>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>
>> My comments are not specifically for this patch, but for all cases where
>> __v4l2_subdev_state_alloc is called.
>>
>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>> exactly trivial either.
>>
>> I think a helper function might be beneficial, but the real problem is with the
>> comment: it does not explain why you shouldn't use it and what needs to be done
>> to fix it.
>>
>> My suggestion would be to document that in the kerneldoc for this function in
>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>> in the other drivers that use this).
> 
> Would it be OK if I left the core rework/documentation to Tomi as a subsequent patch to this one ?

Yes. It would be nice if Tomi can make a patch fixing the comments as suggested above,
and then your patch can go on top. Adding a helper function can be done later, it's
not my main concern.

> 
>> And another question: are more drivers affected by this? Is it possible to
>> find those and fix them all?
> 
> Probably, I only ran into it with the DCMI so far.

Tomi, do you know?

Regards,

	Hans

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

On 29/06/2022 13:04, Marek Vasut wrote:
> On 6/29/22 11:41, Hans Verkuil wrote:
>> Hi Marek, Tomi, Laurent,
> 
> Hi,
> 
> [...]
> 
>>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> index c604d672c2156..c68d32931b277 100644
>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>               struct dcmi_framesize *sd_framesize)
>>>   {
>>>       const struct dcmi_format *sd_fmt;
>>> +    static struct lock_class_key key;
>>>       struct dcmi_framesize sd_fsize;
>>>       struct v4l2_pix_format *pix = &f->fmt.pix;
>>> -    struct v4l2_subdev_pad_config pad_cfg;
>>> -    struct v4l2_subdev_state pad_state = {
>>> -        .pads = &pad_cfg
>>> -        };
>>> +    struct v4l2_subdev_state *sd_state;
>>>       struct v4l2_subdev_format format = {
>>>           .which = V4L2_SUBDEV_FORMAT_TRY,
>>>       };
>>>       bool do_crop;
>>>       int ret;
>>>   +    /*
>>> +     * FIXME: Drop this call, drivers are not supposed to use
>>> +     * __v4l2_subdev_state_alloc().
>>> +     */
>>> +    sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>> +    if (IS_ERR(sd_state))
>>> +        return PTR_ERR(sd_state);
>>> +
>>
>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>
>> My comments are not specifically for this patch, but for all cases where
>> __v4l2_subdev_state_alloc is called.
>>
>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>> exactly trivial either.
>>
>> I think a helper function might be beneficial, but the real problem is with the
>> comment: it does not explain why you shouldn't use it and what needs to be done
>> to fix it.
>>
>> My suggestion would be to document that in the kerneldoc for this function in
>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>> in the other drivers that use this).
> 
> Would it be OK if I left the core rework/documentation to Tomi as a subsequent patch to this one ?

Yes. It would be nice if Tomi can make a patch fixing the comments as suggested above,
and then your patch can go on top. Adding a helper function can be done later, it's
not my main concern.

> 
>> And another question: are more drivers affected by this? Is it possible to
>> find those and fix them all?
> 
> Probably, I only ran into it with the DCMI so far.

Tomi, do you know?

Regards,

	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 12:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,
> 
> On 27/06/2022 19:41, Marek Vasut wrote:
>> Any local subdev state should be allocated and free'd using
>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>> takes care of calling .init_cfg() subdev op. Without this,
>> subdev internal state might be uninitialized by the time
>> any other subdev op is called.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> ---
>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>> ---
>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> index c604d672c2156..c68d32931b277 100644
>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>   			struct dcmi_framesize *sd_framesize)
>>   {
>>   	const struct dcmi_format *sd_fmt;
>> +	static struct lock_class_key key;
>>   	struct dcmi_framesize sd_fsize;
>>   	struct v4l2_pix_format *pix = &f->fmt.pix;
>> -	struct v4l2_subdev_pad_config pad_cfg;
>> -	struct v4l2_subdev_state pad_state = {
>> -		.pads = &pad_cfg
>> -		};
>> +	struct v4l2_subdev_state *sd_state;
>>   	struct v4l2_subdev_format format = {
>>   		.which = V4L2_SUBDEV_FORMAT_TRY,
>>   	};
>>   	bool do_crop;
>>   	int ret;
>>   
>> +	/*
>> +	 * FIXME: Drop this call, drivers are not supposed to use
>> +	 * __v4l2_subdev_state_alloc().
>> +	 */
>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>> +	if (IS_ERR(sd_state))
>> +		return PTR_ERR(sd_state);
>> +
> 
> I've been reading the discussion for the v1 patch, and I seriously do not like this.

I don't like it either.

> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.

Just to emphasize it: afaics this is not an issue with the subdev state. 
This driver was already broken. Before the subdev state change the fix 
would have been to call source subdev's init_cfg. Now 
__v4l2_subdev_state_alloc handles calling init_cfg (along with a few 
other inits).

> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.

Counting this one? I found 3 cases in v5.18-rc4:

1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:

Allocates the state for v4l2_subdev_call, set_fmt, TRY.

Here, a helper macro which does the alloc would be a solution.

2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:

Allocates the state for storing internal active state.

Here, I think, the easiest fix is for the driver to use the subdev state 
properly.

3) drivers/staging/media/tegra-video/vi.c:

Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt, 
TRY. Interestingly the code also calls get_selection but without passing 
the state...

This is a more interesting case as the source's subdev state is actually 
modified by the driver. The driver calls enum_frame_size, then modifies 
the state, then calls set_fmt. I'm not sure if that's really legal... 
The driver directly modifies the state, instead of calling set_selection?

> I think a helper function might be beneficial, but the real problem is with the
> comment: it does not explain why you shouldn't use it and what needs to be done
> to fix it.

That is true. There's no single answer to that. I think instead of 
trying to document that in the v4l2-subdev doc, we can enhance the 
comments in those three call sites to explain how it needs to be fixed.

> My suggestion would be to document that in the kerneldoc for this function in
> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> in the other drivers that use this).
> 
> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?

I think any driver that calls a source subdev's subdev ops which a 
subdev state as a parameter (the ones that used to take 
v4l2_subdev_pad_config), and does not call init_cfg is broken in the 
same way. With some grepping, I couldn't find anyone calling init_cfg. 
Finding those drivers which do those calls is a bit more difficult, but 
can be done with some efforts.

atmel-isc-base.c is one I found, and looks like it's doing something a 
bit similar to the 3) case above.

Perhaps the best way to solve this is just to remove the underscores 
from __v4l2_subdev_state_alloc, and change all the drivers which create 
temporary v4l2_subdev_states to use that (and the free) functions. And 
also create the helper macro which can be used in those cases where the 
call is simple (the state is not modified or accessed by the caller).

It would've been nice to keep __v4l2_subdev_state_alloc internal to the 
v4l2-subdev, but maybe the v4l2 drivers are not there yet. The non-MC 
drivers seem to be doing all kinds of interesting things.

  Tomi

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 12:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,
> 
> On 27/06/2022 19:41, Marek Vasut wrote:
>> Any local subdev state should be allocated and free'd using
>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>> takes care of calling .init_cfg() subdev op. Without this,
>> subdev internal state might be uninitialized by the time
>> any other subdev op is called.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> ---
>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>> ---
>>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>   1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> index c604d672c2156..c68d32931b277 100644
>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>   			struct dcmi_framesize *sd_framesize)
>>   {
>>   	const struct dcmi_format *sd_fmt;
>> +	static struct lock_class_key key;
>>   	struct dcmi_framesize sd_fsize;
>>   	struct v4l2_pix_format *pix = &f->fmt.pix;
>> -	struct v4l2_subdev_pad_config pad_cfg;
>> -	struct v4l2_subdev_state pad_state = {
>> -		.pads = &pad_cfg
>> -		};
>> +	struct v4l2_subdev_state *sd_state;
>>   	struct v4l2_subdev_format format = {
>>   		.which = V4L2_SUBDEV_FORMAT_TRY,
>>   	};
>>   	bool do_crop;
>>   	int ret;
>>   
>> +	/*
>> +	 * FIXME: Drop this call, drivers are not supposed to use
>> +	 * __v4l2_subdev_state_alloc().
>> +	 */
>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>> +	if (IS_ERR(sd_state))
>> +		return PTR_ERR(sd_state);
>> +
> 
> I've been reading the discussion for the v1 patch, and I seriously do not like this.

I don't like it either.

> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.

Just to emphasize it: afaics this is not an issue with the subdev state. 
This driver was already broken. Before the subdev state change the fix 
would have been to call source subdev's init_cfg. Now 
__v4l2_subdev_state_alloc handles calling init_cfg (along with a few 
other inits).

> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.

Counting this one? I found 3 cases in v5.18-rc4:

1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:

Allocates the state for v4l2_subdev_call, set_fmt, TRY.

Here, a helper macro which does the alloc would be a solution.

2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:

Allocates the state for storing internal active state.

Here, I think, the easiest fix is for the driver to use the subdev state 
properly.

3) drivers/staging/media/tegra-video/vi.c:

Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt, 
TRY. Interestingly the code also calls get_selection but without passing 
the state...

This is a more interesting case as the source's subdev state is actually 
modified by the driver. The driver calls enum_frame_size, then modifies 
the state, then calls set_fmt. I'm not sure if that's really legal... 
The driver directly modifies the state, instead of calling set_selection?

> I think a helper function might be beneficial, but the real problem is with the
> comment: it does not explain why you shouldn't use it and what needs to be done
> to fix it.

That is true. There's no single answer to that. I think instead of 
trying to document that in the v4l2-subdev doc, we can enhance the 
comments in those three call sites to explain how it needs to be fixed.

> My suggestion would be to document that in the kerneldoc for this function in
> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> in the other drivers that use this).
> 
> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?

I think any driver that calls a source subdev's subdev ops which a 
subdev state as a parameter (the ones that used to take 
v4l2_subdev_pad_config), and does not call init_cfg is broken in the 
same way. With some grepping, I couldn't find anyone calling init_cfg. 
Finding those drivers which do those calls is a bit more difficult, but 
can be done with some efforts.

atmel-isc-base.c is one I found, and looks like it's doing something a 
bit similar to the 3) case above.

Perhaps the best way to solve this is just to remove the underscores 
from __v4l2_subdev_state_alloc, and change all the drivers which create 
temporary v4l2_subdev_states to use that (and the free) functions. And 
also create the helper macro which can be used in those cases where the 
call is simple (the state is not modified or accessed by the caller).

It would've been nice to keep __v4l2_subdev_state_alloc internal to the 
v4l2-subdev, but maybe the v4l2 drivers are not there yet. The non-MC 
drivers seem to be doing all kinds of interesting things.

  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
> On 29/06/2022 12:41, Hans Verkuil wrote:
> > Hi Marek, Tomi, Laurent,
> > 
> > On 27/06/2022 19:41, Marek Vasut wrote:
> >> Any local subdev state should be allocated and free'd using
> >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> >> takes care of calling .init_cfg() subdev op. Without this,
> >> subdev internal state might be uninitialized by the time
> >> any other subdev op is called.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Alain Volmat <alain.volmat@foss.st.com>
> >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> >> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> >> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> >> Cc: linux-stm32@st-md-mailman.stormreply.com
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> ---
> >> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
> >> ---
> >>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
> >>   1 file changed, 37 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> index c604d672c2156..c68d32931b277 100644
> >> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
> >>   			struct dcmi_framesize *sd_framesize)
> >>   {
> >>   	const struct dcmi_format *sd_fmt;
> >> +	static struct lock_class_key key;
> >>   	struct dcmi_framesize sd_fsize;
> >>   	struct v4l2_pix_format *pix = &f->fmt.pix;
> >> -	struct v4l2_subdev_pad_config pad_cfg;
> >> -	struct v4l2_subdev_state pad_state = {
> >> -		.pads = &pad_cfg
> >> -		};
> >> +	struct v4l2_subdev_state *sd_state;
> >>   	struct v4l2_subdev_format format = {
> >>   		.which = V4L2_SUBDEV_FORMAT_TRY,
> >>   	};
> >>   	bool do_crop;
> >>   	int ret;
> >>   
> >> +	/*
> >> +	 * FIXME: Drop this call, drivers are not supposed to use
> >> +	 * __v4l2_subdev_state_alloc().
> >> +	 */
> >> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> >> +	if (IS_ERR(sd_state))
> >> +		return PTR_ERR(sd_state);
> >> +
> > 
> > I've been reading the discussion for the v1 patch, and I seriously do not like this.
> 
> I don't like it either.
> 
> > My comments are not specifically for this patch, but for all cases where
> > __v4l2_subdev_state_alloc is called.
> 
> Just to emphasize it: afaics this is not an issue with the subdev state. 
> This driver was already broken. Before the subdev state change the fix 
> would have been to call source subdev's init_cfg. Now 
> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few 
> other inits).
> 
> > It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> > exactly trivial either.
> 
> Counting this one? I found 3 cases in v5.18-rc4:
> 
> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
> 
> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
> 
> Here, a helper macro which does the alloc would be a solution.
> 
> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
> 
> Allocates the state for storing internal active state.
> 
> Here, I think, the easiest fix is for the driver to use the subdev state 
> properly.
> 
> 3) drivers/staging/media/tegra-video/vi.c:
> 
> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt, 
> TRY. Interestingly the code also calls get_selection but without passing 
> the state...
> 
> This is a more interesting case as the source's subdev state is actually 
> modified by the driver. The driver calls enum_frame_size, then modifies 
> the state, then calls set_fmt. I'm not sure if that's really legal... 
> The driver directly modifies the state, instead of calling set_selection?
> 
> > I think a helper function might be beneficial, but the real problem is with the
> > comment: it does not explain why you shouldn't use it and what needs to be done
> > to fix it.
> 
> That is true. There's no single answer to that. I think instead of 
> trying to document that in the v4l2-subdev doc, we can enhance the 
> comments in those three call sites to explain how it needs to be fixed.
> 
> > My suggestion would be to document that in the kerneldoc for this function in
> > media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> > in the other drivers that use this).
> > 
> > And another question: are more drivers affected by this? Is it possible to
> > find those and fix them all?
> 
> I think any driver that calls a source subdev's subdev ops which a 
> subdev state as a parameter (the ones that used to take 
> v4l2_subdev_pad_config), and does not call init_cfg is broken in the 
> same way. With some grepping, I couldn't find anyone calling init_cfg. 
> Finding those drivers which do those calls is a bit more difficult, but 
> can be done with some efforts.

A quick check identified the following files:

atmel-isc-base.c
atmel-isi.c
cxusb-analog.c
fimc-capture.c
mcam-core.c
pxa_camera.c
renesas-ceu.c
saa7134-empress.c
via-camera.c

A few drivers with more complex call patterns may be missing.

> atmel-isc-base.c is one I found, and looks like it's doing something a 
> bit similar to the 3) case above.
> 
> Perhaps the best way to solve this is just to remove the underscores 
> from __v4l2_subdev_state_alloc, and change all the drivers which create 
> temporary v4l2_subdev_states to use that (and the free) functions. And 
> also create the helper macro which can be used in those cases where the 
> call is simple (the state is not modified or accessed by the caller).

As long as we prevent any new driver from using that API, that's fine
with me.

> It would've been nice to keep __v4l2_subdev_state_alloc internal to the 
> v4l2-subdev, but maybe the v4l2 drivers are not there yet. The non-MC 
> drivers seem to be doing all kinds of interesting things.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
> On 29/06/2022 12:41, Hans Verkuil wrote:
> > Hi Marek, Tomi, Laurent,
> > 
> > On 27/06/2022 19:41, Marek Vasut wrote:
> >> Any local subdev state should be allocated and free'd using
> >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> >> takes care of calling .init_cfg() subdev op. Without this,
> >> subdev internal state might be uninitialized by the time
> >> any other subdev op is called.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Alain Volmat <alain.volmat@foss.st.com>
> >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> >> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> >> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> >> Cc: linux-stm32@st-md-mailman.stormreply.com
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> ---
> >> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
> >> ---
> >>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
> >>   1 file changed, 37 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> index c604d672c2156..c68d32931b277 100644
> >> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
> >>   			struct dcmi_framesize *sd_framesize)
> >>   {
> >>   	const struct dcmi_format *sd_fmt;
> >> +	static struct lock_class_key key;
> >>   	struct dcmi_framesize sd_fsize;
> >>   	struct v4l2_pix_format *pix = &f->fmt.pix;
> >> -	struct v4l2_subdev_pad_config pad_cfg;
> >> -	struct v4l2_subdev_state pad_state = {
> >> -		.pads = &pad_cfg
> >> -		};
> >> +	struct v4l2_subdev_state *sd_state;
> >>   	struct v4l2_subdev_format format = {
> >>   		.which = V4L2_SUBDEV_FORMAT_TRY,
> >>   	};
> >>   	bool do_crop;
> >>   	int ret;
> >>   
> >> +	/*
> >> +	 * FIXME: Drop this call, drivers are not supposed to use
> >> +	 * __v4l2_subdev_state_alloc().
> >> +	 */
> >> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> >> +	if (IS_ERR(sd_state))
> >> +		return PTR_ERR(sd_state);
> >> +
> > 
> > I've been reading the discussion for the v1 patch, and I seriously do not like this.
> 
> I don't like it either.
> 
> > My comments are not specifically for this patch, but for all cases where
> > __v4l2_subdev_state_alloc is called.
> 
> Just to emphasize it: afaics this is not an issue with the subdev state. 
> This driver was already broken. Before the subdev state change the fix 
> would have been to call source subdev's init_cfg. Now 
> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few 
> other inits).
> 
> > It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> > exactly trivial either.
> 
> Counting this one? I found 3 cases in v5.18-rc4:
> 
> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
> 
> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
> 
> Here, a helper macro which does the alloc would be a solution.
> 
> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
> 
> Allocates the state for storing internal active state.
> 
> Here, I think, the easiest fix is for the driver to use the subdev state 
> properly.
> 
> 3) drivers/staging/media/tegra-video/vi.c:
> 
> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt, 
> TRY. Interestingly the code also calls get_selection but without passing 
> the state...
> 
> This is a more interesting case as the source's subdev state is actually 
> modified by the driver. The driver calls enum_frame_size, then modifies 
> the state, then calls set_fmt. I'm not sure if that's really legal... 
> The driver directly modifies the state, instead of calling set_selection?
> 
> > I think a helper function might be beneficial, but the real problem is with the
> > comment: it does not explain why you shouldn't use it and what needs to be done
> > to fix it.
> 
> That is true. There's no single answer to that. I think instead of 
> trying to document that in the v4l2-subdev doc, we can enhance the 
> comments in those three call sites to explain how it needs to be fixed.
> 
> > My suggestion would be to document that in the kerneldoc for this function in
> > media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> > in the other drivers that use this).
> > 
> > And another question: are more drivers affected by this? Is it possible to
> > find those and fix them all?
> 
> I think any driver that calls a source subdev's subdev ops which a 
> subdev state as a parameter (the ones that used to take 
> v4l2_subdev_pad_config), and does not call init_cfg is broken in the 
> same way. With some grepping, I couldn't find anyone calling init_cfg. 
> Finding those drivers which do those calls is a bit more difficult, but 
> can be done with some efforts.

A quick check identified the following files:

atmel-isc-base.c
atmel-isi.c
cxusb-analog.c
fimc-capture.c
mcam-core.c
pxa_camera.c
renesas-ceu.c
saa7134-empress.c
via-camera.c

A few drivers with more complex call patterns may be missing.

> atmel-isc-base.c is one I found, and looks like it's doing something a 
> bit similar to the 3) case above.
> 
> Perhaps the best way to solve this is just to remove the underscores 
> from __v4l2_subdev_state_alloc, and change all the drivers which create 
> temporary v4l2_subdev_states to use that (and the free) functions. And 
> also create the helper macro which can be used in those cases where the 
> call is simple (the state is not modified or accessed by the caller).

As long as we prevent any new driver from using that API, that's fine
with me.

> It would've been nice to keep __v4l2_subdev_state_alloc internal to the 
> v4l2-subdev, but maybe the v4l2 drivers are not there yet. The non-MC 
> drivers seem to be doing all kinds of interesting things.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 15:20, Laurent Pinchart wrote:
> On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
>> On 29/06/2022 12:41, Hans Verkuil wrote:
>>> Hi Marek, Tomi, Laurent,
>>>
>>> On 27/06/2022 19:41, Marek Vasut wrote:
>>>> Any local subdev state should be allocated and free'd using
>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>> subdev internal state might be uninitialized by the time
>>>> any other subdev op is called.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>>>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> ---
>>>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>>>> ---
>>>>    drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>>    1 file changed, 37 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> index c604d672c2156..c68d32931b277 100644
>>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>>    			struct dcmi_framesize *sd_framesize)
>>>>    {
>>>>    	const struct dcmi_format *sd_fmt;
>>>> +	static struct lock_class_key key;
>>>>    	struct dcmi_framesize sd_fsize;
>>>>    	struct v4l2_pix_format *pix = &f->fmt.pix;
>>>> -	struct v4l2_subdev_pad_config pad_cfg;
>>>> -	struct v4l2_subdev_state pad_state = {
>>>> -		.pads = &pad_cfg
>>>> -		};
>>>> +	struct v4l2_subdev_state *sd_state;
>>>>    	struct v4l2_subdev_format format = {
>>>>    		.which = V4L2_SUBDEV_FORMAT_TRY,
>>>>    	};
>>>>    	bool do_crop;
>>>>    	int ret;
>>>>    
>>>> +	/*
>>>> +	 * FIXME: Drop this call, drivers are not supposed to use
>>>> +	 * __v4l2_subdev_state_alloc().
>>>> +	 */
>>>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>>> +	if (IS_ERR(sd_state))
>>>> +		return PTR_ERR(sd_state);
>>>> +
>>>
>>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>
>> I don't like it either.
>>
>>> My comments are not specifically for this patch, but for all cases where
>>> __v4l2_subdev_state_alloc is called.
>>
>> Just to emphasize it: afaics this is not an issue with the subdev state.
>> This driver was already broken. Before the subdev state change the fix
>> would have been to call source subdev's init_cfg. Now
>> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few
>> other inits).
>>
>>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>>> exactly trivial either.
>>
>> Counting this one? I found 3 cases in v5.18-rc4:
>>
>> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
>>
>> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
>>
>> Here, a helper macro which does the alloc would be a solution.
>>
>> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
>>
>> Allocates the state for storing internal active state.
>>
>> Here, I think, the easiest fix is for the driver to use the subdev state
>> properly.
>>
>> 3) drivers/staging/media/tegra-video/vi.c:
>>
>> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt,
>> TRY. Interestingly the code also calls get_selection but without passing
>> the state...
>>
>> This is a more interesting case as the source's subdev state is actually
>> modified by the driver. The driver calls enum_frame_size, then modifies
>> the state, then calls set_fmt. I'm not sure if that's really legal...
>> The driver directly modifies the state, instead of calling set_selection?
>>
>>> I think a helper function might be beneficial, but the real problem is with the
>>> comment: it does not explain why you shouldn't use it and what needs to be done
>>> to fix it.
>>
>> That is true. There's no single answer to that. I think instead of
>> trying to document that in the v4l2-subdev doc, we can enhance the
>> comments in those three call sites to explain how it needs to be fixed.
>>
>>> My suggestion would be to document that in the kerneldoc for this function in
>>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>>> in the other drivers that use this).
>>>
>>> And another question: are more drivers affected by this? Is it possible to
>>> find those and fix them all?
>>
>> I think any driver that calls a source subdev's subdev ops which a
>> subdev state as a parameter (the ones that used to take
>> v4l2_subdev_pad_config), and does not call init_cfg is broken in the
>> same way. With some grepping, I couldn't find anyone calling init_cfg.
>> Finding those drivers which do those calls is a bit more difficult, but
>> can be done with some efforts.
> 
> A quick check identified the following files:
> 
> atmel-isc-base.c
> atmel-isi.c
> cxusb-analog.c
> fimc-capture.c
> mcam-core.c
> pxa_camera.c
> renesas-ceu.c
> saa7134-empress.c
> via-camera.c
> 
> A few drivers with more complex call patterns may be missing.
> 
>> atmel-isc-base.c is one I found, and looks like it's doing something a
>> bit similar to the 3) case above.
>>
>> Perhaps the best way to solve this is just to remove the underscores
>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>> temporary v4l2_subdev_states to use that (and the free) functions. And
>> also create the helper macro which can be used in those cases where the
>> call is simple (the state is not modified or accessed by the caller).
> 
> As long as we prevent any new driver from using that API, that's fine
> with me.

An alternative would be to keep the v4l2_subdev_state as a local 
variable (allocated in the stack), and add a new function, 
v4l2_subdev_state_local_init() or such. The function would initialize 
the given state, expecting the allocatable fields to be already 
allocated (state->pads, which in the above cases points to another local 
variable, i.e. stack).

This would prevent the need of a free call, which, while not complex as 
such, might cause a bigger amount of changes in some cases to handle the 
error paths correctly.

Of course, if the above-mentioned macro works, then that's the easiest 
solution. But that won't work for all drivers.

  Tomi

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 15:20, Laurent Pinchart wrote:
> On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
>> On 29/06/2022 12:41, Hans Verkuil wrote:
>>> Hi Marek, Tomi, Laurent,
>>>
>>> On 27/06/2022 19:41, Marek Vasut wrote:
>>>> Any local subdev state should be allocated and free'd using
>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>> subdev internal state might be uninitialized by the time
>>>> any other subdev op is called.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>>>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> ---
>>>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>>>> ---
>>>>    drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>>    1 file changed, 37 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> index c604d672c2156..c68d32931b277 100644
>>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>>    			struct dcmi_framesize *sd_framesize)
>>>>    {
>>>>    	const struct dcmi_format *sd_fmt;
>>>> +	static struct lock_class_key key;
>>>>    	struct dcmi_framesize sd_fsize;
>>>>    	struct v4l2_pix_format *pix = &f->fmt.pix;
>>>> -	struct v4l2_subdev_pad_config pad_cfg;
>>>> -	struct v4l2_subdev_state pad_state = {
>>>> -		.pads = &pad_cfg
>>>> -		};
>>>> +	struct v4l2_subdev_state *sd_state;
>>>>    	struct v4l2_subdev_format format = {
>>>>    		.which = V4L2_SUBDEV_FORMAT_TRY,
>>>>    	};
>>>>    	bool do_crop;
>>>>    	int ret;
>>>>    
>>>> +	/*
>>>> +	 * FIXME: Drop this call, drivers are not supposed to use
>>>> +	 * __v4l2_subdev_state_alloc().
>>>> +	 */
>>>> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>>> +	if (IS_ERR(sd_state))
>>>> +		return PTR_ERR(sd_state);
>>>> +
>>>
>>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>
>> I don't like it either.
>>
>>> My comments are not specifically for this patch, but for all cases where
>>> __v4l2_subdev_state_alloc is called.
>>
>> Just to emphasize it: afaics this is not an issue with the subdev state.
>> This driver was already broken. Before the subdev state change the fix
>> would have been to call source subdev's init_cfg. Now
>> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few
>> other inits).
>>
>>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>>> exactly trivial either.
>>
>> Counting this one? I found 3 cases in v5.18-rc4:
>>
>> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
>>
>> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
>>
>> Here, a helper macro which does the alloc would be a solution.
>>
>> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
>>
>> Allocates the state for storing internal active state.
>>
>> Here, I think, the easiest fix is for the driver to use the subdev state
>> properly.
>>
>> 3) drivers/staging/media/tegra-video/vi.c:
>>
>> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt,
>> TRY. Interestingly the code also calls get_selection but without passing
>> the state...
>>
>> This is a more interesting case as the source's subdev state is actually
>> modified by the driver. The driver calls enum_frame_size, then modifies
>> the state, then calls set_fmt. I'm not sure if that's really legal...
>> The driver directly modifies the state, instead of calling set_selection?
>>
>>> I think a helper function might be beneficial, but the real problem is with the
>>> comment: it does not explain why you shouldn't use it and what needs to be done
>>> to fix it.
>>
>> That is true. There's no single answer to that. I think instead of
>> trying to document that in the v4l2-subdev doc, we can enhance the
>> comments in those three call sites to explain how it needs to be fixed.
>>
>>> My suggestion would be to document that in the kerneldoc for this function in
>>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>>> in the other drivers that use this).
>>>
>>> And another question: are more drivers affected by this? Is it possible to
>>> find those and fix them all?
>>
>> I think any driver that calls a source subdev's subdev ops which a
>> subdev state as a parameter (the ones that used to take
>> v4l2_subdev_pad_config), and does not call init_cfg is broken in the
>> same way. With some grepping, I couldn't find anyone calling init_cfg.
>> Finding those drivers which do those calls is a bit more difficult, but
>> can be done with some efforts.
> 
> A quick check identified the following files:
> 
> atmel-isc-base.c
> atmel-isi.c
> cxusb-analog.c
> fimc-capture.c
> mcam-core.c
> pxa_camera.c
> renesas-ceu.c
> saa7134-empress.c
> via-camera.c
> 
> A few drivers with more complex call patterns may be missing.
> 
>> atmel-isc-base.c is one I found, and looks like it's doing something a
>> bit similar to the 3) case above.
>>
>> Perhaps the best way to solve this is just to remove the underscores
>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>> temporary v4l2_subdev_states to use that (and the free) functions. And
>> also create the helper macro which can be used in those cases where the
>> call is simple (the state is not modified or accessed by the caller).
> 
> As long as we prevent any new driver from using that API, that's fine
> with me.

An alternative would be to keep the v4l2_subdev_state as a local 
variable (allocated in the stack), and add a new function, 
v4l2_subdev_state_local_init() or such. The function would initialize 
the given state, expecting the allocatable fields to be already 
allocated (state->pads, which in the above cases points to another local 
variable, i.e. stack).

This would prevent the need of a free call, which, while not complex as 
such, might cause a bigger amount of changes in some cases to handle the 
error paths correctly.

Of course, if the above-mentioned macro works, then that's the easiest 
solution. But that won't work for all drivers.

  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 14:26, Tomi Valkeinen wrote:

[...]

>>> Perhaps the best way to solve this is just to remove the underscores
>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>> also create the helper macro which can be used in those cases where the
>>> call is simple (the state is not modified or accessed by the caller).
>>
>> As long as we prevent any new driver from using that API, that's fine
>> with me.
> 
> An alternative would be to keep the v4l2_subdev_state as a local 
> variable (allocated in the stack), and add a new function, 
> v4l2_subdev_state_local_init() or such. The function would initialize 
> the given state, expecting the allocatable fields to be already 
> allocated (state->pads, which in the above cases points to another local 
> variable, i.e. stack).
> 
> This would prevent the need of a free call, which, while not complex as 
> such, might cause a bigger amount of changes in some cases to handle the 
> error paths correctly.
> 
> Of course, if the above-mentioned macro works, then that's the easiest 
> solution. But that won't work for all drivers.

Don't you think a driver fix shouldn't involve "rework the subsystem" 
requirement to be applicable ?

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 14:26, Tomi Valkeinen wrote:

[...]

>>> Perhaps the best way to solve this is just to remove the underscores
>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>> also create the helper macro which can be used in those cases where the
>>> call is simple (the state is not modified or accessed by the caller).
>>
>> As long as we prevent any new driver from using that API, that's fine
>> with me.
> 
> An alternative would be to keep the v4l2_subdev_state as a local 
> variable (allocated in the stack), and add a new function, 
> v4l2_subdev_state_local_init() or such. The function would initialize 
> the given state, expecting the allocatable fields to be already 
> allocated (state->pads, which in the above cases points to another local 
> variable, i.e. stack).
> 
> This would prevent the need of a free call, which, while not complex as 
> such, might cause a bigger amount of changes in some cases to handle the 
> error paths correctly.
> 
> Of course, if the above-mentioned macro works, then that's the easiest 
> solution. But that won't work for all drivers.

Don't you think a driver fix shouldn't involve "rework the subsystem" 
requirement to be applicable ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

On 29/06/2022 14:26, Tomi Valkeinen wrote:
> On 29/06/2022 15:20, Laurent Pinchart wrote:
>> On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
>>> On 29/06/2022 12:41, Hans Verkuil wrote:
>>>> Hi Marek, Tomi, Laurent,
>>>>
>>>> On 27/06/2022 19:41, Marek Vasut wrote:
>>>>> Any local subdev state should be allocated and free'd using
>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>> subdev internal state might be uninitialized by the time
>>>>> any other subdev op is called.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>>>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>>>>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>>>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> ---
>>>>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>>>>> ---
>>>>>    drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>>>    1 file changed, 37 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> index c604d672c2156..c68d32931b277 100644
>>>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>>>                struct dcmi_framesize *sd_framesize)
>>>>>    {
>>>>>        const struct dcmi_format *sd_fmt;
>>>>> +    static struct lock_class_key key;
>>>>>        struct dcmi_framesize sd_fsize;
>>>>>        struct v4l2_pix_format *pix = &f->fmt.pix;
>>>>> -    struct v4l2_subdev_pad_config pad_cfg;
>>>>> -    struct v4l2_subdev_state pad_state = {
>>>>> -        .pads = &pad_cfg
>>>>> -        };
>>>>> +    struct v4l2_subdev_state *sd_state;
>>>>>        struct v4l2_subdev_format format = {
>>>>>            .which = V4L2_SUBDEV_FORMAT_TRY,
>>>>>        };
>>>>>        bool do_crop;
>>>>>        int ret;
>>>>>    +    /*
>>>>> +     * FIXME: Drop this call, drivers are not supposed to use
>>>>> +     * __v4l2_subdev_state_alloc().
>>>>> +     */
>>>>> +    sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>>>> +    if (IS_ERR(sd_state))
>>>>> +        return PTR_ERR(sd_state);
>>>>> +
>>>>
>>>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>>
>>> I don't like it either.
>>>
>>>> My comments are not specifically for this patch, but for all cases where
>>>> __v4l2_subdev_state_alloc is called.
>>>
>>> Just to emphasize it: afaics this is not an issue with the subdev state.
>>> This driver was already broken. Before the subdev state change the fix
>>> would have been to call source subdev's init_cfg. Now
>>> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few
>>> other inits).
>>>
>>>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>>>> exactly trivial either.
>>>
>>> Counting this one? I found 3 cases in v5.18-rc4:
>>>
>>> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
>>>
>>> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
>>>
>>> Here, a helper macro which does the alloc would be a solution.
>>>
>>> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
>>>
>>> Allocates the state for storing internal active state.
>>>
>>> Here, I think, the easiest fix is for the driver to use the subdev state
>>> properly.
>>>
>>> 3) drivers/staging/media/tegra-video/vi.c:
>>>
>>> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt,
>>> TRY. Interestingly the code also calls get_selection but without passing
>>> the state...
>>>
>>> This is a more interesting case as the source's subdev state is actually
>>> modified by the driver. The driver calls enum_frame_size, then modifies
>>> the state, then calls set_fmt. I'm not sure if that's really legal...
>>> The driver directly modifies the state, instead of calling set_selection?
>>>
>>>> I think a helper function might be beneficial, but the real problem is with the
>>>> comment: it does not explain why you shouldn't use it and what needs to be done
>>>> to fix it.
>>>
>>> That is true. There's no single answer to that. I think instead of
>>> trying to document that in the v4l2-subdev doc, we can enhance the
>>> comments in those three call sites to explain how it needs to be fixed.
>>>
>>>> My suggestion would be to document that in the kerneldoc for this function in
>>>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>>>> in the other drivers that use this).
>>>>
>>>> And another question: are more drivers affected by this? Is it possible to
>>>> find those and fix them all?
>>>
>>> I think any driver that calls a source subdev's subdev ops which a
>>> subdev state as a parameter (the ones that used to take
>>> v4l2_subdev_pad_config), and does not call init_cfg is broken in the
>>> same way. With some grepping, I couldn't find anyone calling init_cfg.
>>> Finding those drivers which do those calls is a bit more difficult, but
>>> can be done with some efforts.
>>
>> A quick check identified the following files:
>>
>> atmel-isc-base.c
>> atmel-isi.c
>> cxusb-analog.c
>> fimc-capture.c
>> mcam-core.c
>> pxa_camera.c
>> renesas-ceu.c
>> saa7134-empress.c
>> via-camera.c
>>
>> A few drivers with more complex call patterns may be missing.
>>
>>> atmel-isc-base.c is one I found, and looks like it's doing something a
>>> bit similar to the 3) case above.
>>>
>>> Perhaps the best way to solve this is just to remove the underscores
>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>> also create the helper macro which can be used in those cases where the
>>> call is simple (the state is not modified or accessed by the caller).
>>
>> As long as we prevent any new driver from using that API, that's fine
>> with me.
> 
> An alternative would be to keep the v4l2_subdev_state as a local variable (allocated in the stack), and add a new function, v4l2_subdev_state_local_init() or such. The function would initialize the
> given state, expecting the allocatable fields to be already allocated (state->pads, which in the above cases points to another local variable, i.e. stack).

Certainly in the case of this stm32 driver, that seems much closer to the original code
so also easier to understand.

The reason I am blocking this patch now is that I want to prevent seeing these terrible
FIXMEs all over the place. It was in hindsight a mistake to accept it in the first place,
so we need something better in place first before we fix this (and other) drivers.

I do apologize to Marek for that, but this issue highlighted a real problem that needs
addressing first.

Regards,

	Hans

> 
> This would prevent the need of a free call, which, while not complex as such, might cause a bigger amount of changes in some cases to handle the error paths correctly.
> 
> Of course, if the above-mentioned macro works, then that's the easiest solution. But that won't work for all drivers.
> 
>  Tomi

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hans Verkuil]

On 29/06/2022 14:26, Tomi Valkeinen wrote:
> On 29/06/2022 15:20, Laurent Pinchart wrote:
>> On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
>>> On 29/06/2022 12:41, Hans Verkuil wrote:
>>>> Hi Marek, Tomi, Laurent,
>>>>
>>>> On 27/06/2022 19:41, Marek Vasut wrote:
>>>>> Any local subdev state should be allocated and free'd using
>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>> subdev internal state might be uninitialized by the time
>>>>> any other subdev op is called.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Cc: Alain Volmat <alain.volmat@foss.st.com>
>>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>>>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>>>>> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
>>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
>>>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> ---
>>>>> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
>>>>> ---
>>>>>    drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>>>>>    1 file changed, 37 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> index c604d672c2156..c68d32931b277 100644
>>>>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>>>>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>>>>>                struct dcmi_framesize *sd_framesize)
>>>>>    {
>>>>>        const struct dcmi_format *sd_fmt;
>>>>> +    static struct lock_class_key key;
>>>>>        struct dcmi_framesize sd_fsize;
>>>>>        struct v4l2_pix_format *pix = &f->fmt.pix;
>>>>> -    struct v4l2_subdev_pad_config pad_cfg;
>>>>> -    struct v4l2_subdev_state pad_state = {
>>>>> -        .pads = &pad_cfg
>>>>> -        };
>>>>> +    struct v4l2_subdev_state *sd_state;
>>>>>        struct v4l2_subdev_format format = {
>>>>>            .which = V4L2_SUBDEV_FORMAT_TRY,
>>>>>        };
>>>>>        bool do_crop;
>>>>>        int ret;
>>>>>    +    /*
>>>>> +     * FIXME: Drop this call, drivers are not supposed to use
>>>>> +     * __v4l2_subdev_state_alloc().
>>>>> +     */
>>>>> +    sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>>>>> +    if (IS_ERR(sd_state))
>>>>> +        return PTR_ERR(sd_state);
>>>>> +
>>>>
>>>> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>>>
>>> I don't like it either.
>>>
>>>> My comments are not specifically for this patch, but for all cases where
>>>> __v4l2_subdev_state_alloc is called.
>>>
>>> Just to emphasize it: afaics this is not an issue with the subdev state.
>>> This driver was already broken. Before the subdev state change the fix
>>> would have been to call source subdev's init_cfg. Now
>>> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few
>>> other inits).
>>>
>>>> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
>>>> exactly trivial either.
>>>
>>> Counting this one? I found 3 cases in v5.18-rc4:
>>>
>>> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
>>>
>>> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
>>>
>>> Here, a helper macro which does the alloc would be a solution.
>>>
>>> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
>>>
>>> Allocates the state for storing internal active state.
>>>
>>> Here, I think, the easiest fix is for the driver to use the subdev state
>>> properly.
>>>
>>> 3) drivers/staging/media/tegra-video/vi.c:
>>>
>>> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt,
>>> TRY. Interestingly the code also calls get_selection but without passing
>>> the state...
>>>
>>> This is a more interesting case as the source's subdev state is actually
>>> modified by the driver. The driver calls enum_frame_size, then modifies
>>> the state, then calls set_fmt. I'm not sure if that's really legal...
>>> The driver directly modifies the state, instead of calling set_selection?
>>>
>>>> I think a helper function might be beneficial, but the real problem is with the
>>>> comment: it does not explain why you shouldn't use it and what needs to be done
>>>> to fix it.
>>>
>>> That is true. There's no single answer to that. I think instead of
>>> trying to document that in the v4l2-subdev doc, we can enhance the
>>> comments in those three call sites to explain how it needs to be fixed.
>>>
>>>> My suggestion would be to document that in the kerneldoc for this function in
>>>> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
>>>> in the other drivers that use this).
>>>>
>>>> And another question: are more drivers affected by this? Is it possible to
>>>> find those and fix them all?
>>>
>>> I think any driver that calls a source subdev's subdev ops which a
>>> subdev state as a parameter (the ones that used to take
>>> v4l2_subdev_pad_config), and does not call init_cfg is broken in the
>>> same way. With some grepping, I couldn't find anyone calling init_cfg.
>>> Finding those drivers which do those calls is a bit more difficult, but
>>> can be done with some efforts.
>>
>> A quick check identified the following files:
>>
>> atmel-isc-base.c
>> atmel-isi.c
>> cxusb-analog.c
>> fimc-capture.c
>> mcam-core.c
>> pxa_camera.c
>> renesas-ceu.c
>> saa7134-empress.c
>> via-camera.c
>>
>> A few drivers with more complex call patterns may be missing.
>>
>>> atmel-isc-base.c is one I found, and looks like it's doing something a
>>> bit similar to the 3) case above.
>>>
>>> Perhaps the best way to solve this is just to remove the underscores
>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>> also create the helper macro which can be used in those cases where the
>>> call is simple (the state is not modified or accessed by the caller).
>>
>> As long as we prevent any new driver from using that API, that's fine
>> with me.
> 
> An alternative would be to keep the v4l2_subdev_state as a local variable (allocated in the stack), and add a new function, v4l2_subdev_state_local_init() or such. The function would initialize the
> given state, expecting the allocatable fields to be already allocated (state->pads, which in the above cases points to another local variable, i.e. stack).

Certainly in the case of this stm32 driver, that seems much closer to the original code
so also easier to understand.

The reason I am blocking this patch now is that I want to prevent seeing these terrible
FIXMEs all over the place. It was in hindsight a mistake to accept it in the first place,
so we need something better in place first before we fix this (and other) drivers.

I do apologize to Marek for that, but this issue highlighted a real problem that needs
addressing first.

Regards,

	Hans

> 
> This would prevent the need of a free call, which, while not complex as such, might cause a bigger amount of changes in some cases to handle the error paths correctly.
> 
> Of course, if the above-mentioned macro works, then that's the easiest solution. But that won't work for all drivers.
> 
>  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 15:39, Marek Vasut wrote:
> On 6/29/22 14:26, Tomi Valkeinen wrote:
> 
> [...]
> 
>>>> Perhaps the best way to solve this is just to remove the underscores
>>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>>> also create the helper macro which can be used in those cases where the
>>>> call is simple (the state is not modified or accessed by the caller).
>>>
>>> As long as we prevent any new driver from using that API, that's fine
>>> with me.
>>
>> An alternative would be to keep the v4l2_subdev_state as a local 
>> variable (allocated in the stack), and add a new function, 
>> v4l2_subdev_state_local_init() or such. The function would initialize 
>> the given state, expecting the allocatable fields to be already 
>> allocated (state->pads, which in the above cases points to another 
>> local variable, i.e. stack).
>>
>> This would prevent the need of a free call, which, while not complex 
>> as such, might cause a bigger amount of changes in some cases to 
>> handle the error paths correctly.
>>
>> Of course, if the above-mentioned macro works, then that's the easiest 
>> solution. But that won't work for all drivers.
> 
> Don't you think a driver fix shouldn't involve "rework the subsystem" 
> requirement to be applicable ?

No, but we should think what's the best way to do the fix, if the fix
is controversial. Otherwise we might just break things even worse.
Adding the macro seems like a much better way, and far from "rework the
subsystem". Granted, this was just a quick edit without testing so it may
fail miserably...

Can you try this out?

Also, a bit unrelated thing: dcmi_get_sensor_format gets the active format
from the source subdev, but dcmi_set_sensor_format sets the try format. That
sounds like a bug. Is it on purpose?


diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index 09a743cd7004..eb831b5932e7 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -999,10 +999,6 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
  	const struct dcmi_format *sd_fmt;
  	struct dcmi_framesize sd_fsize;
  	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
  	struct v4l2_subdev_format format = {
  		.which = V4L2_SUBDEV_FORMAT_TRY,
  	};
@@ -1037,8 +1033,7 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
  	}
  
  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
@@ -1187,10 +1182,6 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	struct v4l2_subdev_format format = {
  		.which = V4L2_SUBDEV_FORMAT_TRY,
  	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
  	int ret;
  
  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
@@ -1203,8 +1194,7 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	}
  
  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b661e1817470..68676d173047 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1433,6 +1433,19 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
  		__result;						\
  	})
  
+#define v4l2_subdev_call_state_try(sd, o, f, args...)                 \
+	({                                                            \
+		int __result;                                         \
+		static struct lock_class_key __key;                   \
+		const char *name = KBUILD_BASENAME                    \
+			":" __stringify(__LINE__) ":state->lock";     \
+		struct v4l2_subdev_state *state =                     \
+			__v4l2_subdev_state_alloc(sd, name, &__key);  \
+		__result = v4l2_subdev_call(sd, o, f, state, ##args); \
+		__v4l2_subdev_state_free(state);                      \
+		__result;                                             \
+	})
+
  /**
   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
   *


  Tomi

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 29/06/2022 15:39, Marek Vasut wrote:
> On 6/29/22 14:26, Tomi Valkeinen wrote:
> 
> [...]
> 
>>>> Perhaps the best way to solve this is just to remove the underscores
>>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>>> also create the helper macro which can be used in those cases where the
>>>> call is simple (the state is not modified or accessed by the caller).
>>>
>>> As long as we prevent any new driver from using that API, that's fine
>>> with me.
>>
>> An alternative would be to keep the v4l2_subdev_state as a local 
>> variable (allocated in the stack), and add a new function, 
>> v4l2_subdev_state_local_init() or such. The function would initialize 
>> the given state, expecting the allocatable fields to be already 
>> allocated (state->pads, which in the above cases points to another 
>> local variable, i.e. stack).
>>
>> This would prevent the need of a free call, which, while not complex 
>> as such, might cause a bigger amount of changes in some cases to 
>> handle the error paths correctly.
>>
>> Of course, if the above-mentioned macro works, then that's the easiest 
>> solution. But that won't work for all drivers.
> 
> Don't you think a driver fix shouldn't involve "rework the subsystem" 
> requirement to be applicable ?

No, but we should think what's the best way to do the fix, if the fix
is controversial. Otherwise we might just break things even worse.
Adding the macro seems like a much better way, and far from "rework the
subsystem". Granted, this was just a quick edit without testing so it may
fail miserably...

Can you try this out?

Also, a bit unrelated thing: dcmi_get_sensor_format gets the active format
from the source subdev, but dcmi_set_sensor_format sets the try format. That
sounds like a bug. Is it on purpose?


diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index 09a743cd7004..eb831b5932e7 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -999,10 +999,6 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
  	const struct dcmi_format *sd_fmt;
  	struct dcmi_framesize sd_fsize;
  	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
  	struct v4l2_subdev_format format = {
  		.which = V4L2_SUBDEV_FORMAT_TRY,
  	};
@@ -1037,8 +1033,7 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
  	}
  
  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
@@ -1187,10 +1182,6 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	struct v4l2_subdev_format format = {
  		.which = V4L2_SUBDEV_FORMAT_TRY,
  	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
  	int ret;
  
  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
@@ -1203,8 +1194,7 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	}
  
  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b661e1817470..68676d173047 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1433,6 +1433,19 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
  		__result;						\
  	})
  
+#define v4l2_subdev_call_state_try(sd, o, f, args...)                 \
+	({                                                            \
+		int __result;                                         \
+		static struct lock_class_key __key;                   \
+		const char *name = KBUILD_BASENAME                    \
+			":" __stringify(__LINE__) ":state->lock";     \
+		struct v4l2_subdev_state *state =                     \
+			__v4l2_subdev_state_alloc(sd, name, &__key);  \
+		__result = v4l2_subdev_call(sd, o, f, state, ##args); \
+		__v4l2_subdev_state_free(state);                      \
+		__result;                                             \
+	})
+
  /**
   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
   *


  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 15:19, Tomi Valkeinen wrote:
> On 29/06/2022 15:39, Marek Vasut wrote:
>> On 6/29/22 14:26, Tomi Valkeinen wrote:
>>
>> [...]
>>
>>>>> Perhaps the best way to solve this is just to remove the underscores
>>>>> from __v4l2_subdev_state_alloc, and change all the drivers which 
>>>>> create
>>>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>>>> also create the helper macro which can be used in those cases where 
>>>>> the
>>>>> call is simple (the state is not modified or accessed by the caller).
>>>>
>>>> As long as we prevent any new driver from using that API, that's fine
>>>> with me.
>>>
>>> An alternative would be to keep the v4l2_subdev_state as a local 
>>> variable (allocated in the stack), and add a new function, 
>>> v4l2_subdev_state_local_init() or such. The function would initialize 
>>> the given state, expecting the allocatable fields to be already 
>>> allocated (state->pads, which in the above cases points to another 
>>> local variable, i.e. stack).
>>>
>>> This would prevent the need of a free call, which, while not complex 
>>> as such, might cause a bigger amount of changes in some cases to 
>>> handle the error paths correctly.
>>>
>>> Of course, if the above-mentioned macro works, then that's the 
>>> easiest solution. But that won't work for all drivers.
>>
>> Don't you think a driver fix shouldn't involve "rework the subsystem" 
>> requirement to be applicable ?
> 
> No, but we should think what's the best way to do the fix, if the fix
> is controversial. Otherwise we might just break things even worse.
> Adding the macro seems like a much better way, and far from "rework the
> subsystem". Granted, this was just a quick edit without testing so it may
> fail miserably...
> 
> Can you try this out?

It seems to work as well. How shall we proceed ?

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/29/22 15:19, Tomi Valkeinen wrote:
> On 29/06/2022 15:39, Marek Vasut wrote:
>> On 6/29/22 14:26, Tomi Valkeinen wrote:
>>
>> [...]
>>
>>>>> Perhaps the best way to solve this is just to remove the underscores
>>>>> from __v4l2_subdev_state_alloc, and change all the drivers which 
>>>>> create
>>>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>>>> also create the helper macro which can be used in those cases where 
>>>>> the
>>>>> call is simple (the state is not modified or accessed by the caller).
>>>>
>>>> As long as we prevent any new driver from using that API, that's fine
>>>> with me.
>>>
>>> An alternative would be to keep the v4l2_subdev_state as a local 
>>> variable (allocated in the stack), and add a new function, 
>>> v4l2_subdev_state_local_init() or such. The function would initialize 
>>> the given state, expecting the allocatable fields to be already 
>>> allocated (state->pads, which in the above cases points to another 
>>> local variable, i.e. stack).
>>>
>>> This would prevent the need of a free call, which, while not complex 
>>> as such, might cause a bigger amount of changes in some cases to 
>>> handle the error paths correctly.
>>>
>>> Of course, if the above-mentioned macro works, then that's the 
>>> easiest solution. But that won't work for all drivers.
>>
>> Don't you think a driver fix shouldn't involve "rework the subsystem" 
>> requirement to be applicable ?
> 
> No, but we should think what's the best way to do the fix, if the fix
> is controversial. Otherwise we might just break things even worse.
> Adding the macro seems like a much better way, and far from "rework the
> subsystem". Granted, this was just a quick edit without testing so it may
> fail miserably...
> 
> Can you try this out?

It seems to work as well. How shall we proceed ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 30/06/2022 03:31, Marek Vasut wrote:
> On 6/29/22 15:19, Tomi Valkeinen wrote:
>> On 29/06/2022 15:39, Marek Vasut wrote:
>>> On 6/29/22 14:26, Tomi Valkeinen wrote:
>>>
>>> [...]
>>>
>>>>>> Perhaps the best way to solve this is just to remove the underscores
>>>>>> from __v4l2_subdev_state_alloc, and change all the drivers which 
>>>>>> create
>>>>>> temporary v4l2_subdev_states to use that (and the free) functions. 
>>>>>> And
>>>>>> also create the helper macro which can be used in those cases 
>>>>>> where the
>>>>>> call is simple (the state is not modified or accessed by the caller).
>>>>>
>>>>> As long as we prevent any new driver from using that API, that's fine
>>>>> with me.
>>>>
>>>> An alternative would be to keep the v4l2_subdev_state as a local 
>>>> variable (allocated in the stack), and add a new function, 
>>>> v4l2_subdev_state_local_init() or such. The function would 
>>>> initialize the given state, expecting the allocatable fields to be 
>>>> already allocated (state->pads, which in the above cases points to 
>>>> another local variable, i.e. stack).
>>>>
>>>> This would prevent the need of a free call, which, while not complex 
>>>> as such, might cause a bigger amount of changes in some cases to 
>>>> handle the error paths correctly.
>>>>
>>>> Of course, if the above-mentioned macro works, then that's the 
>>>> easiest solution. But that won't work for all drivers.
>>>
>>> Don't you think a driver fix shouldn't involve "rework the subsystem" 
>>> requirement to be applicable ?
>>
>> No, but we should think what's the best way to do the fix, if the fix
>> is controversial. Otherwise we might just break things even worse.
>> Adding the macro seems like a much better way, and far from "rework the
>> subsystem". Granted, this was just a quick edit without testing so it may
>> fail miserably...
>>
>> Can you try this out?
> 
> It seems to work as well. How shall we proceed ?

I haven't had to look at this more closely, but I made proper patches of 
this and sent them for review. Note that they're not exactly the same as 
the diff here: the macro was missing lock and unlock for the subdev 
state, which I've added. Again, only compile tested.

  Tomi

Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 30/06/2022 03:31, Marek Vasut wrote:
> On 6/29/22 15:19, Tomi Valkeinen wrote:
>> On 29/06/2022 15:39, Marek Vasut wrote:
>>> On 6/29/22 14:26, Tomi Valkeinen wrote:
>>>
>>> [...]
>>>
>>>>>> Perhaps the best way to solve this is just to remove the underscores
>>>>>> from __v4l2_subdev_state_alloc, and change all the drivers which 
>>>>>> create
>>>>>> temporary v4l2_subdev_states to use that (and the free) functions. 
>>>>>> And
>>>>>> also create the helper macro which can be used in those cases 
>>>>>> where the
>>>>>> call is simple (the state is not modified or accessed by the caller).
>>>>>
>>>>> As long as we prevent any new driver from using that API, that's fine
>>>>> with me.
>>>>
>>>> An alternative would be to keep the v4l2_subdev_state as a local 
>>>> variable (allocated in the stack), and add a new function, 
>>>> v4l2_subdev_state_local_init() or such. The function would 
>>>> initialize the given state, expecting the allocatable fields to be 
>>>> already allocated (state->pads, which in the above cases points to 
>>>> another local variable, i.e. stack).
>>>>
>>>> This would prevent the need of a free call, which, while not complex 
>>>> as such, might cause a bigger amount of changes in some cases to 
>>>> handle the error paths correctly.
>>>>
>>>> Of course, if the above-mentioned macro works, then that's the 
>>>> easiest solution. But that won't work for all drivers.
>>>
>>> Don't you think a driver fix shouldn't involve "rework the subsystem" 
>>> requirement to be applicable ?
>>
>> No, but we should think what's the best way to do the fix, if the fix
>> is controversial. Otherwise we might just break things even worse.
>> Adding the macro seems like a much better way, and far from "rework the
>> subsystem". Granted, this was just a quick edit without testing so it may
>> fail miserably...
>>
>> Can you try this out?
> 
> It seems to work as well. How shall we proceed ?

I haven't had to look at this more closely, but I made proper patches of 
this and sent them for review. Note that they're not exactly the same as 
the diff here: the macro was missing lock and unlock for the subdev 
state, which I've added. Again, only compile tested.

  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel