Messages in this thread | | | Subject | Re: [PATCH v6,4/4] drivers: misc: new driver sram uapi for user level SRAM access | From | 王文虎 <> | Date | Sun, 19 Apr 2020 15:25:35 +0800 (GMT+08:00) |
| |
>> 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 added to the available array and could be configured >> from user level. > >Having a generic user level interface seem reasonable, but it would >be helpful to list one or more particular use cases.
OK, I will use the FSL_85XX_SRAM as a case to describe it.
> >> +if SRAM_UAPI >> + >> +config FSL_85XX_SRAM_UAPI >> + bool "Freescale MPC85xx Cache-SRAM UAPI support" >> + depends on FSL_SOC_BOOKE && PPC32 >> + select FSL_85XX_CACHE_SRAM >> + help >> + This adds the Freescale MPC85xx Cache-SRAM memory allocation and >> + free interfaces to the available SRAM API array, which finally could >> + be used from user level to access the Freescale MPC85xx Cache-SRAM >> + memory. > >Why do you need a hardware specific Kconfig option here, shouldn't >this just use the generic kernel abstraction for the sram? > Yes, I will add a interface for any hardware drivers to register there specific apis instead of the definition here.
>> +struct sram_api { >> + u32 type; >> + long (*sram_alloc)(u32 size, phys_addr_t *phys, u32 align); >> + void (*sram_free)(void *ptr); >> +}; >> + >> +struct sram_uapi { >> + struct list_head res_list; >> + struct sram_api *sa; >> +}; >> + >> +enum SRAM_TYPE { >> +#ifdef FSL_85XX_CACHE_SRAM >> + SRAM_TYPE_FSL_85XX_CACHE_SRAM, >> +#endif >> + SRAM_TYPE_MAX, >> +}; >> + >> +/* keep the SRAM_TYPE value the same with array index */ >> +static struct sram_api srams[] = { >> +#ifdef FSL_85XX_CACHE_SRAM >> + { >> + .type = SRAM_TYPE_FSL_85XX_CACHE_SRAM, >> + .sram_alloc = mpc85xx_cache_sram_alloc, >> + .sram_free = mpc85xx_cache_sram_free, >> + }, >> +#endif >> +}; > >If there is a indeed a requirement for hardware specific functions, >I'd say these should be registered from the hardware specific driver >rather than the generic driver having to know about every single >instance.
Yes, as you mentioned upper, and the interfaces should be registered by hardware drivers. and I'd use a set of generic abstractions of the definitions.
>> +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 = -EINVAL; >> + int size; >> + u32 type; >> + >> + if (!uapi) >> + return ret; >> + >> + switch (cmd) { >> + case SRAM_UAPI_IOCTL_SET_SRAM_TYPE: >> + size = copy_from_user((void *)&type, (const void __user *)arg, >> + sizeof(type)); > >This could be a simpler get_user().
Addressed.
> >> +static const struct file_operations sram_uapi_ops = { >> + .owner = THIS_MODULE, >> + .open = sram_uapi_open, >> + .unlocked_ioctl = sram_uapi_ioctl, >> + .mmap = sram_uapi_mmap, >> + .release = sram_uapi_release, >> +}; > >If you have a .unlocked_ioctl callback, there should also be a >.compat_ioctl one. This can normally point to compat_ptr_ioctl().
Addressed
>> + >> +static struct miscdevice sram_uapi_miscdev = { >> + MISC_DYNAMIC_MINOR, >> + "sram-uapi", >> + &sram_uapi_ops, >> +}; > >The name of the character device should not contain "uapi", that >is kind of implied here.
Addressed
>> + >> +#define SRAM_UAPI_IOCTL_SET_SRAM_TYPE 0 >> +#define SRAM_UAPI_IOCTL_ALLOC 1 >> +#define SRAM_UAPI_IOCTL_FREE 2 >> + >> +struct res_info { >> + u32 offset; >> + u32 size; >> +}; > >This is of course not a proper ioctl interface at all, please see >Documentation/driver-api/ioctl.rst for how to define the numbers >in a uapi header file. > >The offset/size arguments should probably be 64 bit wide.
OK, I will reference the ioctl.rst and make a improvement and I think phys_addr_t would be a better choice.
Thanks, Wenhu
| |