Message-ID: <AB6A-gBhCLyx5bgBRPuIiqp1.3.1587460187094.Hmail.wenhu.wang@vivo.com>
Date:   Tue, 21 Apr 2020 17:09:47 +0800 (GMT+08:00)
From:   王文虎 <wenhu.wang@...o.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     arnd@...db.de, linux-kernel@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, kernel@...o.com, robh@...nel.org,
        Christophe Leroy <christophe.leroy@....fr>,
        Scott Wood <oss@...error.net>,
        Michael Ellerman <mpe@...erman.id.au>,
        Randy Dunlap <rdunlap@...radead.org>
Subject: 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@...o.com>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> Cc: Arnd Bergmann <arnd@...db.de>
>> Cc: Christophe Leroy <christophe.leroy@....fr>
>> Cc: Scott Wood <oss@...error.net>
>> Cc: Michael Ellerman <mpe@...erman.id.au>
>> Cc: Randy Dunlap <rdunlap@...radead.org>
>> Cc: linuxppc-dev@...ts.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@...o.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@...o.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

Powered by blists - more mailing lists