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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Oct 16, 2018 at 05:03:09AM -0700, Todd Poynor wrote:
> From: Nick Ewalt <nicholasewalt@google.com>
> 
> 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