* [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
@ 2021-10-13 16:21 Adrian Fiergolski
2021-10-14 7:22 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Fiergolski @ 2021-10-13 16:21 UTC (permalink / raw)
To: u-boot; +Cc: michal.simek, Adrian Fiergolski
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.
Signed-off-by: Adrian Fiergolski <adrian.fiergolski@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};
if (api_id == PM_FPGA_LOAD) {
/* Swap addr_hi/low because of incompatibility */
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
2021-10-13 16:21 [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request Adrian Fiergolski
@ 2021-10-14 7:22 ` Michal Simek
2021-10-14 8:54 ` Adrian Fiergolski
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2021-10-14 7:22 UTC (permalink / raw)
To: Adrian Fiergolski, u-boot; +Cc: michal.simek
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@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.
Also using regs should be fine because ret_payload[0] is return value
anyway.
Thanks,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
2021-10-14 7:22 ` Michal Simek
@ 2021-10-14 8:54 ` Adrian Fiergolski
2021-10-14 9:47 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Fiergolski @ 2021-10-14 8:54 UTC (permalink / raw)
To: Michal Simek, u-boot
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
2021-10-14 8:54 ` Adrian Fiergolski
@ 2021-10-14 9:47 ` Michal Simek
2021-10-14 11:17 ` Adrian Fiergolski
0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2021-10-14 9:47 UTC (permalink / raw)
To: Adrian Fiergolski, Michal Simek, u-boot
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
2021-10-14 9:47 ` Michal Simek
@ 2021-10-14 11:17 ` Adrian Fiergolski
2021-10-14 11:34 ` Michal Simek
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Fiergolski @ 2021-10-14 11:17 UTC (permalink / raw)
To: Michal Simek, u-boot
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request.
2021-10-14 11:17 ` Adrian Fiergolski
@ 2021-10-14 11:34 ` Michal Simek
0 siblings, 0 replies; 6+ messages in thread
From: Michal Simek @ 2021-10-14 11:34 UTC (permalink / raw)
To: Adrian Fiergolski, Michal Simek, u-boot
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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-14 11:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 16:21 [PATCHv2] firmware: zynqmp: add handling of the NULL return payload pointer in xilinx_pm_request Adrian Fiergolski
2021-10-14 7:22 ` Michal Simek
2021-10-14 8:54 ` Adrian Fiergolski
2021-10-14 9:47 ` Michal Simek
2021-10-14 11:17 ` Adrian Fiergolski
2021-10-14 11:34 ` Michal Simek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.