[PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[PATCH v2,RESEND] 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 registered by specific SRAM hardware level driver to
the available list and then be chosen by users to allocate and map
SRAM memory from user level.

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 a case.

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
---
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

Note: From this on, the SRAM_UAPI driver is independent to any hardware
drivers, so I would only commit the patch itself as v2, while the v1 of
it was wrapped together with patches for Freescale L2-Cache-SRAM device.
Then after, I'd create patches for Freescale L2-Cache-SRAM device as
another series.

Link for v1:
 * https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@vivo.com/
---
 drivers/misc/Kconfig      |  11 ++
 drivers/misc/Makefile     |   1 +
 drivers/misc/sram_uapi.c  | 352 ++++++++++++++++++++++++++++++++++++++
 include/linux/sram_uapi.h |  28 +++
 4 files changed, 392 insertions(+)
 create mode 100644 drivers/misc/sram_uapi.c
 create mode 100644 include/linux/sram_uapi.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 99e151475d8f..b19c8b24f18e 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -465,6 +465,17 @@ 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.
+
 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..66c7b56b635f
--- /dev/null
+++ b/drivers/misc/sram_uapi.c
@@ -0,0 +1,352 @@
+// 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/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/sram_uapi.h>
+
+#define DRIVER_NAME	"sram_uapi"
+
+struct res_info {
+	phys_addr_t offset;
+	phys_addr_t 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_uapi {
+	struct list_head	res_list;
+	struct sram_api		*sa;
+};
+
+static DEFINE_MUTEX(sram_api_list_lock);
+static LIST_HEAD(sram_api_list);
+
+long sram_api_register(struct sram_api *sa)
+{
+	struct sram_api *cur;
+
+	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
+		return -EINVAL;
+
+	mutex_lock(&sram_api_list_lock);
+	list_for_each_entry(cur, &sram_api_list, list) {
+		if (cur->type == sa->type) {
+			pr_err("error sram %s type %d exists\n", sa->name,
+			       sa->type);
+			mutex_unlock(&sram_api_list_lock);
+			return -EEXIST;
+		}
+	}
+
+	kref_init(&sa->kref);
+	list_add_tail(&sa->list, &sram_api_list);
+	pr_info("sram %s type %d registered\n", sa->name, sa->type);
+
+	mutex_unlock(&sram_api_list_lock);
+
+	return 0;
+};
+EXPORT_SYMBOL(sram_api_register);
+
+long sram_api_unregister(struct sram_api *sa)
+{
+	struct sram_api *cur, *tmp;
+	long ret = -ENODEV;
+
+	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
+		return -EINVAL;
+
+	mutex_lock(&sram_api_list_lock);
+	list_for_each_entry_safe(cur, tmp, &sram_api_list, list) {
+		if (cur->type == sa->type && !strcmp(cur->name, sa->name)) {
+			if (kref_read(&cur->kref)) {
+				pr_err("error sram %s type %d is busy\n",
+					sa->name, sa->type);
+				ret = -EBUSY;
+			} else {
+				list_del(&cur->list);
+				pr_info("sram %s type %d unregistered\n",
+					sa->name, sa->type);
+				ret = 0;
+			}
+			break;
+		}
+	}
+	mutex_unlock(&sram_api_list_lock);
+
+	return ret;
+};
+EXPORT_SYMBOL(sram_api_unregister);
+
+static struct sram_api *get_sram_api_from_type(__u32 type)
+{
+	struct sram_api *cur;
+
+	mutex_lock(&sram_api_list_lock);
+	list_for_each_entry(cur, &sram_api_list, list) {
+		if (cur->type == type) {
+			kref_get(&cur->kref);
+			mutex_unlock(&sram_api_list_lock);
+			return cur;
+		}
+	}
+	mutex_unlock(&sram_api_list_lock);
+
+	return NULL;
+}
+
+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 = -ENOIOCTLCMD;
+	int size;
+	__u32 type;
+
+	if (!uapi)
+		return ret;
+
+	switch (cmd) {
+	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
+		if (uapi->sa)
+			return -EEXIST;
+
+		get_user(type, (const __u32 __user *)arg);
+		uapi->sa = get_sram_api_from_type(type);
+		if (uapi->sa)
+			ret = 0;
+		else
+			ret = -ENODEV;
+
+		break;
+
+	case SRAM_UAPI_IOC_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,
+							 PAGE_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_IOC_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);
+	if (uapi->sa)
+		kref_put(&uapi->sa->kref, NULL);
+
+	kfree(uapi);
+
+	return 0;
+}
+
+static const struct file_operations sram_uapi_ops = {
+	.owner		= THIS_MODULE,
+	.open		= sram_uapi_open,
+	.unlocked_ioctl = sram_uapi_ioctl,
+	.compat_ioctl	= compat_ptr_ioctl,
+	.mmap		= sram_uapi_mmap,
+	.release	= sram_uapi_release,
+};
+
+static struct miscdevice sram_uapi_miscdev = {
+	MISC_DYNAMIC_MINOR,
+	"sram",
+	&sram_uapi_ops,
+};
+
+static int __init sram_uapi_init(void)
+{
+	int ret;
+
+	INIT_LIST_HEAD(&sram_api_list);
+	mutex_init(&sram_api_list_lock);
+
+	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");
diff --git a/include/linux/sram_uapi.h b/include/linux/sram_uapi.h
new file mode 100644
index 000000000000..50fbf9dc308f
--- /dev/null
+++ b/include/linux/sram_uapi.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SRAM_UAPI_H
+#define __SRAM_UAPI_H
+
+/* Set SRAM type to be accessed */
+#define SRAM_UAPI_IOC_SET_SRAM_TYPE	_IOW('S', 0, __u32)
+
+/* Allocate resource from SRAM */
+#define SRAM_UAPI_IOC_ALLOC		_IOWR('S', 1, struct res_info)
+
+/* Free allocated resource of SRAM */
+#define SRAM_UAPI_IOC_FREE		_IOW('S', 2, struct res_info)
+
+struct sram_api {
+	struct list_head	list;
+	struct kref		kref;
+	__u32			type;
+	const char		*name;
+
+	long (*sram_alloc)(__u32 size, phys_addr_t *phys, __u32 align);
+	void (*sram_free)(void *ptr);
+};
+
+extern long sram_api_register(struct sram_api *sa);
+
+extern long sram_api_unregister(struct sram_api *sa);
+
+#endif /* __SRAM_UAPI_H */
-- 
2.17.1

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 5:06 AM 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 registered by specific SRAM hardware level driver to
> the available list and then be chosen by users to allocate and map
> SRAM memory from user level.
>
> 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 a case.
>
> 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
> ---
> 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

Looks much better already.

> Note: From this on, the SRAM_UAPI driver is independent to any hardware
> drivers, so I would only commit the patch itself as v2, while the v1 of
> it was wrapped together with patches for Freescale L2-Cache-SRAM device.
> Then after, I'd create patches for Freescale L2-Cache-SRAM device as
> another series.

What I meant to suggest was actually to tie it more closely to
the code we already have in drivers/misc/sram.c, which already
has some form of abstraction.

> +static int __init sram_uapi_init(void)
> +{
> +       int ret;
> +
> +       INIT_LIST_HEAD(&sram_api_list);
> +       mutex_init(&sram_api_list_lock);
> +
> +       ret = misc_register(&sram_uapi_miscdev);
> +       if (ret)
> +               pr_err("failed to register sram uapi misc device\n");

The mutex and listhead are already initialized, so this can
be a one-line function

    return misc_register(&sram_uapi_miscdev);

> --- /dev/null
> +++ b/include/linux/sram_uapi.h

The ioctl definitions need to be in include/uapi/linux/

> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SRAM_UAPI_H
> +#define __SRAM_UAPI_H
> +
> +/* Set SRAM type to be accessed */
> +#define SRAM_UAPI_IOC_SET_SRAM_TYPE    _IOW('S', 0, __u32)
> +
> +/* Allocate resource from SRAM */
> +#define SRAM_UAPI_IOC_ALLOC            _IOWR('S', 1, struct res_info)
> +
> +/* Free allocated resource of SRAM */
> +#define SRAM_UAPI_IOC_FREE             _IOW('S', 2, struct res_info)

struct res_info needs to also be defined here, so user applications can
see the definition, and it has to use __u64, not phys_addr_t, to ensure
the API does not depend on kernel configuraiton.

        Arnd

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Greg KH]

On Sun, Apr 19, 2020 at 08:05:38PM -0700, Wang Wenhu 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 registered by specific SRAM hardware level driver to
> the available list and then be chosen by users to allocate and map
> SRAM memory from user level.
> 
> 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 a case.
> 
> 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
> ---
> 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
> 
> Note: From this on, the SRAM_UAPI driver is independent to any hardware
> drivers, so I would only commit the patch itself as v2, while the v1 of
> it was wrapped together with patches for Freescale L2-Cache-SRAM device.
> Then after, I'd create patches for Freescale L2-Cache-SRAM device as
> another series.
> 
> Link for v1:
>  * https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@vivo.com/

Why was this a RESEND?  What was wrong with the original v2 version?

Anyway, minor review comments:

> ---
>  drivers/misc/Kconfig      |  11 ++
>  drivers/misc/Makefile     |   1 +
>  drivers/misc/sram_uapi.c  | 352 ++++++++++++++++++++++++++++++++++++++
>  include/linux/sram_uapi.h |  28 +++
>  4 files changed, 392 insertions(+)
>  create mode 100644 drivers/misc/sram_uapi.c
>  create mode 100644 include/linux/sram_uapi.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 99e151475d8f..b19c8b24f18e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -465,6 +465,17 @@ 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.

Naming is hard, but shouldn't this just be "sram" and drop the whole
"UAPI" stuff everywhere?  That doesn't make much sense as drivers are
just interfaces to userspace...


> +
>  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..66c7b56b635f
> --- /dev/null
> +++ b/drivers/misc/sram_uapi.c
> @@ -0,0 +1,352 @@
> +// 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/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +#include <linux/sram_uapi.h>
> +
> +#define DRIVER_NAME	"sram_uapi"

If you really need this, just use KBUILD_MODNAME instead.  But I don't
think you need it, as most drivers do not.


> +
> +struct res_info {
> +	phys_addr_t offset;
> +	phys_addr_t 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_uapi {
> +	struct list_head	res_list;
> +	struct sram_api		*sa;
> +};
> +
> +static DEFINE_MUTEX(sram_api_list_lock);
> +static LIST_HEAD(sram_api_list);
> +
> +long sram_api_register(struct sram_api *sa)

Why are all of these functions global?

And shouldn't this return an 'int'?

> +{
> +	struct sram_api *cur;
> +
> +	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
> +		return -EINVAL;

How can any of these be true?  You control all the callers of this
function.

> +
> +	mutex_lock(&sram_api_list_lock);
> +	list_for_each_entry(cur, &sram_api_list, list) {
> +		if (cur->type == sa->type) {
> +			pr_err("error sram %s type %d exists\n", sa->name,
> +			       sa->type);
> +			mutex_unlock(&sram_api_list_lock);
> +			return -EEXIST;
> +		}
> +	}
> +
> +	kref_init(&sa->kref);

So the caller has to allocate the space and set some things up, but not
everything?  That's a mess, why not just do it either all at once with
an "allocate" like function, or just do it all in one function.  Why is
this a separate function anyway?

> +	list_add_tail(&sa->list, &sram_api_list);
> +	pr_info("sram %s type %d registered\n", sa->name, sa->type);

Don't leave debugging comments in the driver, if all goes well a driver
should be totally silent.  Only log errors.

> +
> +	mutex_unlock(&sram_api_list_lock);
> +
> +	return 0;
> +};
> +EXPORT_SYMBOL(sram_api_register);

Exported for what?  This is a stand-alone driver, nothing else uses
these symbols.

> +
> +long sram_api_unregister(struct sram_api *sa)

Why long?

> +{
> +	struct sram_api *cur, *tmp;
> +	long ret = -ENODEV;
> +
> +	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
> +		return -EINVAL;

Again, how can any of this be true?

> +
> +	mutex_lock(&sram_api_list_lock);
> +	list_for_each_entry_safe(cur, tmp, &sram_api_list, list) {
> +		if (cur->type == sa->type && !strcmp(cur->name, sa->name)) {
> +			if (kref_read(&cur->kref)) {
> +				pr_err("error sram %s type %d is busy\n",
> +					sa->name, sa->type);
> +				ret = -EBUSY;
> +			} else {
> +				list_del(&cur->list);
> +				pr_info("sram %s type %d unregistered\n",
> +					sa->name, sa->type);
> +				ret = 0;
> +			}
> +			break;
> +		}
> +	}
> +	mutex_unlock(&sram_api_list_lock);
> +
> +	return ret;
> +};
> +EXPORT_SYMBOL(sram_api_unregister);
> +
> +static struct sram_api *get_sram_api_from_type(__u32 type)
> +{
> +	struct sram_api *cur;
> +
> +	mutex_lock(&sram_api_list_lock);
> +	list_for_each_entry(cur, &sram_api_list, list) {
> +		if (cur->type == type) {
> +			kref_get(&cur->kref);
> +			mutex_unlock(&sram_api_list_lock);
> +			return cur;

Document that the reference count is incremented, or someone will forget
that.


> +		}
> +	}
> +	mutex_unlock(&sram_api_list_lock);
> +
> +	return NULL;
> +}
> +
> +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;

This feels really odd, what are you doing here with this loop to try to
find a space and then put it in and then change the resource???


> +			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);

No locking anywhere?

> +}
> +
> +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) {

Why is the offset somehow the "key" for this?

> +			list_del(&res->list);
> +			res->parent = NULL;
> +			return res;
> +		}
> +	}

