Thread (21 messages) 21 messages, 3 authors, 2019-10-07

Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs

From: Leonard Crestez <hidden>
Date: 2019-09-25 13:13:10
Also in: lkml

On 25.09.2019 13:09, Anson Huang wrote:
quoted hunk ↗ jump to hunk
The SCU firmware does NOT always have return value stored in message
header's function element even the API has response data, those special
APIs are defined as void function in SCU firmware, so they should be
treated as return success always.

Signed-off-by: Anson Huang <redacted>
---
	- This patch is based on the patch of https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11129553%2F&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7Cc0ced6cd07f04023977008d741a07367%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637050029712216472&amp;sdata=Ccq%2Fb2RJdMqmnL7VXrl8YhOlUwC7bWiUG%2BNmiw4OsSM%3D&amp;reserved=0
---
  drivers/firmware/imx/imx-scu.c | 34 ++++++++++++++++++++++++++++++++--
  1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 869be7a..ced5b12 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -78,6 +78,11 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] = {
  	-EIO,	 /* IMX_SC_ERR_FAIL */
  };
  
+static const struct imx_sc_rpc_msg whitelist[] = {
+	{ .svc = IMX_SC_RPC_SVC_MISC, .func = IMX_SC_MISC_FUNC_UNIQUE_ID },
+	{ .svc = IMX_SC_RPC_SVC_MISC, .func = IMX_SC_MISC_FUNC_GET_BUTTON_STATUS },
+};
Until now this low level IPC code didn't treat any svc/func specially 
and this seems good.

The imx_scu_call_rpc function already has an have_resp argument and 
callers are responsible to fill it. Can't we deal with this by adding an 
additional err_ret flag passed by the caller?

We can add wrapper functions to avoid tree-wide changes for all callers.
quoted hunk ↗ jump to hunk
+
  static struct imx_sc_ipc *imx_sc_ipc_handle;
  
  static inline int imx_sc_to_linux_errno(int errno)
@@ -157,11 +162,24 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg)
  	return 0;
  }
  
+static bool imx_scu_call_skip_return_value_check(uint8_t svc, uint8_t func)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(whitelist); i++)
+		if (svc == whitelist[i].svc &&
+			func == whitelist[i].func)
+			return true;
+
+	return false;
+}
+
  /*
   * RPC command/response
   */
  int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
  {
+	uint8_t saved_svc, saved_func;
  	struct imx_sc_rpc_msg *hdr;
  	int ret;
  
@@ -171,8 +189,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
  	mutex_lock(&sc_ipc->lock);
  	reinit_completion(&sc_ipc->done);
  
-	if (have_resp)
+	if (have_resp) {
  		sc_ipc->msg = msg;
+		saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
+		saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
+	}
  	sc_ipc->count = 0;
  	ret = imx_scu_ipc_write(sc_ipc, msg);
  	if (ret < 0) {
@@ -190,7 +211,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
  
  		/* response status is stored in hdr->func field */
  		hdr = msg;
-		ret = hdr->func;
+		/*
+		 * Some special SCU firmware APIs do NOT have return value
+		 * in hdr->func, but they do have response data, those special
+		 * APIs are defined as void function in SCU firmware, so they
+		 * should be treated as return success always.
+		 */
+		if (!imx_scu_call_skip_return_value_check(saved_svc, saved_func))
+			ret = hdr->func;
+		else
+			ret = 0;
  	}
  
  out:

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help