On 09/02/2016 05:49 AM, Jens Wiklander wrote: > On Thu, Sep 01, 2016 at 01:06:04PM -0500, Andrew F. Davis wrote: >> On 09/01/2016 04:22 AM, Jens Wiklander wrote: >>> On Wed, Aug 31, 2016 at 11:40:20AM -0500, Andrew F. Davis wrote: >>>> On 08/31/2016 08:50 AM, Jens Wiklander wrote: >>>>> On Tue, Aug 30, 2016 at 03:23:24PM -0500, Andrew F. Davis wrote: >>>>>> On 08/22/2016 08:00 AM, Jens Wiklander wrote: >>>>>>> +static struct tee_shm_pool * >>>>>>> +optee_config_shm_ioremap(struct device *dev, optee_invoke_fn *invoke_fn, >>>>>>> + void __iomem **ioremaped_shm) >>>>>>> +{ >>>>>>> + struct arm_smccc_res res; >>>>>>> + struct tee_shm_pool *pool; >>>>>>> + unsigned long vaddr; >>>>>>> + phys_addr_t paddr; >>>>>>> + size_t size; >>>>>>> + phys_addr_t begin; >>>>>>> + phys_addr_t end; >>>>>>> + void __iomem *va; >>>>>>> + struct tee_shm_pool_mem_info priv_info; >>>>>>> + struct tee_shm_pool_mem_info dmabuf_info; >>>>>>> + >>>>>>> + invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res); >>>>>>> + if (res.a0 != OPTEE_SMC_RETURN_OK) { >>>>>>> + dev_info(dev, "shm service not available\n"); >>>>>>> + return ERR_PTR(-ENOENT); >>>>>>> + } >>>>>>> + >>>>>>> + if (res.a3 != OPTEE_SMC_SHM_CACHED) { >>>>>>> + dev_err(dev, "only normal cached shared memory supported\n"); >>>>>>> + return ERR_PTR(-EINVAL); >>>>>>> + } >>>>>>> + >>>>>>> + begin = roundup(res.a1, PAGE_SIZE); >>>>>>> + end = rounddown(res.a1 + res.a2, PAGE_SIZE); >>>>>> >>>>>> res.a1/2/3 is really hard to review and understand, would it work better >>>>>> to use a union or cast for the output of invoke_fn based on the function >>>>>> type? >>>>>> >>>>>> In the header that defines what the returned info from these calls means >>>>>> add: >>>>>> >>>>>> struct OPTEE_SMC_GET_SHM_CONFIG_RESULT { >>>>>> unsigned long status; >>>>>> unsigned long start; >>>>>> unsigned long size; >>>>>> unsigned long settings; >>>>>> }; >>>>>> >>>>>> then: >>>>>> >>>>>> union something result; >>>>>> >>>>>> begin = roundup(result.ret.start, PAGE_SIZE); >>>>>> end = rounddown(result.ret.start + result.ret.size, PAGE_SIZE); >>>>>> >>>>>> or similar with just casting to the better named struct type. >>>>> >>>>> optee_smc.h describes what's passed in the registers during an SMC I'd >>>>> rather not clutter it with structs that doesn't add any information >>>>> there. I'm not that happy with casting or using unions to alias struct >>>>> arm_smccc_res either. How about a simple wrapper function for this call >>>>> to deal with the details instead? >>>>> >>>> >>>> I think that would be a good idea anyway, for instance, someday if the >>>> interface changes slightly then you will be able to contain the >>>> compatibility fixes in the wrapper and not out here in the main driver. >>> >>> That interface is supposed to be a stable ABI, incompatible changes in >>> the ABI should be discouraged. If there's an incompatible change it has >>> to be dealt with in the main driver. >> >> Why? This driver is for "OPTEE" not "OPTEE v2.0.01.02", any minor ABI >> changes should be abstracted away as much as possible to keep the main >> driver logically simple, agnostic to any OPTEE version ABI quirks/handling. > > Call me naive, but I don't expect any quirks. The ABI should only be > extended with new functions and old may be deprecated. > If only it was always this simple Linus would have nothing to yell about :), but sometimes ABI quirks happen. I can't really think of a good example right now, and I guess we can always add that level of indirection when/if we ever need it, so I'll drop this concern for now. > Take the function optee_config_shm_ioremap() as an example. That > function will not be used if OP-TEE doesn't use a specific shared memory > pool but instead allocate shared memory via vmalloc() or from user > space. > > This kind of changes/extensions are expected, but that's probably things > the driver need to deal with directly since if change doesn't add > something significant it wouldn't be needed. > >> >>> A small wrapper function in a >>> standalone header file has no chance here as it probably involves using >>> information gathered while probing secure world. >>> >>> What I meant was a small wrapper function just above >>> optee_config_shm_ioremap() to deal with only this call. >>> >> >> This wouldn't do anything that a cast couldn't do. Why not put the >> wrapper function in the header as part of that OPTEE version's ABI >> definition? > > Choosing between wrapper functions or structs in optee_smc.h I'd choose > structs. I'll add structs for the ABI functions where it helps, skipping > for instance the OPTEE_SMC_*UID and OPTEE_SMC_CALL_WITH_ARG functions. > Would that be OK? > That's fine, it wouldn't add anything to name those return values. Thanks, Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html