[PATCH v6,0/4] misc: new driver sram_uapi for user level SRAM access

[PATCH v6,0/4] misc: new driver sram_uapi for user level SRAM access

[Wang Wenhu]

This series add a new misc device driver which act as an interface to
access the Cache-SRAM from user level. This is extremely helpful for
some user space applications that require high performance memory
accesses.

It also fixes the compile errors and warning of the Freescale MPC85xx
Cache-SRAM hardware driver.

The former five version implemented the driver with UIO but they were
commented of not fitful. This version uses a misc divice and implements
the memory allocation and free operations via file operation as suggested
by Scott.

Wang Wenhu (4):
  powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
  powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
  powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
  drivers: misc: new driver sram_uapi for user level SRAM  access

 arch/powerpc/sysdev/fsl_85xx_cache_sram.c |   3 +-
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c     |   1 +
 drivers/misc/Kconfig                      |  25 ++
 drivers/misc/Makefile                     |   1 +
 drivers/misc/sram_uapi.c                  | 294 ++++++++++++++++++++++
 5 files changed, 323 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/sram_uapi.c

-- 
2.17.1

[PATCH v6,1/4] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr

[Wang Wenhu]

Include "linux/of_address.h" to fix the compile error for
mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.

  CC      arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of function ‘of_iomap’; did you mean ‘pci_iomap’? [-Werror=implicit-function-declaration]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
           ^~~~~~~~
           pci_iomap
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  l2ctlr = of_iomap(dev->dev.of_node, 0);
         ^
cc1: all warnings being treated as errors
scripts/Makefile.build:267: recipe for target 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
No change v1-v5
---
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <asm/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
-- 
2.17.1

[PATCH v6,2/4] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram

[Wang Wenhu]

Include linux/io.h into fsl_85xx_cache_sram.c to fix the
implicit-declaration compile error when building Cache-Sram.

arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function ‘instantiate_cache_sram’:
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit declaration of function ‘ioremap_coherent’; did you mean ‘bitmap_complement’? [-Werror=implicit-function-declaration]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                          ^~~~~~~~~~~~~~~~
                          bitmap_complement
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
  cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
                        ^
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit declaration of function ‘iounmap’; did you mean ‘roundup’? [-Werror=implicit-function-declaration]
  iounmap(cache_sram->base_virt);
  ^~~~~~~
  roundup
cc1: all warnings being treated as errors

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
No change v1-v5
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index f6c665dac725..be3aef4229d7 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <asm/pgtable.h>
 #include <asm/fsl_85xx_cache_sram.h>
+#include <linux/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
 
-- 
2.17.1

[PATCH v6,3/4] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram

[Wang Wenhu]

Function instantiate_cache_sram should not be linked into the init
section for its caller mpc85xx_l2ctlr_of_probe is none-__init.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
No change v1-v5
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index be3aef4229d7..3de5ac8382c0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
 }
 EXPORT_SYMBOL(mpc85xx_cache_sram_free);
 
