On 14.10.2021 11:47, Michal Simek wrote: > > > On 10/14/21 10:54, Adrian Fiergolski wrote: >> 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? > > Ok. I see it was purely for handling > return (ret_payload) ? ret_payload[0] : 0; > > Just simply call return within CONFIG_ZYNQMP_IPI code and that should > be it. > It really doesn't look good that you have > > #if defined(CONFIG_ZYNQMP_IPI) > ... > #endif > .. > if > #if defined(CONFIG_ZYNQMP_IPI) > ... > #else > ... > #endif > else > ... > > >> >> 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. > > That's a little bit different story. If ipi_req fails it should be > error out. But if doesn't fail and user doesn't care about return > value you should return just 0/success. > Definitely adding handling for ipi_req() failure is good to do.
Am I wrong assuming that pmu-firmware in case of success will return 0 for any ipi request in the first byte of ret_payload, otherwise non-zero value? If that's true, shouldn't xilinx_pm_request simply return, unconditionally, ret_payload[0] ? Regards, Adrian
