On 03/23/2015 11:07 AM, Joakim Bech wrote:
> On Fri, Mar 20, 2015 at 01:44:36PM +0100, Jens Wiklander wrote:
>> On Fri, Mar 20, 2015 at 12:00:21PM +0100, Pascal Brand wrote:
>>> Hello Jens,
>>>
>>>
>>> On 19 March 2015 at 14:48, Jérôme Forissier <jerome.forissier@linaro.org>
>>> wrote:
>>>
>>>> Hi Jens,
>>>>
>>>> On 03/19/2015 11:23 AM, Jens Wiklander wrote:
>>>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>>> ---
[...]
>>>>> + * Identifies the generic TEE driver, and the specific TEE driver.
>>>>> + */
>>>>> +struct tee_version {
>>>>> +     uint32_t gen_version;
> What about uint32_t vs __u32 an so on in things shared with user space?
> See Documentation/CodingStyle (and an interesting discussion here:
> http://yarchive.net/comp/linux/kernel_headers.html)

I also vote for __u32 etc. everywhere, as it is also recommended in
http://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt.

[...]

>>>>> +
>>>>> +/**
>>>>> + * struct tee_mem_share_data - share memory with Secure OS
>>>>> + * @buf:     share user space memory
>>>>> + * @dma_buf: share foreign dma_buf memory
>>>>> + * @flags:   Flags to/from sharing, unused bits set to zero by caller
>>>>> + * @pad:     Padding, set to zero by caller
>>>>> + *
>>>>> + * If TEE_MEM_SHARE_FLAG_FOREIGN_BUFFER is set use the dma_buf field,
>>>> else
>>>>> + * the buf field in the union.
>>>>> + */
>>>>> +struct tee_mem_share_data {
>>>>> +     union {
>>>>> +             struct tee_mem_buf buf;
>>>>> +             struct tee_mem_dma_buf dma_buf;
>>>>> +     };
>>>>> +     uint32_t flags;
>>>>> +     uint32_t pad;
>>>>> +};
>>>>
>>> Yet another Shared Memory structure ;)
>>> How could we make more explicit the name "tee_mem_share_data"  (YASM will
>>> not be accepted  ;)   )
>>
>> Perhaps we should tie the different structs close to the IOCTL defines
>> in this file. These structs are only used to pass parameter to the ioctl
>> in questsion. The subsystem will use different types internally.
> I think that is a good idea. Another thing, I've touched it before, do
> we actually need to have the word "mem" everywhere? tee_shm (which
> clashes with name in the other h-file, so that exact name wouldn't work
> here), tee_buf and tee_dma_buf would be sufficient according to me.

+1 for tee_buf and tee_dma_buf but I think I would keep

tee_mem_share_data (it seems the meaning is less obvious without it).

--
Jerome

_______________________________________________
Tee-dev mailing list
Tee-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/tee-dev