-int __init instantiate_cache_sram(struct platform_device *dev,
+int instantiate_cache_sram(struct platform_device *dev,
 		struct sram_parameters sram_params)
 {
 	int ret = 0;
-- 
2.17.1

[PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access

[Wang Wenhu]

A generic User-Kernel interface that allows a misc device created
by it to support file-operations of ioctl and mmap to access SRAM
memory from user level. Different kinds of SRAM alloction and free
APIs could be added to the available array and could be configured
from user level.

Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
Notes:
	Implement the interface driver for SRAM access from user
	level upon the comments from Scott.
	The former versions(1-5) were implemented with UIO, but for this
	version, UIO is not used as suggested by Scott.
Links:
	https://lore.kernel.org/patchwork/patch/1226475/
	https://lore.kernel.org/patchwork/patch/1225798/
---
 drivers/misc/Kconfig     |  25 ++++
 drivers/misc/Makefile    |   1 +
 drivers/misc/sram_uapi.c | 294 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 320 insertions(+)
 create mode 100644 drivers/misc/sram_uapi.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 99e151475d8f..e6897ba22684 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -465,6 +465,31 @@ config PVPANIC
 	  a paravirtualized device provided by QEMU; it lets a virtual machine
 	  (guest) communicate panic events to the host.
 
+config SRAM_UAPI
+	bool "Generic SRAM User Level API driver"
+	help
+	  This driver allows you to create a misc device which could be used
+	  as an interface to allocate SRAM memory from user level.
+
+	  It is extremely helpful for some user space applications that require
+	  high performance memory accesses.
+
+	  If unsure, say N.
+
+if SRAM_UAPI
+
+config FSL_85XX_SRAM_UAPI
+	bool "Freescale MPC85xx Cache-SRAM UAPI support"
+	depends on FSL_SOC_BOOKE && PPC32
+	select FSL_85XX_CACHE_SRAM
+	help
+	  This adds the Freescale MPC85xx Cache-SRAM memory allocation and
+	  free interfaces to the available SRAM API array, which finally could
+	  be used from user level to access the Freescale MPC85xx Cache-SRAM
+	  memory.
+
+endif
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 9abf2923d831..794447ca07ca 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
 obj-$(CONFIG_SRAM)		+= sram.o
 obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
+obj-$(CONFIG_SRAM_UAPI)		+= sram_uapi.o
 obj-y				+= mic/
 obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
diff --git a/drivers/misc/sram_uapi.c b/drivers/misc/sram_uapi.c
new file mode 100644
index 000000000000..53f818e1898d
--- /dev/null
+++ b/drivers/misc/sram_uapi.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
+ * All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+#include <linux/miscdevice.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+
+#define DRIVER_NAME	"sram_uapi"
+
+#define SRAM_UAPI_IOCTL_SET_SRAM_TYPE	0
+#define SRAM_UAPI_IOCTL_ALLOC		1
+#define SRAM_UAPI_IOCTL_FREE		2
+
+struct res_info {
+	u32 offset;
+	u32 size;
+};
+
+struct sram_resource {
+	struct list_head	list;
+	struct res_info		info;
+	phys_addr_t		phys;
+	void			*virt;
+	struct vm_area_struct	*vma;
+	struct sram_uapi	*parent;
+};
+
+struct sram_api {
+	u32 type;
+	long (*sram_alloc)(u32 size, phys_addr_t *phys, u32 align);
+	void (*sram_free)(void *ptr);
+};
+
+struct sram_uapi {
+	struct list_head	res_list;
+	struct sram_api		*sa;
+};
+
+enum SRAM_TYPE {
+#ifdef FSL_85XX_CACHE_SRAM
+	SRAM_TYPE_FSL_85XX_CACHE_SRAM,
+#endif
+	SRAM_TYPE_MAX,
+};
+
+/* keep the SRAM_TYPE value the same with array index */
+static struct sram_api srams[] = {
+#ifdef FSL_85XX_CACHE_SRAM
+	{
+		.type		= SRAM_TYPE_FSL_85XX_CACHE_SRAM,
+		.sram_alloc	= mpc85xx_cache_sram_alloc,
+		.sram_free	= mpc85xx_cache_sram_free,
+	},
+#endif
+};
+
+static void sram_uapi_res_insert(struct sram_uapi *uapi,
+				 struct sram_resource *res)
+{
+	struct sram_resource *cur, *tmp;
+	struct list_head *head = &uapi->res_list;
+
+	list_for_each_entry_safe(cur, tmp, head, list) {
+		if (&tmp->list != head &&
+		    (cur->info.offset + cur->info.size + res->info.size <=
+		    tmp->info.offset)) {
+			res->info.offset = cur->info.offset + cur->info.size;
+			res->parent = uapi;
+			list_add(&res->list, &cur->list);
+			return;
+		}
+	}
+
+	if (list_empty(head))
+		res->info.offset = 0;
+	else {
+		tmp = list_last_entry(head, struct sram_resource, list);
+		res->info.offset = tmp->info.offset + tmp->info.size;
+	}
+	list_add_tail(&res->list, head);
+}
+
+static struct sram_resource *sram_uapi_res_delete(struct sram_uapi *uapi,
+						  struct res_info *info)
+{
+	struct sram_resource *res, *tmp;
+
+	list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
+		if (res->info.offset == info->offset) {
+			list_del(&res->list);
+			res->parent = NULL;
+			return res;
+		}
+	}
+
+	return NULL;
+}
+
+static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
+						u32 offset)
+{
+	struct sram_resource *res;
+
+	list_for_each_entry(res, &uapi->res_list, list) {
+		if (res->info.offset == offset)
+			return res;
+	}
+
+	return NULL;
+}
+
+static int sram_uapi_open(struct inode *inode, struct file *filp)
+{
+	struct sram_uapi *uapi;
+
+	uapi = kzalloc(sizeof(*uapi), GFP_KERNEL);
+	if (!uapi)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&uapi->res_list);
+	filp->private_data = uapi;
+
+	return 0;
+}
+
+static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct sram_uapi *uapi = filp->private_data;
+	struct sram_resource *res;
+	struct res_info info;
+	long ret = -EINVAL;
+	int size;
+	u32 type;
+
+	if (!uapi)
+		return ret;
+
+	switch (cmd) {
+	case SRAM_UAPI_IOCTL_SET_SRAM_TYPE:
+		size = copy_from_user((void *)&type, (const void __user *)arg,
+				      sizeof(type));
+		if (type >= SRAM_TYPE_MAX)
+			return -EINVAL;
+
+		uapi->sa = &srams[type];
+
+		ret = 0;
+		break;
+
+	case SRAM_UAPI_IOCTL_ALLOC:
+		if (!uapi->sa)
+			return -EINVAL;
+
+		res = kzalloc(sizeof(*res), GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		size = copy_from_user((void *)&res->info,
+				      (const void __user *)arg,
+				      sizeof(res->info));
+		if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
+			return -EINVAL;
+
+		res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
+					&res->phys,
+					roundup_pow_of_two(res->info.size));
+		if (!res->virt) {
+			kfree(res);
+			return -ENOMEM;
+		}
+
+		sram_uapi_res_insert(uapi, res);
+		size = copy_to_user((void __user *)arg,
+				    (const void *)&res->info,
+				    sizeof(res->info));
+
+		ret = 0;
+		break;
+
+	case SRAM_UAPI_IOCTL_FREE:
+		if (!uapi->sa)
+			return -EINVAL;
+
+		size = copy_from_user((void *)&info, (const void __user *)arg,
+				      sizeof(info));
+
+		res = sram_uapi_res_delete(uapi, &info);
+		if (!res) {
+			pr_err("error no sram resource found\n");
+			return -EINVAL;
+		}
+
+		uapi->sa->sram_free(res->virt);
+		kfree(res);
+
+		ret = 0;
+		break;
+
+	default:
+		pr_err("error no cmd not supported\n");
+		break;
+	}
+
+	return ret;
+}
+
+static int sram_uapi_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct sram_uapi *uapi = filp->private_data;
+	struct sram_resource *res;
+
+	res = sram_uapi_find_res(uapi, vma->vm_pgoff);
+	if (!res)
+		return -EINVAL;
+
+	if (vma->vm_end - vma->vm_start > res->info.size)
+		return -EINVAL;
+
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	return remap_pfn_range(vma, vma->vm_start,
+				res->phys >> PAGE_SHIFT,
+				vma->vm_end - vma->vm_start,
+				vma->vm_page_prot);
+}
+
+static void sram_uapi_res_release(struct sram_uapi *uapi)
+{
+	struct sram_resource *res, *tmp;
+
+	list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
+		list_del(&res->list);
+		uapi->sa->sram_free(res->virt);
+		kfree(res);
+	}
+}
+
+static int sram_uapi_release(struct inode *inodp, struct file *filp)
+{
+	struct sram_uapi *uapi = filp->private_data;
+
+	sram_uapi_res_release(uapi);
+
+	kfree(uapi);
+
+	return 0;
+}
+
+static const struct file_operations sram_uapi_ops = {
+	.owner = THIS_MODULE,
+	.open = sram_uapi_open,
+	.unlocked_ioctl = sram_uapi_ioctl,
+	.mmap = sram_uapi_mmap,
+	.release = sram_uapi_release,
+};
+
+static struct miscdevice sram_uapi_miscdev = {
+	MISC_DYNAMIC_MINOR,
+	"sram-uapi",
+	&sram_uapi_ops,
+};
+
+static int __init sram_uapi_init(void)
+{
+	int ret;
+
+	ret = misc_register(&sram_uapi_miscdev);
+	if (ret)
+		pr_err("failed to register sram_uapi misc device\n");
+
+	return ret;
+}
+
+static void __exit sram_uapi_exit(void)
+{
+	misc_deregister(&sram_uapi_miscdev);
+}
+
+module_init(sram_uapi_init);
+module_exit(sram_uapi_exit);
+
+MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
+MODULE_DESCRIPTION("SRAM User API Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access

[Arnd Bergmann]

On Sat, Apr 18, 2020 at 6:22 PM Wang Wenhu <wenhu.wang@vivo.com> wrote:
>
> A generic User-Kernel interface that allows a misc device created
> by it to support file-operations of ioctl and mmap to access SRAM
> memory from user level. Different kinds of SRAM alloction and free
> APIs could be added to the available array and could be configured
> from user level.

Having a generic user level interface seem reasonable, but it would
be helpful to list one or more particular use cases.

> +if SRAM_UAPI
> +
> +config FSL_85XX_SRAM_UAPI
> +       bool "Freescale MPC85xx Cache-SRAM UAPI support"
> +       depends on FSL_SOC_BOOKE && PPC32
> +       select FSL_85XX_CACHE_SRAM
> +       help
> +         This adds the Freescale MPC85xx Cache-SRAM memory allocation and
> +         free interfaces to the available SRAM API array, which finally could
> +         be used from user level to access the Freescale MPC85xx Cache-SRAM
> +         memory.

Why do you need  a hardware specific Kconfig option here, shouldn't
this just use the generic kernel abstraction for the sram?

> +struct sram_api {
> +       u32 type;
> +       long (*sram_alloc)(u32 size, phys_addr_t *phys, u32 align);
> +       void (*sram_free)(void *ptr);
> +};
> +
> +struct sram_uapi {
> +       struct list_head        res_list;
> +       struct sram_api         *sa;
> +};
> +
> +enum SRAM_TYPE {
> +#ifdef FSL_85XX_CACHE_SRAM
> +       SRAM_TYPE_FSL_85XX_CACHE_SRAM,
> +#endif
> +       SRAM_TYPE_MAX,
> +};
> +
> +/* keep the SRAM_TYPE value the same with array index */
> +static struct sram_api srams[] = {
> +#ifdef FSL_85XX_CACHE_SRAM
> +       {
> +               .type           = SRAM_TYPE_FSL_85XX_CACHE_SRAM,
> +               .sram_alloc     = mpc85xx_cache_sram_alloc,
> +               .sram_free      = mpc85xx_cache_sram_free,
> +       },
> +#endif
> +};

If there is a indeed a requirement for hardware specific functions,
I'd say these should be registered from the hardware specific driver
rather than the generic driver having to know about every single
instance.

> +static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
> +                           unsigned long arg)
> +{
> +       struct sram_uapi *uapi = filp->private_data;
> +       struct sram_resource *res;
> +       struct res_info info;
> +       long ret = -EINVAL;
> +       int size;
> +       u32 type;
> +
> +       if (!uapi)
> +               return ret;
> +
> +       switch (cmd) {
> +       case SRAM_UAPI_IOCTL_SET_SRAM_TYPE:
> +               size = copy_from_user((void *)&type, (const void __user *)arg,
> +                                     sizeof(type));

This could be a simpler get_user().

> +static const struct file_operations sram_uapi_ops = {
> +       .owner = THIS_MODULE,
> +       .open = sram_uapi_open,
> +       .unlocked_ioctl = sram_uapi_ioctl,
> +       .mmap = sram_uapi_mmap,
> +       .release = sram_uapi_release,
> +};

If you have a .unlocked_ioctl callback, there should also be a
.compat_ioctl one. This can normally point to compat_ptr_ioctl().

> +
> +static struct miscdevice sram_uapi_miscdev = {
> +       MISC_DYNAMIC_MINOR,
> +       "sram-uapi",
> +       &sram_uapi_ops,
> +};

The name of the character device should not contain "uapi", that
is kind of implied here.


        Arnd

Re: [PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access

[Arnd Bergmann]

On Sat, Apr 18, 2020 at 6:22 PM Wang Wenhu <wenhu.wang@vivo.com> wrote:
> +#define DRIVER_NAME    "sram_uapi"
> +
> +#define SRAM_UAPI_IOCTL_SET_SRAM_TYPE  0
> +#define SRAM_UAPI_IOCTL_ALLOC          1
> +#define SRAM_UAPI_IOCTL_FREE           2
> +
> +struct res_info {
> +       u32 offset;
> +       u32 size;
> +};

This is of course not a proper ioctl interface at all, please see
Documentation/driver-api/ioctl.rst for how to define the numbers
in a uapi header file.

The offset/size arguments should probably be 64 bit wide.

       Arnd

Re: [PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access

[王文虎]

>> A generic User-Kernel interface that allows a misc device created
>> by it to support file-operations of ioctl and mmap to access SRAM
>> memory from user level. Different kinds of SRAM alloction and free
>> APIs could be added to the available array and could be configured
>> from user level.
>
>Having a generic user level interface seem reasonable, but it would
>be helpful to list one or more particular use cases.

OK, I will use the FSL_85XX_SRAM as a case to describe it.

>
>> +if SRAM_UAPI
>> +
>> +config FSL_85XX_SRAM_UAPI
>> +       bool "Freescale MPC85xx Cache-SRAM UAPI support"
>> +       depends on FSL_SOC_BOOKE && PPC32
>> +       select FSL_85XX_CACHE_SRAM
>> +       help
>> +         This adds the Freescale MPC85xx Cache-SRAM memory allocation and
>> +         free interfaces to the available SRAM API array, which finally could
>> +         be used from user level to access the Freescale MPC85xx Cache-SRAM
>> +         memory.
>
>Why do you need  a hardware specific Kconfig option here, shouldn't
>this just use the generic kernel abstraction for the sram?
>
Yes, I will add a interface for any hardware drivers to register there specific apis
instead of the definition here.

>> +struct sram_api {
>> +       u32 type;
>> +       long (*sram_alloc)(u32 size, phys_addr_t *phys, u32 align);
>> +       void (*sram_free)(void *ptr);
>> +};
>> +
>> +struct sram_uapi {
>> +       struct list_head        res_list;
>> +       struct sram_api         *sa;
>> +};
>> +
>> +enum SRAM_TYPE {
>> +#ifdef FSL_85XX_CACHE_SRAM
>> +       SRAM_TYPE_FSL_85XX_CACHE_SRAM,
>> +#endif
>> +       SRAM_TYPE_MAX,
>> +};
>> +
>> +/* keep the SRAM_TYPE value the same with array index */
>> +static struct sram_api srams[] = {
>> +#ifdef FSL_85XX_CACHE_SRAM
>> +       {
>> +               .type           = SRAM_TYPE_FSL_85XX_CACHE_SRAM,
>> +               .sram_alloc     = mpc85xx_cache_sram_alloc,
>> +               .sram_free      = mpc85xx_cache_sram_free,
>> +       },
>> +#endif
>> +};
>
>If there is a indeed a requirement for hardware specific functions,
>I'd say these should be registered from the hardware specific driver
>rather than the generic driver having to know about every single
>instance.

Yes, as you mentioned upper, and the interfaces should be registered
by hardware drivers. and I'd use a set of generic abstractions of the definitions.

>> +static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
>> +                           unsigned long arg)
>> +{
>> +       struct sram_uapi *uapi = filp->private_data;
>> +       struct sram_resource *res;
>> +       struct res_info info;
>> +       long ret = -EINVAL;
>> +       int size;
>> +       u32 type;
>> +
>> +       if (!uapi)
>> +               return ret;
>> +
>> +       switch (cmd) {
>> +       case SRAM_UAPI_IOCTL_SET_SRAM_TYPE:
>> +               size = copy_from_user((void *)&type, (const void __user *)arg,
>> +                                     sizeof(type));
>
>This could be a simpler get_user().

Addressed.

>
>> +static const struct file_operations sram_uapi_ops = {
>> +       .owner = THIS_MODULE,
>> +       .open = sram_uapi_open,
>> +       .unlocked_ioctl = sram_uapi_ioctl,
>> +       .mmap = sram_uapi_mmap,
>> +       .release = sram_uapi_release,
>> +};
>
>If you have a .unlocked_ioctl callback, there should also be a
>.compat_ioctl one. This can normally point to compat_ptr_ioctl().

Addressed

>> +
>> +static struct miscdevice sram_uapi_miscdev = {
>> +       MISC_DYNAMIC_MINOR,
>> +       "sram-uapi",
>> +       &sram_uapi_ops,
>> +};
>
>The name of the character device should not contain "uapi", that
>is kind of implied here.

Addressed

>> +
>> +#define SRAM_UAPI_IOCTL_SET_SRAM_TYPE  0
>> +#define SRAM_UAPI_IOCTL_ALLOC          1
>> +#define SRAM_UAPI_IOCTL_FREE           2
>> +
>> +struct res_info {
>> +       u32 offset;
>> +       u32 size;
>> +};
>
>This is of course not a proper ioctl interface at all, please see
>Documentation/driver-api/ioctl.rst for how to define the numbers
>in a uapi header file.
>
>The offset/size arguments should probably be 64 bit wide.

OK, I will reference the ioctl.rst and make a improvement and I think
phys_addr_t would be a better choice.

Thanks,
Wenhu