linux-kernel - Re: [PATCH v2 08/15] media: blackfin: bfin_capture: use vb2_ioctl

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]

Message-ID: <CAHG8p1CrBaD_Rk8tkzXg6HucQQQQNmJ-_rvEa8nUOX3QhKKGxQ@mail.gmail.com>
Date:	Tue, 3 Feb 2015 16:27:43 +0800
From:	Scott Jiang <scott.jiang.linux@...il.com>
To:	"Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc:	LMML <linux-media@...r.kernel.org>,
	adi-buildroot-devel@...ts.sourceforge.net,
	LKML <linux-kernel@...r.kernel.org>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>
Subject: Re: [PATCH v2 08/15] media: blackfin: bfin_capture: use vb2_ioctl_* helpers

Hi Lad,

2015-01-23 6:18 GMT+08:00 Lad, Prabhakar <prabhakar.csengg@...il.com>:
> this patch adds support to vb2_ioctl_* helpers.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@...il.com>
> ---
>  drivers/media/platform/blackfin/bfin_capture.c | 108 ++++++-------------------
>  1 file changed, 23 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
> index b2eeace..04b85e3 100644
> --- a/drivers/media/platform/blackfin/bfin_capture.c
> +++ b/drivers/media/platform/blackfin/bfin_capture.c
> @@ -272,15 +272,26 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
>         struct ppi_if *ppi = bcap_dev->ppi;
>         struct bcap_buffer *buf, *tmp;
>         struct ppi_params params;
> +       dma_addr_t addr;
>         int ret;
>
>         /* enable streamon on the sub device */
>         ret = v4l2_subdev_call(bcap_dev->sd, video, s_stream, 1);
>         if (ret && (ret != -ENOIOCTLCMD)) {
>                 v4l2_err(&bcap_dev->v4l2_dev, "stream on failed in subdev\n");
> +               bcap_dev->cur_frm = NULL;
>                 goto err;
>         }
>
> +       /* get the next frame from the dma queue */
> +       bcap_dev->cur_frm = list_entry(bcap_dev->dma_queue.next,
> +                                       struct bcap_buffer, list);
> +       /* remove buffer from the dma queue */
> +       list_del_init(&bcap_dev->cur_frm->list);
> +       addr = vb2_dma_contig_plane_dma_addr(&bcap_dev->cur_frm->vb, 0);
> +       /* update DMA address */
> +       ppi->ops->update_addr(ppi, (unsigned long)addr);
> +
>         /* set ppi params */
>         params.width = bcap_dev->fmt.width;
>         params.height = bcap_dev->fmt.height;
> @@ -320,6 +331,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
>                 goto err;
>         }
>
> +       /* enable ppi */
> +       ppi->ops->start(ppi);
> +
Still wrong here. You can't start ppi before request dma and irq. Also
it's not good to update dma address before request dma. Please
strictly follow the initial sequence in bcap_streamon() because the
order is important. That means you should put all functions in
bcap_start_streaming() before those in bcap_streamon().
And it seems you removed dma buffer check in bcap_streamon(). Yes, in
vb2_internal_streamon() it will check q->queued_count >=
q->min_buffers_needed to start streaming. But if the user doesn't
queue enough buffer, it will return success and set q->streaming = 1.
Is it really right here?

>         /* attach ppi DMA irq handler */
>         ret = ppi->ops->attach_irq(ppi, bcap_isr);
>         if (ret < 0) {
> @@ -334,6 +348,9 @@ static int bcap_start_streaming(struct vb2_queue *vq, unsigned int count)
>         return 0;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/