diff mbox series

[U-Boot,4/4] cmd: fpga: Return values of type command_ret_t only

Message ID 20190411101851.13022-5-ada@thorsis.com
State Superseded
Delegated to: Michal Simek
Headers show
Series fpga: collected patches | expand

Commit Message

Alexander Dahl April 11, 2019, 10:18 a.m. UTC 
Passing on a return value of -1 from a called function leads to printing
out usage text. In case of actually correct usage with correctly
specified parameters but some fail at runtime printing out that usage
text is distracting.

The reason is most 'fpga_' functions return either FPGA_SUCCESS or
FPGA_FAIL, the latter is equal to -1 which is the same value as
CMD_RET_USAGE. So just passing on FPGA_FAIL leads to printing out usage.

We should only return CMD_RET_USAGE in cases, where the user sent wrong
input. Every other case should return CMD_RET_SUCCESS or
CMD_RET_FAILURE, and not simply pass an error code.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 cmd/fpga.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Michal Simek April 16, 2019, 10:59 a.m. UTC | #1 
On 11. 04. 19 12:18, Alexander Dahl wrote:
> Passing on a return value of -1 from a called function leads to printing
> out usage text. In case of actually correct usage with correctly
> specified parameters but some fail at runtime printing out that usage
> text is distracting.
> 
> The reason is most 'fpga_' functions return either FPGA_SUCCESS or
> FPGA_FAIL, the latter is equal to -1 which is the same value as
> CMD_RET_USAGE. So just passing on FPGA_FAIL leads to printing out usage.
> 
> We should only return CMD_RET_USAGE in cases, where the user sent wrong
> input. Every other case should return CMD_RET_SUCCESS or
> CMD_RET_FAILURE, and not simply pass an error code.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>

This should be enough to ensure the same. Please test.

M

diff --git a/include/fpga.h b/include/fpga.h
index 51de5c55f830..ec5144334df1 100644
--- a/include/fpga.h
+++ b/include/fpga.h
@@ -15,7 +15,7 @@

 /* fpga_xxxx function return value definitions */
 #define FPGA_SUCCESS           0
-#define FPGA_FAIL              -1
+#define FPGA_FAIL              1

 /* device numbers must be non-negative */
 #define FPGA_INVALID_DEVICE    -1
diff mbox series

Patch

diff --git a/cmd/fpga.c b/cmd/fpga.c
index 88a8e3f318..1bb41217e5 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -13,6 +13,14 @@ 
 #include <fs.h>
 #include <malloc.h>
 
+static int do_fpga_wrap_ret(int ret)
+{
+	if (ret != FPGA_SUCCESS)
+		debug("fpga: function returned %d\n", ret);
+
+	return ret ? CMD_RET_FAILURE : CMD_RET_SUCCESS;
+}
+
 static long do_fpga_get_device(char *arg)
 {
 	long dev = FPGA_INVALID_DEVICE;
@@ -59,7 +67,7 @@  static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size,
 	}
 	*data_size = local_data_size;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
@@ -110,7 +118,8 @@  int do_fpga_loads(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
 	if (ret)
 		return ret;
 
-	return fpga_loads(dev, (void *)fpga_data, data_size, &fpga_sec_info);
+	return do_fpga_wrap_ret(fpga_loads(dev, (void *)fpga_data,
+					   data_size, &fpga_sec_info));
 }
 #endif
 
@@ -134,7 +143,8 @@  static int do_fpga_loadfs(cmd_tbl_t *cmdtp, int flag, int argc,
 	fpga_fsinfo.dev_part = argv[5];
 	fpga_fsinfo.filename = argv[6];
 
-	return fpga_fsload(dev, (void *)fpga_data, data_size, &fpga_fsinfo);
+	return do_fpga_wrap_ret(fpga_fsload(dev, (void *)fpga_data,
+					    data_size, &fpga_fsinfo));
 }
 #endif
 
@@ -143,7 +153,7 @@  static int do_fpga_info(cmd_tbl_t *cmdtp, int flag, int argc,
 {
 	long dev = do_fpga_get_device(argv[0]);
 
-	return fpga_info(dev);
+	return do_fpga_wrap_ret(fpga_info(dev));
 }
 
 static int do_fpga_dump(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -158,7 +168,7 @@  static int do_fpga_dump(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_dump(dev, (void *)fpga_data, data_size);
+	return do_fpga_wrap_ret(fpga_dump(dev, (void *)fpga_data, data_size));
 }
 
 static int do_fpga_load(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -173,7 +183,8 @@  static int do_fpga_load(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_load(dev, (void *)fpga_data, data_size, BIT_FULL);
+	return do_fpga_wrap_ret(fpga_load(dev, (void *)fpga_data,
+					  data_size, BIT_FULL));
 }
 
 static int do_fpga_loadb(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -188,7 +199,8 @@  static int do_fpga_loadb(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_loadbitstream(dev, (void *)fpga_data, data_size, BIT_FULL);
+	return do_fpga_wrap_ret(fpga_loadbitstream(dev, (void *)fpga_data,
+						   data_size, BIT_FULL));
 }
 
 #if defined(CONFIG_CMD_FPGA_LOADP)
@@ -204,7 +216,8 @@  static int do_fpga_loadp(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_load(dev, (void *)fpga_data, data_size, BIT_PARTIAL);
+	return do_fpga_wrap_ret(fpga_load(dev, (void *)fpga_data,
+					  data_size, BIT_PARTIAL));
 }
 #endif
 
@@ -221,8 +234,8 @@  static int do_fpga_loadbp(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_loadbitstream(dev, (void *)fpga_data, data_size,
-				  BIT_PARTIAL);
+	return do_fpga_wrap_ret(fpga_loadbitstream(dev, (void *)fpga_data,
+						   data_size, BIT_PARTIAL));
 }
 #endif
 
@@ -303,14 +316,14 @@  static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc,
 			data_size = image_size;
 #else
 			puts("Gunzip image is not supported\n");
-			return 1;
+			return CMD_RET_FAILURE;
 #endif
 		} else {
 			data = (ulong)image_get_data(hdr);
 			data_size = image_get_data_size(hdr);
 		}
-		return fpga_load(dev, (void *)data, data_size,
-				  BIT_FULL);
+		return do_fpga_wrap_ret(fpga_load(dev, (void *)data,
+						  data_size, BIT_FULL));
 	}
 #endif
 #if defined(CONFIG_FIT)
@@ -350,7 +363,8 @@  static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc,
 			return CMD_RET_FAILURE;
 		}
 
-		return fpga_load(dev, fit_data, data_size, BIT_FULL);
+		return do_fpga_wrap_ret(fpga_load(dev, fit_data,
+						  data_size, BIT_FULL));
 	}
 #endif
 	default: