Hi, thanks for the review. My responses below. > Hiremath, Vaibhav <hvaibhav@xxxxxx> wrote: > >> -----Original Message----- >> From: Pawel Osciak [mailto:p.osciak@xxxxxxxxxxx] >> Sent: Monday, April 19, 2010 6:00 PM >> To: linux-media@xxxxxxxxxxxxxxx >> Cc: p.osciak@xxxxxxxxxxx; m.szyprowski@xxxxxxxxxxx; >> kyungmin.park@xxxxxxxxxxx; Hiremath, Vaibhav >> Subject: [PATCH v4 1/2] v4l: Add memory-to-memory device helper framework >> for videobuf. >[Hiremath, Vaibhav] Add one line here. > Ok... [snip] >> +/** >> + * v4l2_m2m_querybuf() - multi-queue-aware QUERYBUF multiplexer >> + * >> + * See v4l2_m2m_mmap() documentation for details. >> + */ >> +int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx, >> + struct v4l2_buffer *buf) >> +{ >> + struct videobuf_queue *vq; >> + int ret; >> + >> + vq = v4l2_m2m_get_vq(m2m_ctx, buf->type); >> + ret = videobuf_querybuf(vq, buf); >> + >> + if (buf->memory == V4L2_MEMORY_MMAP >> + && vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) { >> + buf->m.offset += DST_QUEUE_OFF_BASE; >> + } >[Hiremath, Vaibhav] Don't you think we should check for ret value also here? Should it be something - > >if (!ret && buf->memory == V4L2_MEMORY_MMAP > && vq->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) { > buf->m.offset += DST_QUEUE_OFF_BASE; >} > I think it should stay like this. The offset should never be different depending on whether an error is being reported or not. The unmodified offset could confuse userspace applications or even conflict with the other buffer type (although in case of errors userspace should not be using those offsets anyway). [snip] >> +/** >> + * v4l2_m2m_dqbuf() - dequeue a source or destination buffer, depending on >> + * the type >> + */ >> +int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx, >> + struct v4l2_buffer *buf) >> +{ >> + struct videobuf_queue *vq; >> + >> + vq = v4l2_m2m_get_vq(m2m_ctx, buf->type); > > >[Hiremath, Vaibhav] Does it make sense to check the return value here? > Well, videobuf does not check it either. I mean, it would be important if there was a possibility that userspace passes malicious data. But a NULL here would mean a driver error. Best regards -- Pawel Osciak Linux Platform Group Samsung Poland R&D Center -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
