Re: [PATCH 3/3] staging: gasket: page_table: add mapping flags

2018-10-19 Thread Greg Kroah-Hartman
On Tue, Oct 16, 2018 at 05:03:09AM -0700, Todd Poynor wrote:
> From: Nick Ewalt 
> 
> This allows for more precise dma_direction in the dma_map_page requests.
> Also leaves room for adding more flags later.

Why are you adding new features to this code?  It needs to have stuff
cleaned up and removed before you can add new stuff to it.

And why is this new ioctl even needed?

Some patch review comments below:

> +/*
> + * Structure for ioctl mapping buffers with flags when using the Gasket
> + * page_table module.
> + */
> +struct gasket_page_table_ioctl_flags {
> + struct gasket_page_table_ioctl base;
> + /*
> +  * Flags indicating status and attribute requests from the host.
> +  * NOTE: STATUS bit does not need to be set in this request.
> +  *   Set RESERVED bits to 0 to ensure backwards compatibility.
> +  *
> +  * Bitfields:
> +  *   [0] - STATUS: indicates if this entry/slot is free
> +  *0 = PTE_FREE
> +  *1 = PTE_INUSE
> +  *   [2:1]   - DMA_DIRECTION: dma_data_direction requested by host
> +  *   00 = DMA_BIDIRECTIONAL
> +  *   01 = DMA_TO_DEVICE
> +  *   10 = DMA_FROM_DEVICE
> +  *   11 = DMA_NONE
> +  *   [31:3]  - RESERVED

What endian are these bitfields in?

> +  */
> + u32 flags;

"u32" is not a valid variable type for something that goes across the
user/kernel boundry.  It should be __u32.  You all know this stuff...

> diff --git a/drivers/staging/gasket/gasket_page_table.c 
> b/drivers/staging/gasket/gasket_page_table.c
> index b7d460cf15fbc..06e188f5b905c 100644
> --- a/drivers/staging/gasket/gasket_page_table.c
> +++ b/drivers/staging/gasket/gasket_page_table.c
> @@ -87,6 +87,19 @@
>   */
>  #define GASKET_EXTENDED_LVL1_SHIFT 12
>  
> +/*
> + * Utilities for accessing flags bitfields.
> + */
> +#define MASK(field)(((1u << field##_WIDTH) - 1) << field##_SHIFT)
> +#define GET(field, flags)  (((flags) & MASK(field)) >> field##_SHIFT)
> +#define SET(field, flags, val) (((flags) & ~MASK(field)) | ((val) << 
> field##_SHIFT))

Ick, why invent stuff the kernel already has?  Please never do that, use
the functions/macros we already have for this very thing please.

That way I don't have to audit it that you all got it correct, and
neither do you have to guess that you got it correct :)


thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel


[PATCH 3/3] staging: gasket: page_table: add mapping flags

2018-10-16 Thread Todd Poynor
From: Nick Ewalt 

This allows for more precise dma_direction in the dma_map_page requests.
Also leaves room for adding more flags later.

Signed-off-by: Nick Ewalt 
Signed-off-by: Todd Poynor 
---
 drivers/staging/gasket/gasket.h| 33 
 drivers/staging/gasket/gasket_ioctl.c  | 62 +++
 drivers/staging/gasket/gasket_page_table.c | 87 ++
 drivers/staging/gasket/gasket_page_table.h |  4 +-
 4 files changed, 141 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/gasket/gasket.h b/drivers/staging/gasket/gasket.h
index a0f065c517a52..93e7af1551975 100644
--- a/drivers/staging/gasket/gasket.h
+++ b/drivers/staging/gasket/gasket.h
@@ -37,6 +37,31 @@ struct gasket_page_table_ioctl {
u64 device_address;
 };
 
