Change in ...coreboot[master]: drivers/pc80/rtc: Move fetching and cleaning up the layout

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33057

Change subject: drivers/pc80/rtc: Move fetching and cleaning up the layout
......................................................................

drivers/pc80/rtc: Move fetching and cleaning up the layout

Both get_option and set_option need to fetch the layout and clean up
afterwards. Do this in a common location.

Change-Id: I0623738838c62f49e9a8d93a34f01a509ef1f9cb
Signed-off-by: Arthur Heymans 
---
M src/drivers/pc80/rtc/mc146818rtc.c
1 file changed, 39 insertions(+), 38 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33057/1

diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c
index 6e37cd2..3667354 100644
--- a/src/drivers/pc80/rtc/mc146818rtc.c
+++ b/src/drivers/pc80/rtc/mc146818rtc.c
@@ -268,22 +268,12 @@
    return CB_SUCCESS;
 }

-enum cb_err get_option(void *dest, const char *name)
+struct region_device rdev;
+
+static enum cb_err get_layout(struct cmos_option_table *ct)
 {
-	struct cmos_option_table *ct;
-	struct region_device rdev;
-	struct cmos_entries *ce;
-	size_t namelen;
-	int found = 0;
-
-	if (!CONFIG(USE_OPTION_TABLE))
-		return CB_CMOS_OTABLE_DISABLED;
-
    LOCK_NVRAM_CBFS_SPINLOCK();

-	/* Figure out how long name is */
-	namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
-
    if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
    	UNLOCK_NVRAM_CBFS_SPINLOCK();
    	return CB_CMOS_LAYOUT_NOT_FOUND;
@@ -296,6 +286,32 @@
    	UNLOCK_NVRAM_CBFS_SPINLOCK();
    	return CB_CMOS_LAYOUT_NOT_FOUND;
    }
+	return CB_SUCCESS;
+}
+
+static void clean_up_layout(struct cmos_option_table *ct)
+{
+	rdev_munmap(&rdev, ct);
+	UNLOCK_NVRAM_CBFS_SPINLOCK();
+}
+
+enum cb_err get_option(void *dest, const char *name)
+{
+	struct cmos_option_table *ct = NULL;;
+	struct cmos_entries *ce;
+	size_t namelen;
+	int found = 0;
+
+	if (!CONFIG(USE_OPTION_TABLE))
+		return CB_CMOS_OTABLE_DISABLED;
+
+	LOCK_NVRAM_CBFS_SPINLOCK();
+
+	/* Figure out how long name is */
+	namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
+
+	if (get_layout(ct) != CB_SUCCESS)
+		return CB_CMOS_LAYOUT_NOT_FOUND;

/* find the requested entry record */
    ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
@@ -308,23 +324,19 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
+		clean_up_layout(ct);
    	return CB_CMOS_OPTION_NOT_FOUND;
    }

if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC)) {
-		rdev_munmap(&rdev, ct);
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
+		clean_up_layout(ct);
    	return CB_CMOS_CHECKSUM_INVALID;
    }
    if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
+		clean_up_layout(ct);
    	return CB_CMOS_ACCESS_ERROR;
    }
-	rdev_munmap(&rdev, ct);
-	UNLOCK_NVRAM_CBFS_SPINLOCK();
+	clean_up_layout(ct);
    return CB_SUCCESS;
 }

@@ -383,8 +395,7 @@

enum cb_err set_option(const char *name, void *value)
 {
-	struct cmos_option_table *ct;
-	struct region_device rdev;
+	struct cmos_option_table *ct = NULL;
    struct cmos_entries *ce;
    unsigned long length;
    size_t namelen;
@@ -396,18 +407,8 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);

-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
+	if (get_layout(ct) != CB_SUCCESS)
    	return CB_CMOS_LAYOUT_NOT_FOUND;
-	}
-	ct = rdev_mmap_full(&rdev);
-	if (!ct) {
-		printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. "
-		       "Options are disabled\n");
-
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
-		return CB_CMOS_LAYOUT_NOT_FOUND;
-	}

/* find the requested entry record */
    ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
@@ -420,7 +421,7 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
+		clean_up_layout(ct);
    	return CB_CMOS_OPTION_NOT_FOUND;
    }

@@ -430,17 +431,17 @@
    	/* make sure the string is null terminated */
    	if (set_cmos_value(ce->bit + ce->length - 8, 8, &(u8[]){0})
    	    != CB_SUCCESS) {
-			rdev_munmap(&rdev, ct);
+			clean_up_layout(ct);
    		return CB_CMOS_ACCESS_ERROR;
    	}
    }

if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
+		clean_up_layout(ct);
    	return CB_CMOS_ACCESS_ERROR;
    }

-	rdev_munmap(&rdev, ct);
+	clean_up_layout(ct);
    return CB_SUCCESS;
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33057 )

Change subject: drivers/pc80/rtc: Move fetching and cleaning up the layout
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33057/1/src/drivers/pc80/rtc/mc146818rtc.c 
File src/drivers/pc80/rtc/mc146818rtc.c:

https://review.coreboot.org/#/c/33057/1/src/drivers/pc80/rtc/mc146818rtc.c@3... 
PS1, Line 300: 	struct cmos_option_table *ct = NULL;;
Statements terminations use 1 semicolon

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/33057 )

Change subject: drivers/pc80/rtc: Move fetching and cleaning up the layout
......................................................................

drivers/pc80/rtc: Move fetching and cleaning up the layout

Both get_option and set_option need to fetch the layout and clean up
afterwards. Do this in a common location.

Change-Id: I0623738838c62f49e9a8d93a34f01a509ef1f9cb
Signed-off-by: Arthur Heymans 
---
M src/drivers/pc80/rtc/mc146818rtc.c
1 file changed, 39 insertions(+), 38 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33057/2

Change in coreboot[master]: drivers/pc80/rtc: Move fetching and cleaning up the layout

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33057 )

Change subject: drivers/pc80/rtc: Move fetching and cleaning up the layout
......................................................................

Abandoned