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

[王文虎]

Hi, Scott, Greg,

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

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

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

Thanks & regards,
Wenhu

>On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
>> +static void sram_uapi_res_insert(struct sram_uapi *uapi,
>> +				 struct sram_resource *res)
>> +{
>> +	struct sram_resource *cur, *tmp;
>> +	struct list_head *head = &uapi->res_list;
>> +
>> +	list_for_each_entry_safe(cur, tmp, head, list) {
>> +		if (&tmp->list != head &&
>> +		    (cur->info.offset + cur->info.size + res->info.size <=
>> +		    tmp->info.offset)) {
>> +			res->info.offset = cur->info.offset + cur->info.size;
>> +			res->parent = uapi;
>> +			list_add(&res->list, &cur->list);
>> +			return;
>> +		}
>> +	}
>
>We don't need yet another open coded allocator.  If you really need to do this
>then use include/linux/genalloc.h, but maybe keep it simple and just have one
>allocaton per file descriptor so you don't need to manage fd offsets?
>
>> +static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
>> +						__u32 offset)
>> +{
>> +	struct sram_resource *res;
>> +
>> +	list_for_each_entry(res, &uapi->res_list, list) {
>> +		if (res->info.offset == offset)
>> +			return res;
>> +	}
>> +
>> +	return NULL;
>> +}
>
>What if the allocation is more than one page, and the user mmaps starting
>somewhere other than the first page?
>
>> +	switch (cmd) {
>> +	case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>> +		if (uapi->sa)
>> +			return -EEXIST;
>> +
>> +		get_user(type, (const __u32 __user *)arg);
>> +		uapi->sa = get_sram_api_from_type(type);
>> +		if (uapi->sa)
>> +			ret = 0;
>> +		else
>> +			ret = -ENODEV;
>> +
>> +		break;
>> +
>
>Just expose one device per backing SRAM, especially if the user has any reason
>to care about where the SRAM is coming from (correlating sysfs nodes is much
>more expressive than some vague notion of "type").
>
>> +	case SRAM_UAPI_IOC_ALLOC:
>> +		if (!uapi->sa)
>> +			return -EINVAL;
>> +
>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +		if (!res)
>> +			return -ENOMEM;
>> +
>> +		size = copy_from_user((void *)&res->info,
>> +				      (const void __user *)arg,
>> +				      sizeof(res->info));
>> +		if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
>> +			return -EINVAL;
>
>Missing EFAULT test (here and elsewhere), and res leaks on error.
>
>> +
>> +		res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>> +							 &res->phys,
>> +							 PAGE_SIZE);
>
>Do we really need multiple allocators, or could the backend be limited to just
>adding regions to a generic allocator (with that allocator also serving in-
>kernel users)?
>
>If sram_alloc is supposed to return a virtual address, why isn't that the
>return type?
>
>> +		if (!res->virt) {
>> +			kfree(res);
>> +			return -ENOMEM;
>> +		}
>
>ENOSPC might be more appropriate, as this isn't general-purpose RAM.
>
>> +
>> +		sram_uapi_res_insert(uapi, res);
>> +		size = copy_to_user((void __user *)arg,
>> +				    (const void *)&res->info,
>> +				    sizeof(res->info));
>> +
>> +		ret = 0;
>> +		break;
>> +
>> +	case SRAM_UAPI_IOC_FREE:
>> +		if (!uapi->sa)
>> +			return -EINVAL;
>> +
>> +		size = copy_from_user((void *)&info, (const void __user *)arg,
>> +				      sizeof(info));
>> +
>> +		res = sram_uapi_res_delete(uapi, &info);
>> +		if (!res) {
>> +			pr_err("error no sram resource found\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		uapi->sa->sram_free(res->virt);
>> +		kfree(res);
>> +
>> +		ret = 0;
>> +		break;
>
>So you can just delete any arbitrary offset, even if you weren't the one that
>allocated it?  Even if this isn't meant for unprivileged use it seems error-
>prone.  
>
>> +
>> +	default:
>> +		pr_err("error no cmd not supported\n");
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int sram_uapi_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> +	struct sram_uapi *uapi = filp->private_data;
>> +	struct sram_resource *res;
>> +
>> +	res = sram_uapi_find_res(uapi, vma->vm_pgoff);
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	if (vma->vm_end - vma->vm_start > res->info.size)
>> +		return -EINVAL;
>> +
>> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +
>> +	return remap_pfn_range(vma, vma->vm_start,
>> +			       res->phys >> PAGE_SHIFT,
>> +			       vma->vm_end - vma->vm_start,
>> +			       vma->vm_page_prot);
>> +}
>
>Will noncached always be what's wanted here?
>
>-Scott
>
>