On Fri, Jan 20, 2012 at 6:53 PM, Laurent Pinchart
[off-list ref] wrote:

Hi Summit,

Sorry for the late review. I know that this code is now in mainline, but I
still have a couple of comments. I'll send patches if you agree with them.

Hi Laurent,

Thanks for your review; apologies for being late in replying - I was
OoO for last couple of days.

On Monday 26 December 2011 10:23:15 Sumit Semwal wrote:

<snip>

quoted

[snip]

quoted

diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
new file mode 100644
index 0000000..e38ad24
--- /dev/null
+++ b/drivers/base/dma-buf.c

@@ -0,0 +1,291 @@

[snip]

quoted

+/**
+ * dma_buf_export - Creates a new dma_buf, and associates an anon file
+ * with this buffer, so it can be exported.
+ * Also connect the allocator specific data and ops to the buffer.
+ *
+ * @priv: ? ?[in] ? ?Attach private data of allocator to this buffer
+ * @ops: ? ? [in] ? ?Attach allocator-defined dma buf ops to the new buffer.
+ * @size: ? ?[in] ? ?Size of the buffer
+ * @flags: ? [in] ? ?mode flags for the file.
+ *
+ * Returns, on success, a newly created dma_buf object, which wraps the
+ * supplied private data and operations for dma_buf_ops. On either missing
+ * ops, or error in allocating struct dma_buf, will return negative error.
+ *
+ */
+struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? size_t size, int flags)
+{
+ ? ? struct dma_buf *dmabuf;
+ ? ? struct file *file;
+
+ ? ? if (WARN_ON(!priv || !ops
+ ? ? ? ? ? ? ? ? ? ? ? || !ops->map_dma_buf
+ ? ? ? ? ? ? ? ? ? ? ? || !ops->unmap_dma_buf
+ ? ? ? ? ? ? ? ? ? ? ? || !ops->release)) {
+ ? ? ? ? ? ? return ERR_PTR(-EINVAL);
+ ? ? }
+
+ ? ? dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
+ ? ? if (dmabuf == NULL)
+ ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
+
+ ? ? dmabuf->priv = priv;
+ ? ? dmabuf->ops = ops;

dmabuf->ops will never but NULL, but (see below)

<snip>

quoted

+struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device *dev)
+{
+ ? ? struct dma_buf_attachment *attach;
+ ? ? int ret;
+
+ ? ? if (WARN_ON(!dmabuf || !dev || !dmabuf->ops))

you still check dmabuf->ops here, as well as in several places below.
Shouldn't these checks be removed ?

You're right - these can be removed.

quoted

+ ? ? ? ? ? ? return ERR_PTR(-EINVAL);
+
+ ? ? attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
+ ? ? if (attach == NULL)
+ ? ? ? ? ? ? goto err_alloc;

What about returning ERR_PTR(-ENOMEM) directly here ?

Right; we can do that.

quoted

+
+ ? ? mutex_lock(&dmabuf->lock);
+
+ ? ? attach->dev = dev;
+ ? ? attach->dmabuf = dmabuf;

These two lines can be moved before mutex_lock().

:) Yes - thanks for catching this.
<snip>

--
Regards,

Laurent Pinchart

Let me know if you'd send patches for these, or should I just go ahead
and correct.

Best regards,
~Sumit.