Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
On Mon, 2020-04-27 at 09:13 -0500, Rob Herring wrote: > On Sun, Apr 19, 2020 at 10:06 PM 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 > > Cc: Greg Kroah-Hartman > > Cc: Arnd Bergmann > > Cc: Christophe Leroy > > Cc: Scott Wood > > Cc: Michael Ellerman > > Cc: Randy Dunlap > > 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-...@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
Re: [PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
On Sun, Apr 19, 2020 at 10:06 PM 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 > Cc: Greg Kroah-Hartman > Cc: Arnd Bergmann > Cc: Christophe Leroy > Cc: Scott Wood > Cc: Michael Ellerman > Cc: Randy Dunlap > 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-...@ti.com/
Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
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,-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: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 = >res_list;
>>> +
>>> + list_for_each_entry_safe(cur, tmp, head, list) {
>>> + if (>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(>list, >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, >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 *)>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,
>>> +>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 *)>info,
>>> + sizeof(res->info));
>>> +
>>> + ret = 0;
>>> + break;
>>> +
>>> + case SRAM_UAPI_IOC_FREE:
>>> + if (!uapi->sa)
>>> + return -EINVAL;
>>> +
>>> + size = copy_from_user((void *), (const void __user *)arg,
>>> +
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 = >res_list;
>> +
>> +list_for_each_entry_safe(cur, tmp, head, list) {
>> +if (>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(>list, >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, >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 *)>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,
>> + >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 *)>info,
>> +sizeof(res->info));
>> +
>> +ret = 0;
>> +break;
>> +
>> +case SRAM_UAPI_IOC_FREE:
>> +if (!uapi->sa)
>> +return -EINVAL;
>> +
>> +size = copy_from_user((void *), (const void __user *)arg,
>> + sizeof(info));
>> +
>> +res = sram_uapi_res_delete(uapi, );
>> +if (!res) {
>> +pr_err("error no sram resource found\n");
>> +return -EINVAL;
>> +}
>> +
>> +uapi->sa->sram_free(res->virt);
>> +kfree(res);
>> +
>> +ret
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
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
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_headres_list;
struct sram_api *sa;
};
The res_list would be a list of resources that allocated attached to it:
struct sram_resource {
struct list_headlist;
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
>> Cc: Greg Kroah-Hartman
>> Cc: Arnd Bergmann
>> Cc: Christophe Leroy
>> Cc: Scott Wood
>> Cc: Michael Ellerman
>> Cc: Randy Dunlap
>> Cc:
Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
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 = >res_list;
> +
> + list_for_each_entry_safe(cur, tmp, head, list) {
> + if (>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(>list, >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, >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 *)>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,
> + >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 *)>info,
> + sizeof(res->info));
> +
> + ret = 0;
> + break;
> +
> + case SRAM_UAPI_IOC_FREE:
> + if (!uapi->sa)
> + return -EINVAL;
> +
> + size = copy_from_user((void *), (const void __user *)arg,
> + sizeof(info));
> +
> + res = sram_uapi_res_delete(uapi, );
> + 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,
>
Re: [PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
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
> Cc: Greg Kroah-Hartman
> Cc: Arnd Bergmann
> Cc: Christophe Leroy
> Cc: Scott Wood
> Cc: Michael Ellerman
> Cc: Randy Dunlap
> 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 ..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
> + * All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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_headlist;
> + struct res_info info;
> + phys_addr_t phys;
> + void*virt;
> + struct vm_area_struct *vma;
> + struct sram_uapi*parent;
> +};
> +
> +struct sram_uapi {
> + struct list_headres_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;
> +
> +
Re: [PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
On Mon, Apr 20, 2020 at 5:06 AM 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
> Cc: Greg Kroah-Hartman
> Cc: Arnd Bergmann
> Cc: Christophe Leroy
> Cc: Scott Wood
> Cc: Michael Ellerman
> Cc: Randy Dunlap
> 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(_api_list);
> + mutex_init(_api_list_lock);
> +
> + ret = misc_register(_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(_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
[PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
A generic User-Kernel interface that allows a misc device created
by it to support file-operations of ioctl and mmap to access SRAM
memory from user level. Different kinds of SRAM alloction and free
APIs could be 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
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: Randy Dunlap
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/
---
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 ..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
+ * All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRIVER_NAME"sram_uapi"
+
+struct res_info {
+ phys_addr_t offset;
+ phys_addr_t size;
+};
+
+struct sram_resource {
+ struct list_headlist;
+ struct res_info info;
+ phys_addr_t phys;
+ void*virt;
+ struct vm_area_struct *vma;
+ struct sram_uapi*parent;
+};
+
+struct sram_uapi {
+ struct list_headres_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(_api_list_lock);
+ list_for_each_entry(cur, _api_list, list) {
+ if (cur->type == sa->type) {
+ pr_err("error sram %s type %d exists\n", sa->name,
+ sa->type);
+ mutex_unlock(_api_list_lock);
+ return -EEXIST;
+ }
+ }
+
+ kref_init(>kref);
+ list_add_tail(>list, _api_list);
+ pr_info("sram %s type %d registered\n", sa->name, sa->type);
+
+ mutex_unlock(_api_list_lock);
+
+ return 0;
+};
+EXPORT_SYMBOL(sram_api_register);
+
+long sram_api_unregister(struct
