Re: [PATCH v3,4/5] misc: sram_dynamic for user level SRAM access

[Christophe Leroy]

Le 24/04/2020 Ã 04:45, Wang Wenhu a ÃcritÂ:
> A generic User-Kernel interface module that allows a misc device created
> when a backend SRAM hardware device driver registers its APIs to support
> file operations of ioctl and mmap for user space applications to allocate
> SRAM memory, mmap it to process address space and free it then after.
>
> It is extremely helpful for the user space applications that require
> high performance memory accesses, such as embedded networking devices
> that would process data in user space, and PowerPC e500 is one case.
>
> Signed-off-by: Wang Wenhu <wenhu.wang@xxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Christophe Leroy <christophe.leroy@xxxxxx>
> Cc: Scott Wood <oss@xxxxxxxxxxxx>
> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> ---
> Changes since v1: addressed comments from Arnd
>   * Changed the ioctl cmd definitions using _IO micros
>   * Export interfaces for HW-SRAM drivers to register apis to available list
>   * Modified allocation alignment to PAGE_SIZE
>   * Use phys_addr_t as type of SRAM resource size and offset
>   * Support compat_ioctl
>   * Misc device name:sram
>   * Use tristate for SRAM_UAPI
>   * Use postcore_initcall
>
> Changes since v2: addressed comments from Arnd, greg and Scott
>   * Name the module with sram_dynamic in comparing with drivers/misc/sram.c
>
>      I tried to tie the sram_dynamic with the abstractions in sram.c as
>      Arnd suggested, and actually sram.c probes SRAM devices from devicetree
>      and manages them with different partitions and create memory pools which
>      are managed with genalloc functions.
>
>      Here sram_dynamic acts only as a interface to user space. A SRAM memory
>      pool is managed by the module that registers APIs to us, such as the
>      backend hardware driver of Freescale 85xx Cache-SRAM.
>
>   * Create one sram_device for each backend SRAM device(from Scott)
>   * Allow only one block of SRAM memory allocated to a file descriptor(from Scott)
>   * Add sysfs files for every allocated SRAM memory block
>   * More documentations(As Greg commented)
>   * Make uapi and non-uapi components apart(from Arnd and Greg)
>
> Links:
> v1: https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@xxxxxxxx/
> v2: https://lore.kernel.org/lkml/20200420030538.101696-1-wenhu.wang@xxxxxxxx/
> UIO version:
> v5: https://lore.kernel.org/lkml/20200417071616.44598-5-wenhu.wang@xxxxxxxx/
> ---
>   drivers/misc/Kconfig         |  11 +
>   drivers/misc/Makefile        |   1 +
>   drivers/misc/sram_dynamic.c  | 580 +++++++++++++++++++++++++++++++++++
>   drivers/misc/sram_uapi.c     | 351 +++++++++++++++++++++
>   include/linux/sram_dynamic.h |  23 ++
>   include/uapi/linux/sram.h    |  11 +
>   6 files changed, 977 insertions(+)
>   create mode 100644 drivers/misc/sram_dynamic.c
>   create mode 100644 drivers/misc/sram_uapi.c
>   create mode 100644 include/linux/sram_dynamic.h
>   create mode 100644 include/uapi/linux/sram.h

> diff --git a/include/linux/sram_dynamic.h b/include/linux/sram_dynamic.h
> new file mode 100644
> index 000000000000..c77e9e7b1151
> --- /dev/null
> +++ b/include/linux/sram_dynamic.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SRAM_DYNAMIC_H
> +#define __SRAM_DYNAMIC_H
> +
> +struct sram_api {
> +	const char	*name;
> +	struct sram_device *sdev;
> +	void *(*alloc)(__u32 size, phys_addr_t *phys, __u32 align);
> +	void (*free)(void *ptr);
> +};
> +
> +extern int __must_check
> +	__sram_register_device(struct module *owner,
> +			       struct device *parent,
> +			       struct sram_api *sa);

'extern' keyword is useless here, remove it (checkpatch --strict will 
likely tell you the same)

> +
> +/* Use a define to avoid include chaining to get THIS_MODULE */
> +#define sram_register_device(parent, sa) \
> +	__sram_register_device(THIS_MODULE, parent, sa)
> +
> +extern void sram_unregister_device(struct sram_api *sa);

Same, no 'extern' please.

> +
> +#endif /* __SRAM_DYNAMIC_H */
> diff --git a/include/uapi/linux/sram.h b/include/uapi/linux/sram.h
> new file mode 100644
> index 000000000000..9b4a2615dbfe
> --- /dev/null
> +++ b/include/uapi/linux/sram.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SRAM_H
> +#define __SRAM_H
> +
> +/* Allocate memory resource from SRAM */
> +#define SRAM_UAPI_IOC_ALLOC	_IOWR('S', 0, __be64)
> +
> +/* Free allocated memory resource to SRAM */
> +#define SRAM_UAPI_IOC_FREE	_IO('S', 1)
> +
> +#endif /* __SRAM_H */

Christophe