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.
Thanks,
Michal
