Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
王文虎
wenhu.wang at vivo.com
Tue Apr 21 19:09:47 AEST 2020
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 at vivo.com>
>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> Cc: Arnd Bergmann <arnd at arndb.de>
>> Cc: Christophe Leroy <christophe.leroy at c-s.fr>
>> Cc: Scott Wood <oss at buserror.net>
>> Cc: Michael Ellerman <mpe at ellerman.id.au>
>> Cc: Randy Dunlap <rdunlap at infradead.org>
>> Cc: linuxppc-dev at 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 at 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 at 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
More information about the Linuxppc-dev
mailing list