diff mbox series

[v4,03/18] secvar_main: increase error verbosity, restyle all comments

Message ID 20200511213152.24952-4-erichte@linux.ibm.com
State Changes Requested
Headers show
Series Add initial secure variable storage and backend drivers | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (0f1937ef40fca0c3212a9dff1010b832a24fb063)

Commit Message

Eric Richter May 11, 2020, 9:31 p.m. UTC
This commit converts all the "//" style comments to the appropriate
C-style, adds a few comments, and also adds a couple error log outputs.

The following commits performs a minor refactor of the secvar_main
flow. This commit has been separated out to reduce the amount of noise
in those commits, and can likely be squashed if necessary.

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
V4:
 - adjusted lines to 80 columns

 libstb/secvar/secvar_main.c | 41 ++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Oliver O'Halloran May 12, 2020, 3:58 a.m. UTC | #1
On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote:
> This commit converts all the "//" style comments to the appropriate
> C-style, adds a few comments, and also adds a couple error log outputs.
> 
> The following commits performs a minor refactor of the secvar_main
> flow. This commit has been separated out to reduce the amount of noise
> in those commits, and can likely be squashed if necessary.

Try not to mix unrelated changes. It's an 18 patch series, having 19
won't hurt anyone.

> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
> V4:
>  - adjusted lines to 80 columns
> 
>  libstb/secvar/secvar_main.c | 41 ++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
> index b40dd646..3b842d16 100644
> --- a/libstb/secvar/secvar_main.c
> +++ b/libstb/secvar/secvar_main.c
> @@ -14,10 +14,10 @@
>  struct list_head variable_bank;
>  struct list_head update_bank;
>  
> -int secvar_enabled = 0;	// Set to 1 if secvar is supported
> -int secvar_ready = 0;	// Set to 1 when base secvar inits correctly
> +int secvar_enabled = 0;	/* Set to 1 if secvar is supported */
> +int secvar_ready = 0;	/* Set to 1 when base secvar inits correctly */
>  
> -// To be filled in by platform.secvar_init
> +/* To be filled in by platform.secvar_init */
>  struct secvar_storage_driver secvar_storage = {0};
>  struct secvar_backend_driver secvar_backend = {0};
>  
> @@ -43,7 +43,7 @@ int secvar_main(struct secvar_storage_driver storage_driver,
>  	if (rc)
>  		goto fail;
>  
> -	// Failures here should indicate some kind of hardware problem
> +	/* Failures here should indicate some kind of hardware problem */
>  	rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
>  	if (rc)
>  		goto fail;
> @@ -52,21 +52,35 @@ int secvar_main(struct secvar_storage_driver storage_driver,
>  	if (rc)
>  		goto fail;
>  
> -	// At this point, base secvar is functional. Rest is up to the backend
> +	/* At this point, base secvar is functional.
> +	 * The rest is up to the backend */

skiboot style for multi-line comments is:

/*
 * Like this.
 * 
 * It's the same style that most of the kernel uses.
 */

>  	secvar_ready = 1;
>  	secvar_set_status("okay");
>  
> -	if (secvar_backend.pre_process)
> +	if (secvar_backend.pre_process) {
>  		rc = secvar_backend.pre_process();
> +		if (rc) {
> +			prlog(PR_ERR, "Error in backend pre_process = %d\n", rc);
> +			goto out;
> +		}
> +	}
>  
> -	// Process is required, error if it doesn't exist
> +	/* Process is required, error if it doesn't exist */
>  	if (!secvar_backend.process)
>  		goto out;
>  
> +	/* Process variable updates from the update bank. */
>  	rc = secvar_backend.process();
> -		secvar_set_update_status(rc);
> +
> +	/* Create and set the update-status device tree property */
> +	secvar_set_update_status(rc);
> +
> +	/* Only write to the storage if we actually processed updates
> +	 * OPAL_EMPTY implies no updates were processed
> +	 * Refer to full table in doc/device-tree/ibm,opal/secvar.rst */
>  	if (rc == OPAL_SUCCESS) {
> -		rc = secvar_storage.write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +		rc = secvar_storage.write_bank(&variable_bank,
> +					       SECVAR_VARIABLE_BANK);
>  		if (rc)
>  			goto out;
>  
> @@ -75,10 +89,13 @@ int secvar_main(struct secvar_storage_driver storage_driver,
>  			goto out;
>  	}
>  
> -	if (secvar_backend.post_process)
> +	if (secvar_backend.post_process) {
>  		rc = secvar_backend.post_process();
> -	if (rc)
> -		goto out;
> +		if (rc) {
> +			prlog(PR_ERR, "Error in backend post_process = %d\n", rc);
> +			goto out;
> +		}
> +	}
>  
>  	prlog(PR_INFO, "secvar initialized successfully\n");
>
diff mbox series

Patch

diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
index b40dd646..3b842d16 100644
--- a/libstb/secvar/secvar_main.c
+++ b/libstb/secvar/secvar_main.c
@@ -14,10 +14,10 @@ 
 struct list_head variable_bank;
 struct list_head update_bank;
 
-int secvar_enabled = 0;	// Set to 1 if secvar is supported
-int secvar_ready = 0;	// Set to 1 when base secvar inits correctly
+int secvar_enabled = 0;	/* Set to 1 if secvar is supported */
+int secvar_ready = 0;	/* Set to 1 when base secvar inits correctly */
 
-// To be filled in by platform.secvar_init
+/* To be filled in by platform.secvar_init */
 struct secvar_storage_driver secvar_storage = {0};
 struct secvar_backend_driver secvar_backend = {0};
 
@@ -43,7 +43,7 @@  int secvar_main(struct secvar_storage_driver storage_driver,
 	if (rc)
 		goto fail;
 
-	// Failures here should indicate some kind of hardware problem
+	/* Failures here should indicate some kind of hardware problem */
 	rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
 	if (rc)
 		goto fail;
@@ -52,21 +52,35 @@  int secvar_main(struct secvar_storage_driver storage_driver,
 	if (rc)
 		goto fail;
 
-	// At this point, base secvar is functional. Rest is up to the backend
+	/* At this point, base secvar is functional.
+	 * The rest is up to the backend */
 	secvar_ready = 1;
 	secvar_set_status("okay");
 
-	if (secvar_backend.pre_process)
+	if (secvar_backend.pre_process) {
 		rc = secvar_backend.pre_process();
+		if (rc) {
+			prlog(PR_ERR, "Error in backend pre_process = %d\n", rc);
+			goto out;
+		}
+	}
 
-	// Process is required, error if it doesn't exist
+	/* Process is required, error if it doesn't exist */
 	if (!secvar_backend.process)
 		goto out;
 
+	/* Process variable updates from the update bank. */
 	rc = secvar_backend.process();
-		secvar_set_update_status(rc);
+
+	/* Create and set the update-status device tree property */
+	secvar_set_update_status(rc);
+
+	/* Only write to the storage if we actually processed updates
+	 * OPAL_EMPTY implies no updates were processed
+	 * Refer to full table in doc/device-tree/ibm,opal/secvar.rst */
 	if (rc == OPAL_SUCCESS) {
-		rc = secvar_storage.write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+		rc = secvar_storage.write_bank(&variable_bank,
+					       SECVAR_VARIABLE_BANK);
 		if (rc)
 			goto out;
 
@@ -75,10 +89,13 @@  int secvar_main(struct secvar_storage_driver storage_driver,
 			goto out;
 	}
 
-	if (secvar_backend.post_process)
+	if (secvar_backend.post_process) {
 		rc = secvar_backend.post_process();
-	if (rc)
-		goto out;
+		if (rc) {
+			prlog(PR_ERR, "Error in backend post_process = %d\n", rc);
+			goto out;
+		}
+	}
 
 	prlog(PR_INFO, "secvar initialized successfully\n");