No locking anywhere?

> +
> +	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;
> +	}

No lock???  No reference counting?

> +
> +	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 = -ENOIOCTLCMD;

-ENOTTY?

> +	int size;
> +	__u32 type;
> +
> +	if (!uapi)
> +		return ret;
> +
> +	switch (cmd) {
> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
> +		if (uapi->sa)
> +			return -EEXIST;
> +
> +		get_user(type, (const __u32 __user *)arg);
> +		uapi->sa = get_sram_api_from_type(type);
> +		if (uapi->sa)
> +			ret = 0;
> +		else
> +			ret = -ENODEV;
> +
> +		break;

Why does userspace have to set the type of sram?


> +
> +	case SRAM_UAPI_IOC_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,
> +							 PAGE_SIZE);

Look ma, userspace just allocated as much memory as it asked for!

Remember, all input is evil, check everything.

And, as is obvious now, this isn't really a "driver", it's a shim layer
for something else.  Who sets up sram_alloc()?

> +		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));

an alloc function copies a bunch of memory to userspace????

You need to document the heck out of this new interface as it really
does not make sense to me, sorry.

> +
> +		ret = 0;
> +		break;
> +
> +	case SRAM_UAPI_IOC_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);

Two step delete function with locking at all?  Ugh :(


> +
> +		ret = 0;
> +		break;
> +
> +	default:
> +		pr_err("error no cmd not supported\n");
> +		break;

Userspace now can spam the kernel log, do not do that.

> +	}
> +
> +	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);
> +	if (uapi->sa)
> +		kref_put(&uapi->sa->kref, NULL);

