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

[Michal Simek]

On 10/14/21 13:17, Adrian Fiergolski wrote:
> 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 <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.
> 
> 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] ?

It is also plm for versal case. And assumption is right ret_payload[0] 
is return value from firmware if command passes/fails.

It means if ret_payload is NULL user doesn't care about returning values 
via ret_payload[1-up] but for sure it cares that command passes.

That means you can return in EL2 case regs.regs[0] as return value.

Thanks,
Michal