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

[Adrian Fiergolski]

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?

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