Um, hm, how do I put this nicely.

Wow, this is wrong.  So so so so so so wrong.

There's documentation in the kernel tree about how wrong this is.  The
kernel itself will warn you about how wrong this is.  Actually, no, it
will just crash.  If this function ever gets called your kernel just
blew up, so you really really really did not test this code at all.

{sigh}

Someone owes me a new bottle of whisky, this one is about to be empty...




> +
> +	kfree(uapi);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations sram_uapi_ops = {
> +	.owner		= THIS_MODULE,
> +	.open		= sram_uapi_open,
> +	.unlocked_ioctl = sram_uapi_ioctl,
> +	.compat_ioctl	= compat_ptr_ioctl,
> +	.mmap		= sram_uapi_mmap,
> +	.release	= sram_uapi_release,
> +};
> +
> +static struct miscdevice sram_uapi_miscdev = {
> +	MISC_DYNAMIC_MINOR,
> +	"sram",
> +	&sram_uapi_ops,

named identifiers please, this isn't the 1980's.

> +};
> +
> +static int __init sram_uapi_init(void)
> +{
> +	int ret;
> +
> +	INIT_LIST_HEAD(&sram_api_list);

Why do you need a static list?

> +	mutex_init(&sram_api_list_lock);

Why do you need a static lock?

> +
> +	ret = misc_register(&sram_uapi_miscdev);
> +	if (ret)
> +		pr_err("failed to register sram uapi misc device\n");

No need for the error, don't be noisy.

> +
> +	return ret;
> +}
> +
> +static void __exit sram_uapi_exit(void)
> +{
> +	misc_deregister(&sram_uapi_miscdev);
> +}
> +
> +module_init(sram_uapi_init);
> +module_exit(sram_uapi_exit);

drop the crazy static list and lock (again, this isn't the 1990's), and
you can get rid of the module_init/exit stuff too and just use a single
line to register the misc device.

Meta-comment here, you are trying to create a driver with a new api, but
you have no real users of this api, so no one knows if this really is
the correct api at all.  That's not ok, especially for one that is
purporting to be so "generic" as to claim it is a sram driver for
everyone.




> +
> +MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
> +MODULE_DESCRIPTION("SRAM User API Driver");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/sram_uapi.h b/include/linux/sram_uapi.h
> new file mode 100644
> index 000000000000..50fbf9dc308f
> --- /dev/null
> +++ b/include/linux/sram_uapi.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SRAM_UAPI_H
> +#define __SRAM_UAPI_H
> +
> +/* Set SRAM type to be accessed */
> +#define SRAM_UAPI_IOC_SET_SRAM_TYPE	_IOW('S', 0, __u32)
> +
> +/* Allocate resource from SRAM */
> +#define SRAM_UAPI_IOC_ALLOC		_IOWR('S', 1, struct res_info)
> +
> +/* Free allocated resource of SRAM */
> +#define SRAM_UAPI_IOC_FREE		_IOW('S', 2, struct res_info)
> +
> +struct sram_api {
> +	struct list_head	list;
> +	struct kref		kref;
> +	__u32			type;
> +	const char		*name;
> +
> +	long (*sram_alloc)(__u32 size, phys_addr_t *phys, __u32 align);
> +	void (*sram_free)(void *ptr);
> +};

