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.w...@vivo.com> >> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> >> Cc: Arnd Bergmann <a...@arndb.de> >> Cc: Christophe Leroy <christophe.le...@c-s.fr> >> Cc: Scott Wood <o...@buserror.net> >> Cc: Michael Ellerman <m...@ellerman.id.au> >> Cc: Randy Dunlap <rdun...@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.w...@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.w...@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.w...@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