lkml.org 
[lkml]   [2018]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 4/6] char: fastrpc: Add support for create remote init process
From
Date
Thanks Arnd for the review comments,

On 30/11/18 13:26, Arnd Bergmann wrote:
> On Fri, Nov 30, 2018 at 11:48 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> 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?
Yes, I totally agree with you, Will rework this part as suggested.
>
>> + } else if (init->flags == FASTRPC_INIT_CREATE) {
>
> How about splitting each flags== case into a separate function?

Once I move this to a command code then make this 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?
>
Make sense, will do it in next version.

>> @@ -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.
Yes, the driver just uses dma_buf, it looks like leftover from downstream!

>
>> + 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.
Sure, Will recheck all the structures before sending next version!


--srini
>
> Arnd
>

\
 
 \ /
  Last update: 2018-11-30 14:35    [W:0.043 / U:4.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site