Why is this structure in a uapi header file?

Oh wait, this isn't a uapi header file, this thing doesn't work at all.

ugh, this needs _so_ much work.

And again, someone owes me more whisky....

greg k-h

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Scott Wood]

On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
> +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;
> +		}
> +	}

We don't need yet another open coded allocator.  If you really need to do this
then use include/linux/genalloc.h, but maybe keep it simple and just have one
allocaton per file descriptor so you don't need to manage fd offsets?

> +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;
> +}

What if the allocation is more than one page, and the user mmaps starting
somewhere other than the first page?

> +	switch (cmd) {
> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
> +		if (uapi->sa)
> +			return -EEXIST;
> +
> +		get_user(type, (const __u32 __user *)arg);
> +		uapi->sa = get_sram_api_from_type(type);
> +		if (uapi->sa)
> +			ret = 0;
> +		else
> +			ret = -ENODEV;
> +
> +		break;
> +

Just expose one device per backing SRAM, especially if the user has any reason
to care about where the SRAM is coming from (correlating sysfs nodes is much
more expressive than some vague notion of "type").

> +	case SRAM_UAPI_IOC_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;

Missing EFAULT test (here and elsewhere), and res leaks on error.

> +
> +		res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
> +							 &res->phys,
> +							 PAGE_SIZE);

Do we really need multiple allocators, or could the backend be limited to just
adding regions to a generic allocator (with that allocator also serving in-
kernel users)?

If sram_alloc is supposed to return a virtual address, why isn't that the
return type?

> +		if (!res->virt) {
> +			kfree(res);
> +			return -ENOMEM;
> +		}

ENOSPC might be more appropriate, as this isn't general-purpose RAM.

> +
> +		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_IOC_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;

So you can just delete any arbitrary offset, even if you weren't the one that
allocated it?  Even if this isn't meant for unprivileged use it seems error-
prone.  

> +
> +	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);
> +}

Will noncached always be what's wanted here?

-Scott

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

Hi, Greg, Arnd,

Thank you for your comments first, and then really very very very sorry
for driving Greg to sigh and I hope there would be chance to share Moutai
(rather than whisky, we drink it much, a kind of Baijiu), after the virus.

Back to the comments, I'd like to do a bit of documentation or explanation first,
which should have been done early or else there would not be so much to explain:
1. What I have been trying to do is to access the Freescale Cache-SRAM device form
user level;
2. I implemented it using UIO, which was thought of non-proper;
3. So I implemented it here to create a misc device as a interface between Kernel
and user space applications, as the comments of Scott suggested;
[Link for 2 & 3: https://lore.kernel.org/patchwork/patch/1225798/]
4. This is exactly a shim of Cache-SRAM hardware level driver, and it would use
apis provided by the hardware driver to do the allocation and free work of
SRAM memory;
5. The file drivers/misc/sram.c actually has done some abstractions for SRAM access,
but it is used as a static way that resources are defined within devicetree;
6. This patch is to reach a way that allocate and release SRAM memory dynamically
which is much more convenient for user space applications, and for another reason,
the Cache-SRAM of Freescale is kind of not compatible with drivers/misc/sram.c;

So I implemented the sram_uapi.c as following:
1. Create a misc device as a interface that support ioctl and mmap for memory access;
2. For that I think this could be used for any SRAM devices that would support dynamic
memory allocation and release, I create sram_api_list as follow:

	static DEFINE_MUTEX(sram_api_list_lock);
	static LIST_HEAD(sram_api_list);

and different SRAM devices could add their apis to the sram_api_list
which could be used for the allocation from them;
	a. int sram_api_register(struct sram_api *sa);
	b. int sram_api_unregister(struct sram_api *sa);
	c. ioctl case SRAM_UAPI_IOC_SET_SRAM_TYPE;
the register and unregister api are exported for any SRAM drivers to do this,
and maybe user could chose one with ioctl to allocate;
[maybe only one SRAM device available currently and the implementation is redundant]
3. For each fd that opened, a sram_uapi would be created:

	struct sram_uapi {
		struct list_head	res_list;
		struct sram_api		*sa;
	};

The res_list would be a list of resources that allocated attached to it:

	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;
	};

The sa is a pointer to the apis registered by hardware SRAM driver as descibed earlier,
and when a fd is attached to it, kref it once;
And for the resources, it is accessed by the process only so I did not use a lock here,
and as Greg commented, lock is needed for the fd sharement.
4. The exported apis are currently not reference and I would add another patch series
for Freescale Cache-SRAM to use the api sram_api_register to register its allocation
and free api, as well as sram_api_unregister for unregistering;

