diff mbox series

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

Message ID 20230427103125.26719-2-stefan.herbrechtsmeier-oss@weidmueller.com
State Superseded
Delegated to: Michal Simek
Headers show
Series [v3,1/2] firmware: zynqmp: Remove extraordinary return value | expand

Commit Message

Stefan Herbrechtsmeier April 27, 2023, 10:31 a.m. UTC 
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(-)

Comments

Michal Simek May 16, 2023, 8:26 a.m. UTC | #1 
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.

And tbh zynqmp_pmufw_config_close should also do the same thing.

The rest looks good to me.

Thanks,
Michal
Stefan Herbrechtsmeier May 16, 2023, 11:23 a.m. UTC | #2 
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?

Regards
   Stefan
Michal Simek May 16, 2023, 11:43 a.m. UTC | #3 
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
diff mbox series

Patch

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));
 
 	return 0;
 }
@@ -239,9 +230,23 @@  int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id)
  */
 int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size)
 {
+	static bool checked;
+	static bool skip;
 	int err;
 	u32 ret_payload[PAYLOAD_ARG_CNT];
 
+	if (!checked) {
+		checked = true;
+
+		if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) {
+			printf("PMUFW:  No permission to change config object\n");
+			skip = true;
+		}
+	}
+
+	if (skip)
+		return -EACCES;
+
 	if (IS_ENABLED(CONFIG_SPL_BUILD))
 		printf("Loading new PMUFW cfg obj (%ld bytes)\n", size);
 
@@ -250,8 +255,6 @@  int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size)
 	err = xilinx_pm_request(PM_SET_CONFIGURATION, (u32)(u64)cfg_obj, 0, 0,
 				0, ret_payload);
 	if (err == XST_PM_NO_ACCESS) {
-		if (((u32 *)cfg_obj)[NODE_ID_LOCATION] == NODE_OCM_BANK_0)
-			printf("PMUFW:  No permission to change config object\n");
 		return -EACCES;
 	}
 
@@ -295,9 +298,6 @@  static int zynqmp_power_probe(struct udevice *dev)
 	       ret >> ZYNQMP_PM_VERSION_MAJOR_SHIFT,
 	       ret & ZYNQMP_PM_VERSION_MINOR_MASK);
 
-	if (IS_ENABLED(CONFIG_ARCH_ZYNQMP))
-		zynqmp_pmufw_node(NODE_OCM_BANK_0);
-
 	return 0;
 };