Re: [PATCH v1 04/14] tee: shm: add page accessor functions

From: Volodymyr Babchuk
Date: Fri Sep 29 2017 - 06:17:15 EST


Hi Yury,

On 29.09.17 01:14, Yury Norov wrote:
Hi Volodymyr,

On Thu, Sep 28, 2017 at 09:04:01PM +0300, Volodymyr Babchuk wrote:
From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>

In order to register a shared buffer in TEE, we need accessor
function that return list of pages for that buffer.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
---
include/linux/tee_drv.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 6ba8b76..d773827 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -394,6 +394,20 @@ static inline size_t tee_shm_get_size(struct tee_shm *shm)
}
/**
+ * tee_shm_get_pages() - Get list of pages that hold shared buffer
+ * @shm: Shared memory handle
+ * @num_pages: Number of pages will be stored there
+ * @returns pointer to pages array
+ */
+static inline struct page **tee_shm_get_pages(struct tee_shm *shm,
+ size_t *num_pages)
+{
+ if (num_pages)
+ *num_pages = shm->num_pages;

My concern is about this check

The only use of the tee_shm_get_pages() I found is in patch #9:
+ size_t page_num;
+
+ pages = tee_shm_get_pages(shm, &page_num);

So there's no any valid scenario where you should pass NULL to the
function. And I don't understand why you do this check.

Even more, if in future there will be an occasion when function will
be passed with NULL, the error will become hidden by this code.
Yes, I think you are right. I added that check in case someone want
to get only pointer to pages. But this is semantically invalid.
I'll remove that check. Thank you for review.