Actually, I implemented the api list another way in v1, but was commented by Arnd not a
better way.
[https://lore.kernel.org/patchwork/patch/1226689/]

Then for the comments:
1. Ioctl: I will move the block to uapi, and use static length definition of __be64;
2. Naming: as Greg commented, so hard, and how do you think of
sram_dynamic(as compared to drivers/misc/sram.c)?
3. API list: only one SRAM device? But however, this is a shim, and hardware level
SRAM driver(s) should be enabled to register or init the apis for real allocation;
4. Test: My team and me myself did not cover it, especially newly added kref, and that
is really really so wrong of us, and I will make efforts to test for every logical path
next time, and this would never ever happen. Really sorry for it.

>On Sun, Apr 19, 2020 at 08:05:38PM -0700, Wang Wenhu 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 registered by specific SRAM hardware level driver to
>> the available list and then be chosen by users to allocate and map
>> SRAM memory from user level.
>> 
>> 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 a case.
>> 
>> 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
>> ---
>> 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
>> 
>> Note: From this on, the SRAM_UAPI driver is independent to any hardware
>> drivers, so I would only commit the patch itself as v2, while the v1 of
>> it was wrapped together with patches for Freescale L2-Cache-SRAM device.
>> Then after, I'd create patches for Freescale L2-Cache-SRAM device as
>> another series.
>> 
>> Link for v1:
>>  * https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@vivo.com/
>
>Why was this a RESEND?  What was wrong with the original v2 version?
>
>Anyway, minor review comments:
>
>> ---
>>  drivers/misc/Kconfig      |  11 ++
>>  drivers/misc/Makefile     |   1 +
>>  drivers/misc/sram_uapi.c  | 352 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/sram_uapi.h |  28 +++
>>  4 files changed, 392 insertions(+)
>>  create mode 100644 drivers/misc/sram_uapi.c
>>  create mode 100644 include/linux/sram_uapi.h
>> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 99e151475d8f..b19c8b24f18e 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -465,6 +465,17 @@ 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.
>
>Naming is hard, but shouldn't this just be "sram" and drop the whole
>"UAPI" stuff everywhere?  That doesn't make much sense as drivers are
>just interfaces to userspace...
>
>
>> +
>>  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..66c7b56b635f
>> --- /dev/null
>> +++ b/drivers/misc/sram_uapi.c
>> @@ -0,0 +1,352 @@
>> +// 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/fs.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/sram_uapi.h>
>> +
>> +#define DRIVER_NAME	"sram_uapi"
>
>If you really need this, just use KBUILD_MODNAME instead.  But I don't
>think you need it, as most drivers do not.
>

Yes, no need here.

>
>> +
>> +struct res_info {
>> +	phys_addr_t offset;
>> +	phys_addr_t 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_uapi {
>> +	struct list_head	res_list;
>> +	struct sram_api		*sa;
>> +};
>> +
>> +static DEFINE_MUTEX(sram_api_list_lock);
>> +static LIST_HEAD(sram_api_list);
>> +
>> +long sram_api_register(struct sram_api *sa)
>
>Why are all of these functions global?
>
>And shouldn't this return an 'int'?
>

Should be int, and as explained upper, this is to be called by
hardware driver like Freescale Cache-SRAM to register their
allocation and free apis to the sram_api_list.

>> +{
>> +	struct sram_api *cur;
>> +
>> +	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
>> +		return -EINVAL;
>
>How can any of these be true?  You control all the callers of this
>function.

Allocation and free apis are really needed, while name maybe is just
a superfluous choice(which may be added to attrs), I will drop it.

>
>> +
>> +	mutex_lock(&sram_api_list_lock);
>> +	list_for_each_entry(cur, &sram_api_list, list) {
>> +		if (cur->type == sa->type) {
>> +			pr_err("error sram %s type %d exists\n", sa->name,
>> +			       sa->type);
>> +			mutex_unlock(&sram_api_list_lock);
>> +			return -EEXIST;
>> +		}
>> +	}
>> +
>> +	kref_init(&sa->kref);
>
>So the caller has to allocate the space and set some things up, but not
>everything?  That's a mess, why not just do it either all at once with
>an "allocate" like function, or just do it all in one function.  Why is
>this a separate function anyway?
>

Should be done by caller.

>> +	list_add_tail(&sa->list, &sram_api_list);
>> +	pr_info("sram %s type %d registered\n", sa->name, sa->type);
>
>Don't leave debugging comments in the driver, if all goes well a driver
>should be totally silent.  Only log errors.
>
>> +
>> +	mutex_unlock(&sram_api_list_lock);
>> +
>> +	return 0;
>> +};
>> +EXPORT_SYMBOL(sram_api_register);
>
>Exported for what?  This is a stand-alone driver, nothing else uses
>these symbols.
>

To be added to the hardware drivers, and currently Freescale Cache-SRAM.
(So I should have done all these within one series?)

>> +
>> +long sram_api_unregister(struct sram_api *sa)
>
>Why long?

int everywhere(rather than uapi, I was influenced by ioctl return type).

>
>> +{
>> +	struct sram_api *cur, *tmp;
>> +	long ret = -ENODEV;
>> +
>> +	if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
>> +		return -EINVAL;
>
>Again, how can any of this be true?
>
>> +
>> +	mutex_lock(&sram_api_list_lock);
>> +	list_for_each_entry_safe(cur, tmp, &sram_api_list, list) {
>> +		if (cur->type == sa->type && !strcmp(cur->name, sa->name)) {
>> +			if (kref_read(&cur->kref)) {
>> +				pr_err("error sram %s type %d is busy\n",
>> +					sa->name, sa->type);
>> +				ret = -EBUSY;
>> +			} else {
>> +				list_del(&cur->list);
>> +				pr_info("sram %s type %d unregistered\n",
>> +					sa->name, sa->type);
>> +				ret = 0;
>> +			}
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&sram_api_list_lock);
>> +
>> +	return ret;
>> +};
>> +EXPORT_SYMBOL(sram_api_unregister);
>> +
>> +static struct sram_api *get_sram_api_from_type(__u32 type)
>> +{
>> +	struct sram_api *cur;
>> +
>> +	mutex_lock(&sram_api_list_lock);
>> +	list_for_each_entry(cur, &sram_api_list, list) {
>> +		if (cur->type == type) {
>> +			kref_get(&cur->kref);
>> +			mutex_unlock(&sram_api_list_lock);
>> +			return cur;
>
>Document that the reference count is incremented, or someone will forget
>that.
>
I will.
>
>> +		}
>> +	}
>> +	mutex_unlock(&sram_api_list_lock);
>> +
>> +	return NULL;
>> +}
>> +
>> +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;
>
>This feels really odd, what are you doing here with this loop to try to
>find a space and then put it in and then change the resource???
>
Like commets from Scott, I used the fd to allocate a lot of memory areas and they are
managed with different offset. So I just need to keep them in a sequence and no overlap.
The parent is useless and I will delete it. Anyway, things should be done before put it in.

I may change it according to the comments from Scott, that a fd only be attached to one
memory area which looks exactly like the thing in drivers/misc/sram.c. And then, this block
is useless and will be dropped.
 
>
>> +			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);
>
>No locking anywhere?

This is attached to the fd, and may be co-accessed, should be locked for access.

>
>> +}
>> +
>> +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) {
>
>Why is the offset somehow the "key" for this?

A fd attached to a lot of memory areas, and they are arranged by offset within fd.

>
>> +			list_del(&res->list);
>> +			res->parent = NULL;
>> +			return res;
>> +		}
>> +	}
>
>No locking anywhere?
>
>> +
>> +	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;
>> +	}
>
>No lock???  No reference counting?

