linux-kernel - Re: [PATCH] media: meson: vdec: esparser: check parsing state with hardware write pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Jan 2023 15:04:56 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Christian Hewitt <christianshewitt@...il.com>
Cc:     Neil Armstrong <neil.armstrong@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-media@...r.kernel.org, linux-amlogic@...ts.infradead.org,
        linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Neil Armstrong <narmstrong@...libre.com>
Subject: Re: [PATCH] media: meson: vdec: esparser: check parsing state with
 hardware write pointer

On Mon, Jan 23, 2023 at 09:43:00AM +0000, Christian Hewitt wrote:
> From: Neil Armstrong <narmstrong@...libre.com>
> 
> Also check the hardware write pointer to check if ES Parser has stalled.
> 

Presumably this is something which has happens in real life?  Say a user
has this hardware and is wondering if this patch fixes their bug, what
does this bug look like to them?

Please add a Fixes tag.

> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
> Signed-off-by: Christian Hewitt <christianshewitt@...il.com>
> ---
>  drivers/staging/media/meson/vdec/esparser.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index df5956c6141d..41b705f999b2 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -300,6 +300,7 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  	u32 num_dst_bufs = 0;
>  	u32 offset;
>  	u32 pad_size;
> +	u32 wp, wp2;
>  
>  	/*
>  	 * When max ref frame is held by VP9, this should be -= 3 to prevent a
> @@ -354,15 +355,21 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  	}
>  
>  	pad_size = esparser_pad_start_code(core, vb, payload_size);
> +	wp = amvdec_read_parser(core, PARSER_VIDEO_WP);
>  	ret = esparser_write_data(core, phy, payload_size + pad_size);
> +	wp2 = amvdec_read_parser(core, PARSER_VIDEO_WP);
>  

Why is check not done inside the esparser_write_data() function?  It
really feels like doing a write where nothing happens should be a bug
for both callers...

The esparser_write_data() function returns > 0 on success, 0 on timeout
(I guess timeout is an error-ish) and negative if the user presses
CTR-C.  There are no comments to explain the unusual returns.  Could we
clean this up so it just returns negatives on error and zero on success
or if not then let's add a comment explaining what's going on?

>  	if (ret <= 0) {
> -		dev_warn(core->dev, "esparser: input parsing error\n");
> -		amvdec_remove_ts(sess, vb->timestamp);
> -		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>  		amvdec_write_parser(core, PARSER_FETCH_CMD, 0);
>  
> -		return 0;
> +		if (ret < 0 || wp2 == wp) {

So in this patch if there is a timeout but something was written then
that's kind of a success path?

regards,
dan carpenter

> +			dev_err(core->dev, "esparser: input parsing error ret %d (%x <=> %x)\n",
> +				ret, wp, wp2);
> +			amvdec_remove_ts(sess, vb->timestamp);
> +			v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
> +
> +			return 0;
> +		}
>  	}
>  
>  	atomic_inc(&sess->esparser_queued_bufs);