+/*
+ * Structure for ioctl mapping buffers with flags when using the Gasket
+ * page_table module.
+ */
+struct gasket_page_table_ioctl_flags {
+   struct gasket_page_table_ioctl base;
+   /*
+* Flags indicating status and attribute requests from the host.
+* NOTE: STATUS bit does not need to be set in this request.
+*   Set RESERVED bits to 0 to ensure backwards compatibility.
+*
+* Bitfields:
+*   [0] - STATUS: indicates if this entry/slot is free
+*0 = PTE_FREE
+*1 = PTE_INUSE
+*   [2:1]   - DMA_DIRECTION: dma_data_direction requested by host
+*   00 = DMA_BIDIRECTIONAL
+*   01 = DMA_TO_DEVICE
+*   10 = DMA_FROM_DEVICE
+*   11 = DMA_NONE
+*   [31:3]  - RESERVED
+*/
+   u32 flags;
+};
+
 /*
  * Common structure for ioctls mapping and unmapping buffers when using the
  * Gasket page_table module.
@@ -119,4 +144,12 @@ struct gasket_coherent_alloc_config_ioctl {
 #define GASKET_IOCTL_CONFIG_COHERENT_ALLOCATOR 
\
_IOWR(GASKET_IOCTL_BASE, 11, struct gasket_coherent_alloc_config_ioctl)
 
+/*
+ * Tells the kernel to map size bytes at host_address to device_address in
+ * page_table_index page table. Passes flags to indicate additional attribute
+ * requests for the mapped memory.
+ */
+#define GASKET_IOCTL_MAP_BUFFER_FLAGS  
\
+   _IOW(GASKET_IOCTL_BASE, 12, struct gasket_page_table_ioctl_flags)
+
 #endif /* __GASKET_H__ */
diff --git a/drivers/staging/gasket/gasket_ioctl.c 
b/drivers/staging/gasket/gasket_ioctl.c
index 0ca48e688818f..e80f38509f47c 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -20,6 +20,7 @@
 #define trace_gasket_ioctl_integer_data(x)
 #define trace_gasket_ioctl_eventfd_data(x, ...)
 #define trace_gasket_ioctl_page_table_data(x, ...)
+#define trace_gasket_ioctl_page_table_flags_data(x, ...)
 #define trace_gasket_ioctl_config_coherent_allocator(x, ...)
 #endif
 
@@ -130,29 +131,59 @@ static int gasket_partition_page_table(
 }
 
 /* Map a userspace buffer to a device virtual address. */
+static int gasket_map_buffers_common(
+   struct gasket_dev *gasket_dev,
+   struct gasket_page_table_ioctl_flags *pibuf)
+{
+   if (pibuf->base.page_table_index >= gasket_dev->num_page_tables)
+   return -EFAULT;
+
+   if 
(gasket_page_table_are_addrs_bad(gasket_dev->page_table[pibuf->base.page_table_index],
+   pibuf->base.host_address,
+   pibuf->base.device_address,
+   pibuf->base.size))
+   return -EINVAL;
+
+   return 
gasket_page_table_map(gasket_dev->page_table[pibuf->base.page_table_index],
+pibuf->base.host_address,
+pibuf->base.device_address,
+pibuf->base.size / PAGE_SIZE,
+pibuf->flags);
+}
+
 static int gasket_map_buffers(struct gasket_dev *gasket_dev,
  struct gasket_page_table_ioctl __user *argp)
 {
-   struct gasket_page_table_ioctl ibuf;
+   struct gasket_page_table_ioctl_flags ibuf;
 
-   if (copy_from_user(, argp, sizeof(struct gasket_page_table_ioctl)))
+   if (copy_from_user(, argp, sizeof(struct 
gasket_page_table_ioctl)))
return -EFAULT;
 
-   trace_gasket_ioctl_page_table_data(ibuf.page_table_index, ibuf.size,
-  ibuf.host_address,
-  ibuf.device_address);
+   ibuf.flags = 0;
 
-   if (ibuf.page_table_index >= gasket_dev->num_page_tables)
+   trace_gasket_ioctl_page_table_data(ibuf.base.page_table_index,
+  ibuf.base.size,
+  ibuf.base.host_address,
+  ibuf.base.device_address);
+
+   return