As addressed upper, should use lock. And yes, should be reference counted.

>
>> +
>> +	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 = -ENOIOCTLCMD;
>
>-ENOTTY?

Yes.

>
>> +	int size;
>> +	__u32 type;
>> +
>> +	if (!uapi)
>> +		return ret;
>> +
>> +	switch (cmd) {
>> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>> +		if (uapi->sa)
>> +			return -EEXIST;
>> +
>> +		get_user(type, (const __u32 __user *)arg);
>> +		uapi->sa = get_sram_api_from_type(type);
>> +		if (uapi->sa)
>> +			ret = 0;
>> +		else
>> +			ret = -ENODEV;
>> +
>> +		break;
>
>Why does userspace have to set the type of sram?
>
Currently only one SRAM available on SoC, if as descriped upper there
are more, user fd could select which one it would be attached to.
>
>> +
>> +	case SRAM_UAPI_IOC_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,
>> +							 PAGE_SIZE);
>
>Look ma, userspace just allocated as much memory as it asked for!
>
>Remember, all input is evil, check everything.
>
>And, as is obvious now, this isn't really a "driver", it's a shim layer
>for something else.  Who sets up sram_alloc()?
>

Yes, a shim, and sram_api_register exported to be called by hardware driver
to register it. Params should be checked and I guess the size checked by
lower alloc api?(-ENOMEM or something be returned by it if not afforded to?)

Actually in v1, I used a array to reference the apis statically, and wrapped
with specific MICROs, and changed it this way according to the comments of Arnd.
Link: https://lore.kernel.org/patchwork/patch/1226689/

>> +		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));
>
>an alloc function copies a bunch of memory to userspace????

Tell the userspace the offset of area within the fd, which is the key
as you mentioned upper, and could be used for SRAM_UAPI_IOC_FREE to release,
and mmap to finally attach it to the vma of user process.

>
>You need to document the heck out of this new interface as it really
>does not make sense to me, sorry.
>
>> +
>> +		ret = 0;
>> +		break;
>> +
>> +	case SRAM_UAPI_IOC_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);
>
>Two step delete function with locking at all?  Ugh :(
>

As upper, should be locked.

>> +
>> +		ret = 0;
>> +		break;
>> +
>> +	default:
>> +		pr_err("error no cmd not supported\n");
>> +		break;
>
>Userspace now can spam the kernel log, do not do that.
>

Learned.

>> +	}
>> +
>> +	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);
>> +	if (uapi->sa)
>> +		kref_put(&uapi->sa->kref, NULL);
>
>Um, hm, how do I put this nicely.
>
>Wow, this is wrong.  So so so so so so wrong.
>
>There's documentation in the kernel tree about how wrong this is.  The
>kernel itself will warn you about how wrong this is.  Actually, no, it
>will just crash.  If this function ever gets called your kernel just
>blew up, so you really really really did not test this code at all.
>
>{sigh}
>
>Someone owes me a new bottle of whisky, this one is about to be empty...
>
>
I will check it again and again and again and again. So so so so Sorry.
And I would catch up with it after another checking done.
>
>
>> +
>> +	kfree(uapi);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations sram_uapi_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= sram_uapi_open,
>> +	.unlocked_ioctl = sram_uapi_ioctl,
>> +	.compat_ioctl	= compat_ptr_ioctl,
>> +	.mmap		= sram_uapi_mmap,
>> +	.release	= sram_uapi_release,
>> +};
>> +
>> +static struct miscdevice sram_uapi_miscdev = {
>> +	MISC_DYNAMIC_MINOR,
>> +	"sram",
>> +	&sram_uapi_ops,
>
>named identifiers please, this isn't the 1980's.
>

? you mean MISC_DYNAMIC_MINOR?

>> +};
>> +
>> +static int __init sram_uapi_init(void)
>> +{
>> +	int ret;
>> +
>> +	INIT_LIST_HEAD(&sram_api_list);
>
>Why do you need a static list?
>
>> +	mutex_init(&sram_api_list_lock);
>
>Why do you need a static lock?
>

Like we have there SRAM or anything like it that would call exported
symbol sram_api_unregister, this is to keep it safe.

>> +
>> +	ret = misc_register(&sram_uapi_miscdev);
>> +	if (ret)
>> +		pr_err("failed to register sram uapi misc device\n");
>
>No need for the error, don't be noisy.

Got it.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static void __exit sram_uapi_exit(void)
>> +{
>> +	misc_deregister(&sram_uapi_miscdev);
>> +}
>> +
>> +module_init(sram_uapi_init);
>> +module_exit(sram_uapi_exit);
>
>drop the crazy static list and lock (again, this isn't the 1990's), and
>you can get rid of the module_init/exit stuff too and just use a single
>line to register the misc device.
>
>Meta-comment here, you are trying to create a driver with a new api, but
>you have no real users of this api, so no one knows if this really is
>the correct api at all.  That's not ok, especially for one that is
>purporting to be so "generic" as to claim it is a sram driver for
>everyone.
>
>
As mentioned before, maybe I should have added the caller of sram_api_unregister
in Freescale Cache-SRAM driver too in a series.
>
>
>> +
>> +MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
>> +MODULE_DESCRIPTION("SRAM User API Driver");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/sram_uapi.h b/include/linux/sram_uapi.h
>> new file mode 100644
>> index 000000000000..50fbf9dc308f
>> --- /dev/null
>> +++ b/include/linux/sram_uapi.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SRAM_UAPI_H
>> +#define __SRAM_UAPI_H
>> +
>> +/* Set SRAM type to be accessed */
>> +#define SRAM_UAPI_IOC_SET_SRAM_TYPE	_IOW('S', 0, __u32)
>> +
>> +/* Allocate resource from SRAM */
>> +#define SRAM_UAPI_IOC_ALLOC		_IOWR('S', 1, struct res_info)
>> +
>> +/* Free allocated resource of SRAM */
>> +#define SRAM_UAPI_IOC_FREE		_IOW('S', 2, struct res_info)
>> +
>> +struct sram_api {
>> +	struct list_head	list;
>> +	struct kref		kref;
>> +	__u32			type;
>> +	const char		*name;
>> +
>> +	long (*sram_alloc)(__u32 size, phys_addr_t *phys, __u32 align);
>> +	void (*sram_free)(void *ptr);
>> +};
>
>Why is this structure in a uapi header file?
>
>Oh wait, this isn't a uapi header file, this thing doesn't work at all.

What can I say......

>
>ugh, this needs _so_ much work.
I will do it do it and do it.
>
>And again, someone owes me more whisky....
Sorry for that
>

Thanks and regards,
Wenhu

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Greg KH]

