Messages in this thread | | | From | Arnd Bergmann <> | Date | Sat, 18 Apr 2020 21:07:54 +0200 | Subject | Re: [PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access |
| |
On Sat, Apr 18, 2020 at 6:22 PM Wang Wenhu <wenhu.wang@vivo.com> 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 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.
> +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?
> +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.
> +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().
> +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().
> + > +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.
Arnd
| |