On 14.10.2021 at 09:22, Michal Simek wrote: > > First of all subject is quite long. Please make it shorter and remove > dot at the end. > > > On 10/13/21 18:21, Adrian Fiergolski wrote: >> When a caller is not interested in the returned message, the >> ret_payload pointer is set to NULL in the u-boot-sources. >> In this case, under EL3, the memory from address 0x0 would be >> overwritten by xilinx_pm_request with the returned IPI message, >> damaging the original data under this address. The patch, in case >> ret_payload is NULL, assigns the pointer to the array >> holding the IPI message being sent. > > There is any limit around 80 chars on the line that's why please use it. > >> >> Signed-off-by: Adrian Fiergolski <[email protected]> >> --- >> drivers/firmware/firmware-zynqmp.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/firmware/firmware-zynqmp.c >> b/drivers/firmware/firmware-zynqmp.c >> index d4dc856baf..7517a84f0e 100644 >> --- a/drivers/firmware/firmware-zynqmp.c >> +++ b/drivers/firmware/firmware-zynqmp.c >> @@ -154,16 +154,24 @@ U_BOOT_DRIVER(zynqmp_power) = { >> int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 >> arg1, u32 arg2, >> u32 arg3, u32 *ret_payload) >> { >> +#if defined(CONFIG_ZYNQMP_IPI) >> + /* >> + * Use fixed payload and arg size as the EL2 call. The firmware >> + * is capable to handle PMUFW_PAYLOAD_ARG_CNT bytes but the >> + * firmware API is limited by the SMC call size >> + */ >> + u32 regs[] = {api_id, arg0, arg1, arg2, arg3}; >> + >> + /* >> + * Use regs array in case ret_payload is NULL >> + */ >> + if (ret_payload == NULL) >> + ret_payload = regs; >> +#endif >> debug("%s at EL%d, API ID: 0x%0x\n", __func__, current_el(), >> api_id); >> if (IS_ENABLED(CONFIG_SPL_BUILD) || current_el() == 3) { >> #if defined(CONFIG_ZYNQMP_IPI) >> - /* >> - * Use fixed payload and arg size as the EL2 call. The firmware >> - * is capable to handle PMUFW_PAYLOAD_ARG_CNT bytes but the >> - * firmware API is limited by the SMC call size >> - */ >> - u32 regs[] = {api_id, arg0, arg1, arg2, arg3}; >> > > Based on your description the only code affected by this is here and > there is no reason to move it out of there. > It should be enough just to define ret_payload here as: > > /* Use regs array in case ret_payload is NULL */ > if (!ret_payload) > ret_payload = regs; > > > Or is there any other reason why you have moved code from here? > In else part regs are defined as pt_regs that's why that regs won't be > used anyway. And reg_payload is checked already.
I moved the code, because otherwise, the life scope of regs would be limited to the first branch of the 'if' statement. As a consequence, outside 'if', reg_payload would point to the already invalid location of the regs variable in the return statement. Do you agree? I also realised, that in the return statement, if ret_payload was NULL, a success status (0) was returned. Is it intended? After all, even e.g. a setter call to pmu-firmware (not interested in the returned payload), could fail. The returned value would be misleading then. Regards, Adrian