On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
> Hi, Greg, Arnd,
> 
> Thank you for your comments first, and then really very very very sorry
> for driving Greg to sigh and I hope there would be chance to share Moutai
> (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
> 
> Back to the comments, I'd like to do a bit of documentation or explanation first,
> which should have been done early or else there would not be so much to explain:
> 1. What I have been trying to do is to access the Freescale Cache-SRAM device form
> user level;
> 2. I implemented it using UIO, which was thought of non-proper;

I still think that using uio is the best way to do this, and never said
it was "non-proper".  All we got bogged down in was the DT
representation of stuff from what I remember.  That should be worked
through.

thanks,

greg k-h

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

>On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
>> Hi, Greg, Arnd,
>> 
>> Thank you for your comments first, and then really very very very sorry
>> for driving Greg to sigh and I hope there would be chance to share Moutai
>> (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
>> 
>> Back to the comments, I'd like to do a bit of documentation or explanation first,
>> which should have been done early or else there would not be so much to explain:
>> 1. What I have been trying to do is to access the Freescale Cache-SRAM device form
>> user level;
>> 2. I implemented it using UIO, which was thought of non-proper;
>
>I still think that using uio is the best way to do this, and never said
>it was "non-proper".  All we got bogged down in was the DT
>representation of stuff from what I remember.  That should be worked
>through.
>
>thanks,
>
>greg k-h

Surely, but so how would things go? Scott said not fit for him. And he was
gonna to write a new patch(Oh,  that is what I have been doing.....,and I really
donot think he need to do it)

This is the final version using UIO, and even Christophe had Reviewed-by,
https://lore.kernel.org/patchwork/patch/1226225/

If no ending reaches, I have to make a step forward to keep working with
the misc device version.

Thanks, and regards,
Wenhu

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

Hi, Scott, Greg,

Thank you for your helpful comments.
For that Greg mentioned that the patch (or patch series) via UIO should worked through,
so I want to make it clear that if it would go upstream?(And if so, when? No push, just ask)

Also I have been wondering how the patches with components in different subsystems
go get upstream to the mainline? Like patch 1-3 are of linuxppc-dev, and patch 4 is of
subsystem UIO, and if acceptable, how would you deal with them?

Back to the devicetree thing, I make it detached from hardware compatibilities which belong
to the hardware level driver and also used module parameter for of_id definition as dt-binding
is not allowed for UIO now. So as I can see, things may go well and there is no harm to anything,
I hope you(Scott) please take a re-consideration. 

Thanks & regards,
Wenhu

>On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
>> +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;
>> +		}
>> +	}
>
>We don't need yet another open coded allocator.  If you really need to do this
>then use include/linux/genalloc.h, but maybe keep it simple and just have one
>allocaton per file descriptor so you don't need to manage fd offsets?
>
>> +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;
>> +}
>
>What if the allocation is more than one page, and the user mmaps starting
>somewhere other than the first page?
>
>> +	switch (cmd) {
>> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>> +		if (uapi->sa)
>> +			return -EEXIST;
>> +
>> +		get_user(type, (const __u32 __user *)arg);
>> +		uapi->sa = get_sram_api_from_type(type);
>> +		if (uapi->sa)
>> +			ret = 0;
>> +		else
>> +			ret = -ENODEV;
>> +
>> +		break;
>> +
>
>Just expose one device per backing SRAM, especially if the user has any reason
>to care about where the SRAM is coming from (correlating sysfs nodes is much
>more expressive than some vague notion of "type").
>
>> +	case SRAM_UAPI_IOC_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;
>
>Missing EFAULT test (here and elsewhere), and res leaks on error.
>
>> +
>> +		res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>> +							 &res->phys,
>> +							 PAGE_SIZE);
>
>Do we really need multiple allocators, or could the backend be limited to just
>adding regions to a generic allocator (with that allocator also serving in-
>kernel users)?
>
>If sram_alloc is supposed to return a virtual address, why isn't that the
>return type?
>
>> +		if (!res->virt) {
>> +			kfree(res);
>> +			return -ENOMEM;
>> +		}
>
>ENOSPC might be more appropriate, as this isn't general-purpose RAM.
>
>> +
>> +		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_IOC_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;
>
>So you can just delete any arbitrary offset, even if you weren't the one that
>allocated it?  Even if this isn't meant for unprivileged use it seems error-
>prone.  
>
>> +
>> +	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);
>> +}
>
>Will noncached always be what's wanted here?
>
>-Scott
>
>

Re:Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

>Hi, Scott, Greg,
>
>Thank you for your helpful comments.
>For that Greg mentioned that the patch (or patch series) via UIO should worked through,
>so I want to make it clear that if it would go upstream?(And if so, when? No push, just ask)
>
>Also I have been wondering how the patches with components in different subsystems
>go get upstream to the mainline? Like patch 1-3 are of linuxppc-dev, and patch 4 is of
>subsystem UIO, and if acceptable, how would you deal with them?
>
>Back to the devicetree thing, I make it detached from hardware compatibilities which belong
>to the hardware level driver and also used module parameter for of_id definition as dt-binding
>is not allowed for UIO now. So as I can see, things may go well and there is no harm to anything,
>I hope you(Scott) please take a re-consideration. 
>

