Re: [PATCH v3 2/2] firmware: zynqmp: Move config object permission check

[Michal Simek <michal.simek@amd.com>]

On 5/16/23 13:23, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 16.05.2023 um 10:26 schrieb Michal Simek:
> 
>> Hi,
>>
>> first of all sorry for delay.
>>
>> On 4/27/23 12:31, Stefan Herbrechtsmeier wrote:
>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> Move the check of the permission to change a config object from
>>> zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function
>>> to simplify the code and check the permission only if required.
>>>
>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - Added
>>>
>>>   drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++---------------
>>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/firmware/firmware-zynqmp.c 
>>> b/drivers/firmware/firmware-zynqmp.c
>>> index 2b1ad5d2c3..d12159fa78 100644
>>> --- a/drivers/firmware/firmware-zynqmp.c
>>> +++ b/drivers/firmware/firmware-zynqmp.c
>>> @@ -70,20 +70,11 @@ int zynqmp_pmufw_config_close(void)
>>>     int zynqmp_pmufw_node(u32 id)
>>>   {
>>> -    static bool skip_config;
>>> -    int ret;
>>> -
>>> -    if (skip_config)
>>> -        return 0;
>>> -
>>>       /* Record power domain id */
>>>       xpm_configobject[NODE_ID_LOCATION] = id;
>>>   -    ret = zynqmp_pmufw_load_config_object(xpm_configobject,
>>> -                          sizeof(xpm_configobject));
>>> -
>>> -    if (ret == -EACCES && id == NODE_OCM_BANK_0)
>>> -        skip_config = true;
>>> +    zynqmp_pmufw_load_config_object(xpm_configobject,
>>> +                    sizeof(xpm_configobject));
>>
>> This is not right.
>> It should be
>> return zynqmp_pmufw_load... for error propagation.
> 
> At the moment the zynqmp_pmufw_node and zynqmp_pmufw_config_close doesn't return 
> an error. Should the zynqmp_pmufw_load_config_object return 0 or -EACCES if it 
> is skipped?

In context zynqmp_pmufw_node and it's dependency on returning EACESS for failure 
case which your code depends on here

+		if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) {
+			printf("PMUFW:  No permission to change config object\n");
+			skip = true;
+		}


And for second part around skip and return code. I would say what you have is 
fine. It means returning -EACCES is appropriate here.

And as I see do_zynqmp_pmufw should also return that value. That command will 
simply fail if there is no permission.

And for close part I would say the same. Error should be propagated.
I expect current command behavior when you call "pmufw node close" on regular 
system will just pass but it should just fail because command wasn't successful.

Thanks,
Michal