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

[Adrian Fiergolski]

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] ?

Regards,
Adrian