On 11/24/25 4:25 PM, Mukesh Ojha wrote: > On Mon, Nov 24, 2025 at 12:48:31PM +0100, Konrad Dybcio wrote: >> On 11/21/25 12:01 PM, Mukesh Ojha wrote: >>> Qualcomm remote processor may rely on Static and Dynamic resources for >>> it to be functional. Static resources are fixed like for example, >>> memory-mapped addresses required by the subsystem and dynamic >>> resources, such as shared memory in DDR etc., are determined at >>> runtime during the boot process. >>> >>> For most of the Qualcomm SoCs, when run with Gunyah or older QHEE >>> hypervisor, all the resources whether it is static or dynamic, is >>> managed by the hypervisor. Dynamic resources if it is present for a >>> remote processor will always be coming from secure world via SMC call >>> while static resources may be present in remote processor firmware >>> binary or it may be coming qcom_scm_pas_get_rsc_table() SMC call along >>> with dynamic resources. >>> >>> Some of the remote processor drivers, such as video, GPU, IPA, etc., do >>> not check whether resources are present in their remote processor >>> firmware binary. In such cases, the caller of this function should set >>> input_rt and input_rt_size as NULL and zero respectively. Remoteproc >>> framework has method to check whether firmware binary contain resources >>> or not and they should be pass resource table pointer to input_rt and >>> resource table size to input_rt_size and this will be forwarded to >>> TrustZone for authentication. TrustZone will then append the dynamic >>> resources and return the complete resource table in output_rt >>> >>> More about documentation on resource table format can be found in >>> include/linux/remoteproc.h >>> >>> Signed-off-by: Mukesh Ojha <[email protected]> >>> --- >> >> [...] >> >>> +int qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *ctx, void >>> *input_rt, >>> + size_t input_rt_size, void **output_rt, >>> + size_t *output_rt_size) >>> +{ >>> + unsigned int retry_num = 5; >>> + int ret; >>> + >>> + do { >>> + *output_rt = kzalloc(*output_rt_size, GFP_KERNEL); >>> + if (!*output_rt) >>> + return -ENOMEM; >>> + >>> + ret = __qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt, >>> + input_rt_size, output_rt, >>> + output_rt_size); >>> + if (ret) >>> + kfree(*output_rt); >>> + >>> + } while (ret == -EAGAIN && --retry_num); >> >> Will firmware return -EAGAIN as a result, or is this to handle the >> "buffer too small case"? > > The latter one where a re-attempt could pass.. > >> >> I think the latter should use a different errno (EOVERFLOW?) and print >> a message since we decided that it's the caller that suggests a suitable >> output buffer size > > Agree with error code.. > > This is kept on the caller side keeping future in mind. where we can have > resource table coming from the client and it needs to go to TZ for > authentication. > > Are you suggesting to move this retry on the caller side ?
I think we got confused in the review of the previous iterations and made qcom_scm_pas_get_rsc_table() retry 5 times (on the basis that "some" error could happen in firmware), but if it's specifically "buf too small", we should only need to call it utmost twice - once to get the required larger size (or succeed and exit) and another one with a now-correctly sized buffer. Looking at it again, do we really need to be so stringent about the maximum resource table size? Can we just push the currently defined SZ_16K inside qcom_scm_pas_get_rsc_table() as a constant and bump it up as necessary in the future? > Just for information, once video patches becomes ready, we may bring this > qcom_mdt_pas_map_devmem_rscs()[1] helper for video or any other client > should be do this here as well ? > > I wanted to optimize this path, where caller is making a guess and > gets the updated output size in return. We always end up allocating in __qcom_scm_pas_get_rsc_table() so I think guessing a number like SZ_16K which is plenty for a effectively small u64[] in this file is ok too. Perhaps we could even shrink it down a bit.. > [1] > https://lore.kernel.org/lkml/[email protected]/#t > >> >> In case it doesn't make sense for the caller to share their expectations, >> the buffer could be allocated (and perhaps resized if necessary) internally >> with some validation (i.e. a rsctable likely won't be 5 GiB) > > Are you saying output_size as well should be checked and it should not be > greater than something like UINT_MAX or something.. ? > > + *output_rt_size = res.result[2]; Yeah we should probably make sure this doesn't exceed a large-but-not- entirely-unreasonable value Konrad
