On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote: > + > +static int fastrpc_init_process(struct fastrpc_user *fl, > + struct fastrpc_ioctl_init *init) > +{ > + struct fastrpc_ioctl_invoke *ioctl; > + struct fastrpc_phy_page pages[1]; > + struct fastrpc_map *file = NULL, *mem = NULL; > + struct fastrpc_buf *imem = NULL; > + int err = 0; > + > + ioctl = kzalloc(sizeof(*ioctl), GFP_KERNEL); > + if (!ioctl) > + return -ENOMEM; > + > + if (init->flags == FASTRPC_INIT_ATTACH) { > + remote_arg_t ra[1]; > + int tgid = fl->tgid; > + > + ra[0].buf.pv = (void *)&tgid; > + ra[0].buf.len = sizeof(tgid); > + ioctl->handle = 1; > + ioctl->sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); > + ioctl->pra = ra; > + fl->pd = 0; > + > + err = fastrpc_internal_invoke(fl, 1, ioctl); > + if (err) > + goto bail; It doesn't seem right to me to dynamically allocate an 'ioctl' data structure from kernel context and pass that down to another function. Maybe eliminate that structure and change fastrpc_internal_invoke to take the individual arguments here instead of a pointer? > + } else if (init->flags == FASTRPC_INIT_CREATE) { How about splitting each flags== case into a separate function? > diff --git a/include/uapi/linux/fastrpc.h b/include/uapi/linux/fastrpc.h > index 8fec66601337..6b596fc7ddf3 100644 > --- a/include/uapi/linux/fastrpc.h > +++ b/include/uapi/linux/fastrpc.h > @@ -6,6 +6,12 @@ > #include <linux/types.h> > > #define FASTRPC_IOCTL_INVOKE _IOWR('R', 3, struct fastrpc_ioctl_invoke) > +#define FASTRPC_IOCTL_INIT _IOWR('R', 4, struct fastrpc_ioctl_init) > + > +/* INIT a new process or attach to guestos */ > +#define FASTRPC_INIT_ATTACH 0 > +#define FASTRPC_INIT_CREATE 1 > +#define FASTRPC_INIT_CREATE_STATIC 2 Maybe use three command codes here, and remove the 'flags' member? > @@ -53,4 +59,16 @@ struct fastrpc_ioctl_invoke { > unsigned int *crc; > }; > > +struct fastrpc_ioctl_init { > + uint32_t flags; /* one of FASTRPC_INIT_* macros */ > + uintptr_t file; /* pointer to elf file */ > + uint32_t filelen; /* elf file length */ > + int32_t filefd; /* ION fd for the file */ What does this have to do with ION? The driver seems to otherwise just use the generic dma_buf interfaces. > + uintptr_t mem; /* mem for the PD */ > + uint32_t memlen; /* mem length */ > + int32_t memfd; /* fd for the mem */ > + int attrs; > + unsigned int siglen; > +}; This structure is again not suitable for ioctls. Please used fixed-length members and no holes in the structure. Arnd