I mean I have get some new work done based on the comments of Arnd, Scott and Greg. Also a lot of tests done.
So it would be better to make it clear whether I shoud keep the work going or the UIO version is to be accepted
to go upstream recently in the future.

Thanks & regards,
Wenhu
>
>>On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
>>> +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;
>>> +		}
>>> +	}
>>
>>We don't need yet another open coded allocator.  If you really need to do this
>>then use include/linux/genalloc.h, but maybe keep it simple and just have one
>>allocaton per file descriptor so you don't need to manage fd offsets?
>>
>>> +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;
>>> +}
>>
>>What if the allocation is more than one page, and the user mmaps starting
>>somewhere other than the first page?
>>
>>> +	switch (cmd) {
>>> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>>> +		if (uapi->sa)
>>> +			return -EEXIST;
>>> +
>>> +		get_user(type, (const __u32 __user *)arg);
>>> +		uapi->sa = get_sram_api_from_type(type);
>>> +		if (uapi->sa)
>>> +			ret = 0;
>>> +		else
>>> +			ret = -ENODEV;
>>> +
>>> +		break;
>>> +
>>
>>Just expose one device per backing SRAM, especially if the user has any reason
>>to care about where the SRAM is coming from (correlating sysfs nodes is much
>>more expressive than some vague notion of "type").
>>
>>> +	case SRAM_UAPI_IOC_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;
>>
>>Missing EFAULT test (here and elsewhere), and res leaks on error.
>>
>>> +
>>> +		res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>>> +							 &res->phys,
>>> +							 PAGE_SIZE);
>>
>>Do we really need multiple allocators, or could the backend be limited to just
>>adding regions to a generic allocator (with that allocator also serving in-
>>kernel users)?
>>
>>If sram_alloc is supposed to return a virtual address, why isn't that the
>>return type?
>>
>>> +		if (!res->virt) {
>>> +			kfree(res);
>>> +			return -ENOMEM;
>>> +		}
>>
>>ENOSPC might be more appropriate, as this isn't general-purpose RAM.
>>
>>> +
>>> +		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_IOC_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;
>>
>>So you can just delete any arbitrary offset, even if you weren't the one that
>>allocated it?  Even if this isn't meant for unprivileged use it seems error-
>>prone.  
>>
>>> +
>>> +	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);
>>> +}
>>
>>Will noncached always be what's wanted here?
>>
>>-Scott
>>
>>
>
>

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Scott Wood]

On Tue, 2020-04-21 at 11:34 +0200, Greg KH wrote:
> On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
> > Hi, Greg, Arnd,
> > 
> > Thank you for your comments first, and then really very very very sorry
> > for driving Greg to sigh and I hope there would be chance to share Moutai
> > (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
> > 
> > Back to the comments, I'd like to do a bit of documentation or explanation
> > first,
> > which should have been done early or else there would not be so much to
> > explain:
> > 1. What I have been trying to do is to access the Freescale Cache-SRAM
> > device form
> > user level;
> > 2. I implemented it using UIO, which was thought of non-proper;
> 
> I still think that using uio is the best way to do this, and never said
> it was "non-proper".  All we got bogged down in was the DT
> representation of stuff from what I remember.  That should be worked
> through.

The hardware is already reperesented in the DT (the various "fsl,<chip>-l2-
cache-controller" nodes).  What is there to "work through"?

I didn't say UIO was "non-proper" though I did question whether it was the
best fit.  We don't need the IRQ stuff, and we do want some means of
allocating multiple regions to different users (at least, that seems to be a
requirement for Wenhu, and it leaves open the possibility of a kernel driver
allocating some SRAM for itself which appears to be what
arch/powerpc/sysdev/fsl_85xx_cache_sram.c was originally meant for) and I
don't see how you'd do that through UIO.

So that leaves either a separate interface for dynamic region allocation (in
which case why not map through that interface), static allocation via
boot/module parameters which you didn't like, or abusing the device tree with
something that's not hardware description (why don't we replace kmalloc with
some device tree nodes while we're at it).

-Scott

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Rob Herring]

On Sun, Apr 19, 2020 at 10:06 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 registered by specific SRAM hardware level driver to
> the available list and then be chosen by users to allocate and map
> SRAM memory from user level.
>
> 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 a case.
>
> 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
> ---
> 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
>
> Note: From this on, the SRAM_UAPI driver is independent to any hardware
> drivers, so I would only commit the patch itself as v2, while the v1 of
> it was wrapped together with patches for Freescale L2-Cache-SRAM device.
> Then after, I'd create patches for Freescale L2-Cache-SRAM device as
> another series.

There's work to add SRAM support to dma-buf heaps[1]. Take a look and
see if that works for you.

Rob

[1] https://lore.kernel.org/lkml/20200424222740.16259-1-afd@ti.com/

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[Scott Wood]

On Mon, 2020-04-27 at 09:13 -0500, Rob Herring wrote:
> On Sun, Apr 19, 2020 at 10:06 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 registered by specific SRAM hardware level driver to
> > the available list and then be chosen by users to allocate and map
> > SRAM memory from user level.
> > 
> > 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 a case.
> > 
> > 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
> > ---
> > 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
> > 
> > Note: From this on, the SRAM_UAPI driver is independent to any hardware
> > drivers, so I would only commit the patch itself as v2, while the v1 of
> > it was wrapped together with patches for Freescale L2-Cache-SRAM device.
> > Then after, I'd create patches for Freescale L2-Cache-SRAM device as
> > another series.
> 
> There's work to add SRAM support to dma-buf heaps[1]. Take a look and
> see if that works for you.
> 
> Rob
> 
> [1] https://lore.kernel.org/lkml/20200424222740.16259-1-afd@ti.com/
> 

The dma heap API itself (what makes it specific to DMA, rather than any
special-purpose allocator?) seems like it could be what we're looking for. 
The issue with drivers/misc/sram.c is that it seems like its main purpose is
to get sram description from the device tree, but this sram isn't static (it's
a reconfiguration of L2 cache into SRAM mode) and thus can't be described by
mmio-sram.

-Scott