[PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.

[Michal Simek]

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 <adrian.fiergolski at fastree3d.com>
>>> ---
>>>    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.

Thanks,
Michal