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. 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? Thanks, Jens -- 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