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

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

[Anson Huang]

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 <Anson.Huang@nxp.com>
---
	- This patch is based on the patch of https://patchwork.kernel.org/patch/11129553/
---
 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 },
+};
+
 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:
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

On 25.09.2019 13:09, Anson Huang wrote:
> 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 <Anson.Huang@nxp.com>
> ---
> 	- 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.

> +
>   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

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

[Anson Huang]

Hi, Leonard

> On 25.09.2019 13:09, Anson Huang wrote:
> > 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 <Anson.Huang@nxp.com>
> > ---
> > 	- This patch is based on the patch of
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fpatch%2F11129553%2F&amp;data=02%7C01%7Cleonar
> d.cres
> >
> tez%40nxp.com%7Cc0ced6cd07f04023977008d741a07367%7C686ea1d3bc2b
> 4c6fa92
> >
> cd99c5c301635%7C0%7C0%7C637050029712216472&amp;sdata=Ccq%2Fb2R
> JdMqmnL7
> > VXrl8YhOlUwC7bWiUG%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?

Can you make it more detail? The have_resp is a bool, so where to add the flag?
The caller ONLY passes imx_sc_ipc, imx_sc_rpc_msg and have_resp, ONLY
imx_sc_ipc can add a flag, is it what you meant? 

imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)

> 
> We can add wrapper functions to avoid tree-wide changes for all callers.

I agree, maybe we can add a new imx_scu_call_rpc function for those special APIs?
The new API will be ONLY for those APIs with response but without return value check,
then other callers will NOT be impacted, what do you think?

Thanks,
Anson

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi,

On 19-09-25 18:07, Anson Huang wrote:
> 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 <Anson.Huang@nxp.com>
> ---
> 	- This patch is based on the patch of https://patchwork.kernel.org/patch/11129553/
> ---
>  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 },
> +};

Is this going to be extended in the near future? I see some upcoming
problems here if someone uses a different scu-fw<->kernel combination as
nxp would suggest.

Regards,
  Marco

> +
>  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:
> -- 
> 2.7.4
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Marco

> On 19-09-25 18:07, Anson Huang wrote:
> > 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 <Anson.Huang@nxp.com>
> > ---
> > 	- This patch is based on the patch of
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fpatch%2F11129553%2F&amp;data=02%7C01%7Canson.
> huang%
> >
> 40nxp.com%7C1f4108cc25eb4618f43c08d742576fa3%7C686ea1d3bc2b4c6fa
> 92cd99
> >
> c5c301635%7C0%7C0%7C637050815608963707&amp;sdata=BZBg4cOR2rP%2
> BRBNn15i
> > Qq3%2FXBYwhuCLkgYzFRbfEgVU%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 }, };
> 
> Is this going to be extended in the near future? I see some upcoming
> problems here if someone uses a different scu-fw<->kernel combination as
> nxp would suggest.

Could be, but I checked the current APIs, ONLY these 2 will be used in Linux kernel, so
I ONLY add these 2 APIs for now.

However, after rethink, maybe we should add another imx_sc_rpc API for those special
APIs? To avoid checking it for all the APIs called which may impact some performance.
Still under discussion, if you have better idea, please advise, thanks!

Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Anson,

On 19-09-26 08:03, Anson Huang wrote:
> Hi, Marco
> 
> > On 19-09-25 18:07, Anson Huang wrote:
> > > 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 <Anson.Huang@nxp.com>
> > > ---
> > > 	- This patch is based on the patch of
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > >
> > hwork.kernel.org%2Fpatch%2F11129553%2F&amp;data=02%7C01%7Canson.
> > huang%
> > >
> > 40nxp.com%7C1f4108cc25eb4618f43c08d742576fa3%7C686ea1d3bc2b4c6fa
> > 92cd99
> > >
> > c5c301635%7C0%7C0%7C637050815608963707&amp;sdata=BZBg4cOR2rP%2
> > BRBNn15i
> > > Qq3%2FXBYwhuCLkgYzFRbfEgVU%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 }, };
> > 
> > Is this going to be extended in the near future? I see some upcoming
> > problems here if someone uses a different scu-fw<->kernel combination as
> > nxp would suggest.
> 
> Could be, but I checked the current APIs, ONLY these 2 will be used in Linux kernel, so
> I ONLY add these 2 APIs for now.

Okay.

> However, after rethink, maybe we should add another imx_sc_rpc API for those special
> APIs? To avoid checking it for all the APIs called which may impact some performance.
> Still under discussion, if you have better idea, please advise, thanks!

Adding a special api shouldn't be the right fix. Imagine if someone (not
a nxp-developer) wants to add a new driver. How could he be expected to
know which api he should use. The better abbroach would be to fix the
scu-fw instead of adding quirks..

Regards,
  Marco


> 
> Anson

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

On 2019-09-26 1:06 PM, Marco Felsch wrote:
> On 19-09-26 08:03, Anson Huang wrote:
>>> On 19-09-25 18:07, Anson Huang wrote:
>>>> 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.
>>>>
>>>> +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 }, };
>>>
>>> Is this going to be extended in the near future? I see some upcoming
>>> problems here if someone uses a different scu-fw<->kernel combination as
>>> nxp would suggest.
>>
>> Could be, but I checked the current APIs, ONLY these 2 will be used in Linux kernel, so
>> I ONLY add these 2 APIs for now.
> 
> Okay.
> 
>> However, after rethink, maybe we should add another imx_sc_rpc API for those special
>> APIs? To avoid checking it for all the APIs called which may impact some performance.
>> Still under discussion, if you have better idea, please advise, thanks!

My suggestion is to refactor the code and add a new API for the this "no 
error value" convention. Internally they can call a common function with 
flags.

> Adding a special api shouldn't be the right fix. Imagine if someone (not
> a nxp-developer) wants to add a new driver. How could he be expected to
> know which api he should use. The better abbroach would be to fix the
> scu-fw instead of adding quirks..

Right now developers who want to make SCFW calls in upstream need to 
define the message struct in their driver based on protocol 
documentation. This includes:

* Binary layout of the message (a packed struct)
* If the message has a response (already a bool flag)
* If an error code is returned (this patch adds support for it)

Since callers are already exposed to the binary protocol exposing them 
to minor quirks of the calling convention also seems reasonable. Having 
the low-level IPC code peek at message IDs seems like a hack; this 
belong at a slightly higher level.

In older internal trees we use a very large amount of generated wrapper 
functions. This hides calling convention details from callers but is 
extremely ugly and verbose.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Leonard

> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > On 19-09-26 08:03, Anson Huang wrote:
> >>> On 19-09-25 18:07, Anson Huang wrote:
> >>>> 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.
> >>>>
> >>>> +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 }, };
> >>>
> >>> Is this going to be extended in the near future? I see some upcoming
> >>> problems here if someone uses a different scu-fw<->kernel
> >>> combination as nxp would suggest.
> >>
> >> Could be, but I checked the current APIs, ONLY these 2 will be used
> >> in Linux kernel, so I ONLY add these 2 APIs for now.
> >
> > Okay.
> >
> >> However, after rethink, maybe we should add another imx_sc_rpc API
> >> for those special APIs? To avoid checking it for all the APIs called which
> may impact some performance.
> >> Still under discussion, if you have better idea, please advise, thanks!
> 
> My suggestion is to refactor the code and add a new API for the this "no
> error value" convention. Internally they can call a common function with
> flags.

If I understand your point correctly, that means the loop check of whether the API
is with "no error value" for every API still NOT be skipped, it is just refactoring the code,
right?

> 
> > Adding a special api shouldn't be the right fix. Imagine if someone
> > (not a nxp-developer) wants to add a new driver. How could he be
> > expected to know which api he should use. The better abbroach would be
> > to fix the scu-fw instead of adding quirks..

Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
FW released has been finalized, so the API implementation can NOT be changed, but
they will pay attention to this issue for new added APIs later. That means the number
of APIs having this issue a very limited. 

> 
> Right now developers who want to make SCFW calls in upstream need to
> define the message struct in their driver based on protocol documentation.
> This includes:
> 
> * Binary layout of the message (a packed struct)
> * If the message has a response (already a bool flag)
> * If an error code is returned (this patch adds support for it)
> 
> Since callers are already exposed to the binary protocol exposing them to
> minor quirks of the calling convention also seems reasonable. Having the
> low-level IPC code peek at message IDs seems like a hack; this belong at a
> slightly higher level.

A little confused, so what you suggested is to add make the imx_scu_call_rpc()
becomes the "slightly higher level" API, then in this API, check the message IDs
to decide whether to return error value, then calls a new API which will have
the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
function, am I right?

Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Anson, Leonard,

On 19-09-27 01:20, Anson Huang wrote:
> Hi, Leonard
> 
> > On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > On 19-09-26 08:03, Anson Huang wrote:
> > >>> On 19-09-25 18:07, Anson Huang wrote:
> > >>>> 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.
> > >>>>
> > >>>> +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 }, };
> > >>>
> > >>> Is this going to be extended in the near future? I see some upcoming
> > >>> problems here if someone uses a different scu-fw<->kernel
> > >>> combination as nxp would suggest.
> > >>
> > >> Could be, but I checked the current APIs, ONLY these 2 will be used
> > >> in Linux kernel, so I ONLY add these 2 APIs for now.
> > >
> > > Okay.
> > >
> > >> However, after rethink, maybe we should add another imx_sc_rpc API
> > >> for those special APIs? To avoid checking it for all the APIs called which
> > may impact some performance.
> > >> Still under discussion, if you have better idea, please advise, thanks!
> > 
> > My suggestion is to refactor the code and add a new API for the this "no
> > error value" convention. Internally they can call a common function with
> > flags.
> 
> If I understand your point correctly, that means the loop check of whether the API
> is with "no error value" for every API still NOT be skipped, it is just refactoring the code,
> right?

How makes this things easier?

> > > Adding a special api shouldn't be the right fix. Imagine if someone
> > > (not a nxp-developer) wants to add a new driver. How could he be
> > > expected to know which api he should use. The better abbroach would be
> > > to fix the scu-fw instead of adding quirks..
> 
> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
> FW released has been finalized, so the API implementation can NOT be changed, but
> they will pay attention to this issue for new added APIs later. That means the number
> of APIs having this issue a very limited.

This means those APIs which already have this bug will not be fixed?
IMHO this sounds a bit weird since this is a changeable peace of code ;)

> > Right now developers who want to make SCFW calls in upstream need to
> > define the message struct in their driver based on protocol documentation.
> > This includes:
> > 
> > * Binary layout of the message (a packed struct)
> > * If the message has a response (already a bool flag)
> > * If an error code is returned (this patch adds support for it)

Why should I specify if a error code is returned?

Regards,
  Marco

> > Since callers are already exposed to the binary protocol exposing them to
> > minor quirks of the calling convention also seems reasonable. Having the
> > low-level IPC code peek at message IDs seems like a hack; this belong at a
> > slightly higher level.
> 
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
> function, am I right?
> 
> Anson

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Marco

> On 19-09-27 01:20, Anson Huang wrote:
> > Hi, Leonard
> >
> > > On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > > On 19-09-26 08:03, Anson Huang wrote:
> > > >>> On 19-09-25 18:07, Anson Huang wrote:
> > > >>>> 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.
> > > >>>>
> > > >>>> +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 }, };
> > > >>>
> > > >>> Is this going to be extended in the near future? I see some
> > > >>> upcoming problems here if someone uses a different
> > > >>> scu-fw<->kernel combination as nxp would suggest.
> > > >>
> > > >> Could be, but I checked the current APIs, ONLY these 2 will be
> > > >> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > >
> > > > Okay.
> > > >
> > > >> However, after rethink, maybe we should add another imx_sc_rpc
> > > >> API for those special APIs? To avoid checking it for all the APIs
> > > >> called which
> > > may impact some performance.
> > > >> Still under discussion, if you have better idea, please advise, thanks!
> > >
> > > My suggestion is to refactor the code and add a new API for the this
> > > "no error value" convention. Internally they can call a common
> > > function with flags.
> >
> > If I understand your point correctly, that means the loop check of
> > whether the API is with "no error value" for every API still NOT be
> > skipped, it is just refactoring the code, right?
> 
> How makes this things easier?

I think it is just for making a better SW layer.

> 
> > > > Adding a special api shouldn't be the right fix. Imagine if
> > > > someone (not a nxp-developer) wants to add a new driver. How could
> > > > he be expected to know which api he should use. The better
> > > > abbroach would be to fix the scu-fw instead of adding quirks..
> >
> > Yes, fixing SCU FW is the best solution, but we have talked to SCU FW
> > owner, the SCU FW released has been finalized, so the API
> > implementation can NOT be changed, but they will pay attention to this
> > issue for new added APIs later. That means the number of APIs having this
> issue a very limited.
> 
> This means those APIs which already have this bug will not be fixed?
> IMHO this sounds a bit weird since this is a changeable peace of code ;)

We can't say it is a bug, the SCU FW owner design it in this way, there are
some void function in SCU FW API, and once there is response data from SCU,
that means the API call is successful, and void function does NOT have a return
value for caller.
So it is just current SCU IPC driver in kernel NOT 100% matching SCU FW, those
void function has such return value issue.

Anson.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

On 27.09.2019 04:20, Anson Huang wrote:
>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>> On 19-09-26 08:03, Anson Huang wrote:
>>>>> On 19-09-25 18:07, Anson Huang wrote:
>>>>>> 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.
>>>>>>
>>>>>> +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 }, };
>>>>>
>>>>> Is this going to be extended in the near future? I see some upcoming
>>>>> problems here if someone uses a different scu-fw<->kernel
>>>>> combination as nxp would suggest.
>>>>
>>>> Could be, but I checked the current APIs, ONLY these 2 will be used
>>>> in Linux kernel, so I ONLY add these 2 APIs for now.
>>>
>>> Okay.
>>>
>>>> However, after rethink, maybe we should add another imx_sc_rpc API
>>>> for those special APIs? To avoid checking it for all the APIs called which
>> may impact some performance.
>>>> Still under discussion, if you have better idea, please advise, thanks!
>>
>> My suggestion is to refactor the code and add a new API for the this "no
>> error value" convention. Internally they can call a common function with
>> flags.
> 
> If I understand your point correctly, that means the loop check of whether the API
> is with "no error value" for every API still NOT be skipped, it is just refactoring the code,
> right?

>> Right now developers who want to make SCFW calls in upstream need to
>> define the message struct in their driver based on protocol documentation.
>> This includes:
>>
>> * Binary layout of the message (a packed struct)
>> * If the message has a response (already a bool flag)
>> * If an error code is returned (this patch adds support for it)
>>
>> Since callers are already exposed to the binary protocol exposing them to
>> minor quirks of the calling convention also seems reasonable. Having the
>> low-level IPC code peek at message IDs seems like a hack; this belong at a
>> slightly higher level.
> 
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
> function, am I right?

No, I mean there should be no loop enumerating svc/func ids: *the caller 
should know* that it's calling a func which doesn't return an error code 
and call a different variant of imx_scu_call_rpc

Maybe add an internal __imx_scu_call_rpc_flags and turn the current 
imx_scu_call_rpc into a wrapper.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

On 27.09.2019 12:06, Marco Felsch wrote:
> Hi Anson, Leonard,
> 
> On 19-09-27 01:20, Anson Huang wrote:
>> Hi, Leonard
>>
>>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>>> On 19-09-26 08:03, Anson Huang wrote:
>>>>>> On 19-09-25 18:07, Anson Huang wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> +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 }, };
>>>>>>
>>>>>> Is this going to be extended in the near future? I see some upcoming
>>>>>> problems here if someone uses a different scu-fw<->kernel
>>>>>> combination as nxp would suggest.
>>>>>
>>>>> Could be, but I checked the current APIs, ONLY these 2 will be used
>>>>> in Linux kernel, so I ONLY add these 2 APIs for now.
>>>>
>>>> Okay.
>>>>
>>>>> However, after rethink, maybe we should add another imx_sc_rpc API
>>>>> for those special APIs? To avoid checking it for all the APIs called which
>>> may impact some performance.
>>>>> Still under discussion, if you have better idea, please advise, thanks!
>>>
>>> My suggestion is to refactor the code and add a new API for the this "no
>>> error value" convention. Internally they can call a common function with
>>> flags.
>>
>>>> Adding a special api shouldn't be the right fix. Imagine if someone
>>>> (not a nxp-developer) wants to add a new driver. How could he be
>>>> expected to know which api he should use. The better abbroach would be
>>>> to fix the scu-fw instead of adding quirks..
>>
>> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
>> FW released has been finalized, so the API implementation can NOT be changed, but
>> they will pay attention to this issue for new added APIs later. That means the number
>> of APIs having this issue a very limited.
> 
> This means those APIs which already have this bug will not be fixed?
> IMHO this sounds a bit weird since this is a changeable peace of code ;)

It's not a bug, it's a documented feature ;)

>>> Right now developers who want to make SCFW calls in upstream need to
>>> define the message struct in their driver based on protocol documentation.
>>> This includes:
>>>
>>> * Binary layout of the message (a packed struct)
>>> * If the message has a response (already a bool flag)
>>> * If an error code is returned (this patch adds support for it)
> 
> Why should I specify if a error code is returned?

Because you're already defining the message struct and you're already 
specifying if a response is required.

The assumption is that anyone adding a SCFW call to a driver is already 
looking at SCFW documentation which describes the binary message format.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Leonard/Marco
	I think we should get aligned first, my original thought is to let SCU API caller NOT aware of those special APIs, so have to do the special handling in imx_scu_call_rpc(). And the short loop check has to be used which would impact the performance a little bit I think. But Leonard stated the caller should know the SCU FW API's usage, if so, then I think the special callers can just skip the return value check, adding a comment to describe the reason, would it be much more easier than changing the imx_scu_call_rpc()? Or any other suggestion?

Anson

> On 27.09.2019 12:06, Marco Felsch wrote:
> > Hi Anson, Leonard,
> >
> > On 19-09-27 01:20, Anson Huang wrote:
> >> Hi, Leonard
> >>
> >>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> >>>> On 19-09-26 08:03, Anson Huang wrote:
> >>>>>> On 19-09-25 18:07, Anson Huang wrote:
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> +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 }, };
> >>>>>>
> >>>>>> Is this going to be extended in the near future? I see some
> >>>>>> upcoming problems here if someone uses a different
> >>>>>> scu-fw<->kernel combination as nxp would suggest.
> >>>>>
> >>>>> Could be, but I checked the current APIs, ONLY these 2 will be
> >>>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> >>>>
> >>>> Okay.
> >>>>
> >>>>> However, after rethink, maybe we should add another imx_sc_rpc API
> >>>>> for those special APIs? To avoid checking it for all the APIs
> >>>>> called which
> >>> may impact some performance.
> >>>>> Still under discussion, if you have better idea, please advise, thanks!
> >>>
> >>> My suggestion is to refactor the code and add a new API for the this
> >>> "no error value" convention. Internally they can call a common
> >>> function with flags.
> >>
> >>>> Adding a special api shouldn't be the right fix. Imagine if someone
> >>>> (not a nxp-developer) wants to add a new driver. How could he be
> >>>> expected to know which api he should use. The better abbroach would
> >>>> be to fix the scu-fw instead of adding quirks..
> >>
> >> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW
> >> owner, the SCU FW released has been finalized, so the API
> >> implementation can NOT be changed, but they will pay attention to
> >> this issue for new added APIs later. That means the number of APIs having
> this issue a very limited.
> >
> > This means those APIs which already have this bug will not be fixed?
> > IMHO this sounds a bit weird since this is a changeable peace of code
> > ;)
> 
> It's not a bug, it's a documented feature ;)
> 
> >>> Right now developers who want to make SCFW calls in upstream need to
> >>> define the message struct in their driver based on protocol
> documentation.
> >>> This includes:
> >>>
> >>> * Binary layout of the message (a packed struct)
> >>> * If the message has a response (already a bool flag)
> >>> * If an error code is returned (this patch adds support for it)
> >
> > Why should I specify if a error code is returned?
> 
> Because you're already defining the message struct and you're already
> specifying if a response is required.
> 
> The assumption is that anyone adding a SCFW call to a driver is already
> looking at SCFW documentation which describes the binary message format.
> 
> --
> Regards,
> Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leonard Crestez]

On 2019-09-27 4:20 AM, Anson Huang wrote:
>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>> On 19-09-26 08:03, Anson Huang wrote:
>>>>> On 19-09-25 18:07, Anson Huang wrote:
>>>>>> 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.
>>>>>>
>>>>>> +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 }, };
>>>>>
>>>>> Is this going to be extended in the near future? I see some upcoming
>>>>> problems here if someone uses a different scu-fw<->kernel
>>>>> combination as nxp would suggest.
>>>>
>>>> Could be, but I checked the current APIs, ONLY these 2 will be used
>>>> in Linux kernel, so I ONLY add these 2 APIs for now.
>>>
>>> Okay.
>>>
>>>> However, after rethink, maybe we should add another imx_sc_rpc API
>>>> for those special APIs? To avoid checking it for all the APIs called which
>> may impact some performance.
>>>> Still under discussion, if you have better idea, please advise, thanks!
>>
>> My suggestion is to refactor the code and add a new API for the this "no
>> error value" convention. Internally they can call a common function with
>> flags.
> 
> If I understand your point correctly, that means the loop check of whether the API
> is with "no error value" for every API still NOT be skipped, it is just refactoring the code,
> right?

There would be no "loop" anywhere: the responsibility would fall on the 
call to call the right RPC function. In the current layering scheme 
(drivers -> RPC -> mailbox) the RPC layer treats all calls the same and 
it's up the the caller to provide information about calling convention.

An example implementation:
* Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
* Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp to 
a flag
* Make get button status call __imx_sc_rpc_call_flags with the 
_IMX_SC_RPC_NOERROR flag

Hope this makes my suggestion clearer? Pushing this to the caller is a 
bit ugly but I think it's worth preserving the fact that the imx rpc 
core treats services in an uniform way.

>>> Adding a special api shouldn't be the right fix. Imagine if someone
>>> (not a nxp-developer) wants to add a new driver. How could he be
>>> expected to know which api he should use. The better abbroach would be
>>> to fix the scu-fw instead of adding quirks..
> 
> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
> FW released has been finalized, so the API implementation can NOT be changed, but
> they will pay attention to this issue for new added APIs later. That means the number
> of APIs having this issue a very limited.
> 
>>
>> Right now developers who want to make SCFW calls in upstream need to
>> define the message struct in their driver based on protocol documentation.
>> This includes:
>>
>> * Binary layout of the message (a packed struct)
>> * If the message has a response (already a bool flag)
>> * If an error code is returned (this patch adds support for it)
>>
>> Since callers are already exposed to the binary protocol exposing them to
>> minor quirks of the calling convention also seems reasonable. Having the
>> low-level IPC code peek at message IDs seems like a hack; this belong at a
>> slightly higher level.
> 
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
> function, am I right?

No, I am saying that the caller (button driver) should be responsible 
for calling with a special flag which states that the RPC call.

In internal tree this is handled inside the machine-generated function 
calls, right? These are mostly skipped in upstream but maybe for these 
particular calls we can manually add wrappers inside 
"drivers/firmware/imx/misc.c".

And even if the functions "return void" from a SCFW perspective it still 
makes sense to return general kernel errors, for example from mailbox.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Leonard

> On 2019-09-27 4:20 AM, Anson Huang wrote:
> >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> >>> On 19-09-26 08:03, Anson Huang wrote:
> >>>>> On 19-09-25 18:07, Anson Huang wrote:
> >>>>>> 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.
> >>>>>>
> >>>>>> +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 }, };
> >>>>>
> >>>>> Is this going to be extended in the near future? I see some
> >>>>> upcoming problems here if someone uses a different scu-fw<->kernel
> >>>>> combination as nxp would suggest.
> >>>>
> >>>> Could be, but I checked the current APIs, ONLY these 2 will be used
> >>>> in Linux kernel, so I ONLY add these 2 APIs for now.
> >>>
> >>> Okay.
> >>>
> >>>> However, after rethink, maybe we should add another imx_sc_rpc API
> >>>> for those special APIs? To avoid checking it for all the APIs
> >>>> called which
> >> may impact some performance.
> >>>> Still under discussion, if you have better idea, please advise, thanks!
> >>
> >> My suggestion is to refactor the code and add a new API for the this
> >> "no error value" convention. Internally they can call a common
> >> function with flags.
> >
> > If I understand your point correctly, that means the loop check of
> > whether the API is with "no error value" for every API still NOT be
> > skipped, it is just refactoring the code, right?
> 
> There would be no "loop" anywhere: the responsibility would fall on the call
> to call the right RPC function. In the current layering scheme (drivers -> RPC ->
> mailbox) the RPC layer treats all calls the same and it's up the the caller to
> provide information about calling convention.
> 
> An example implementation:
> * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> * Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp to a
> flag
> * Make get button status call __imx_sc_rpc_call_flags with the
> _IMX_SC_RPC_NOERROR flag
> 
> Hope this makes my suggestion clearer? Pushing this to the caller is a bit ugly
> but I think it's worth preserving the fact that the imx rpc core treats services
> in an uniform way.

It is clear now, so essentially it is same as 2 separate APIs, still need to change the
button driver and uid driver to use the special flag, meanwhile, need to change the
third parament of imx_sc_rpc_call() from bool to u32.

If no one opposes this approach, I will redo the patch together with the button driver
and uid driver after holiday.

Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Leonard

> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > >>> On 19-09-26 08:03, Anson Huang wrote:
> > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > >>>>>> 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.
> > >>>>>>
> > >>>>>> +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 }, };
> > >>>>>
> > >>>>> Is this going to be extended in the near future? I see some
> > >>>>> upcoming problems here if someone uses a different
> > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > >>>>
> > >>>> Could be, but I checked the current APIs, ONLY these 2 will be
> > >>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > >>>
> > >>> Okay.
> > >>>
> > >>>> However, after rethink, maybe we should add another imx_sc_rpc
> > >>>> API for those special APIs? To avoid checking it for all the APIs
> > >>>> called which
> > >> may impact some performance.
> > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > >>
> > >> My suggestion is to refactor the code and add a new API for the
> > >> this "no error value" convention. Internally they can call a common
> > >> function with flags.
> > >
> > > If I understand your point correctly, that means the loop check of
> > > whether the API is with "no error value" for every API still NOT be
> > > skipped, it is just refactoring the code, right?
> >
> > There would be no "loop" anywhere: the responsibility would fall on
> > the call to call the right RPC function. In the current layering
> > scheme (drivers -> RPC ->
> > mailbox) the RPC layer treats all calls the same and it's up the the
> > caller to provide information about calling convention.
> >
> > An example implementation:
> > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > * Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp
> > to a flag
> > * Make get button status call __imx_sc_rpc_call_flags with the
> > _IMX_SC_RPC_NOERROR flag
> >
> > Hope this makes my suggestion clearer? Pushing this to the caller is a
> > bit ugly but I think it's worth preserving the fact that the imx rpc
> > core treats services in an uniform way.
> 
> It is clear now, so essentially it is same as 2 separate APIs, still need to change
> the button driver and uid driver to use the special flag, meanwhile, need to
> change the third parament of imx_sc_rpc_call() from bool to u32.

Correct one thing, no need to change the parameter of imx_sc_rpc_call(), just add
another API using the flag as parameter, and imx_sc_rpc_call() calls the new API.

Anson 

> 
> If no one opposes this approach, I will redo the patch together with the
> button driver and uid driver after holiday.
> 
> Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Anson, Leonard,

On 19-09-30 07:42, Anson Huang wrote:
> Hi, Leonard
> 
> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > >>> On 19-09-26 08:03, Anson Huang wrote:
> > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > >>>>>> 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.
> > >>>>>>
> > >>>>>> +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 }, };
> > >>>>>
> > >>>>> Is this going to be extended in the near future? I see some
> > >>>>> upcoming problems here if someone uses a different scu-fw<->kernel
> > >>>>> combination as nxp would suggest.
> > >>>>
> > >>>> Could be, but I checked the current APIs, ONLY these 2 will be used
> > >>>> in Linux kernel, so I ONLY add these 2 APIs for now.
> > >>>
> > >>> Okay.
> > >>>
> > >>>> However, after rethink, maybe we should add another imx_sc_rpc API
> > >>>> for those special APIs? To avoid checking it for all the APIs
> > >>>> called which
> > >> may impact some performance.
> > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > >>
> > >> My suggestion is to refactor the code and add a new API for the this
> > >> "no error value" convention. Internally they can call a common
> > >> function with flags.
> > >
> > > If I understand your point correctly, that means the loop check of
> > > whether the API is with "no error value" for every API still NOT be
> > > skipped, it is just refactoring the code, right?
> > 
> > There would be no "loop" anywhere: the responsibility would fall on the call
> > to call the right RPC function. In the current layering scheme (drivers -> RPC ->
> > mailbox) the RPC layer treats all calls the same and it's up the the caller to
> > provide information about calling convention.
> > 
> > An example implementation:
> > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > * Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp to a
> > flag
> > * Make get button status call __imx_sc_rpc_call_flags with the
> > _IMX_SC_RPC_NOERROR flag
> > 
> > Hope this makes my suggestion clearer? Pushing this to the caller is a bit ugly
> > but I think it's worth preserving the fact that the imx rpc core treats services
> > in an uniform way.
> 
> It is clear now, so essentially it is same as 2 separate APIs, still need to change the
> button driver and uid driver to use the special flag, meanwhile, need to change the
> third parament of imx_sc_rpc_call() from bool to u32.
> 
> If no one opposes this approach, I will redo the patch together with the button driver
> and uid driver after holiday.

As Ansons said that are two approaches and in both ways the caller needs
to know if the error code is valid. Extending the flags seems better to
me but it looks still not that good. One question, does the scu-fw set
the error-msg to something? If not than why should we specify a flag or
a other api? Nowadays the caller needs to know that the error-msg-field isn't
set so if the caller sets the msg-packet to zero and fills the rpc-id
the error-msg-field shouldn't be touched by the firmware. So it should
be zero.

Regards,
  Marco

> Anson
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Marco

> On 19-09-30 07:42, Anson Huang wrote:
> > Hi, Leonard
> >
> > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > >>> On 19-09-26 08:03, Anson Huang wrote:
> > > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > > >>>>>> 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.
> > > >>>>>>
> > > >>>>>> +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 }, };
> > > >>>>>
> > > >>>>> Is this going to be extended in the near future? I see some
> > > >>>>> upcoming problems here if someone uses a different
> > > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > > >>>>
> > > >>>> Could be, but I checked the current APIs, ONLY these 2 will be
> > > >>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > >>>
> > > >>> Okay.
> > > >>>
> > > >>>> However, after rethink, maybe we should add another imx_sc_rpc
> > > >>>> API for those special APIs? To avoid checking it for all the
> > > >>>> APIs called which
> > > >> may impact some performance.
> > > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > > >>
> > > >> My suggestion is to refactor the code and add a new API for the
> > > >> this "no error value" convention. Internally they can call a
> > > >> common function with flags.
> > > >
> > > > If I understand your point correctly, that means the loop check of
> > > > whether the API is with "no error value" for every API still NOT
> > > > be skipped, it is just refactoring the code, right?
> > >
> > > There would be no "loop" anywhere: the responsibility would fall on
> > > the call to call the right RPC function. In the current layering
> > > scheme (drivers -> RPC ->
> > > mailbox) the RPC layer treats all calls the same and it's up the the
> > > caller to provide information about calling convention.
> > >
> > > An example implementation:
> > > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > > * Make a tiny imx_sc_rpc_call wrapper which just converts
> > > resp/noresp to a flag
> > > * Make get button status call __imx_sc_rpc_call_flags with the
> > > _IMX_SC_RPC_NOERROR flag
> > >
> > > Hope this makes my suggestion clearer? Pushing this to the caller is
> > > a bit ugly but I think it's worth preserving the fact that the imx
> > > rpc core treats services in an uniform way.
> >
> > It is clear now, so essentially it is same as 2 separate APIs, still
> > need to change the button driver and uid driver to use the special
> > flag, meanwhile, need to change the third parament of imx_sc_rpc_call()
> from bool to u32.
> >
> > If no one opposes this approach, I will redo the patch together with
> > the button driver and uid driver after holiday.
> 
> As Ansons said that are two approaches and in both ways the caller needs to
> know if the error code is valid. Extending the flags seems better to me but it
> looks still not that good. One question, does the scu-fw set the error-msg to
> something? If not than why should we specify a flag or a other api?
> Nowadays the caller needs to know that the error-msg-field isn't set so if the
> caller sets the msg-packet to zero and fills the rpc-id the error-msg-field
> shouldn't be touched by the firmware. So it should be zero.

The flow are as below for those special APIs with response data but no return value from SCU FW:

1. caller sends msg with a header field and data field, the header field has svc ID and function ID;
2. SCU FW will service the caller and then clear the SVC ID before return, the response data will be
Put in msg data field, and if the APIs has return value, SCU FW will put the return value in function ID of msg;  

The caller has no chance to set the msg-packet to zero and rpc-id, it needs to pass correct rpc-id to SCU FW and
Get response data from SCU FW, and for those special APIs has function ID NOT over-written by SCU FW's return
Value, but the function ID is a unsigned int, and the SCU FW return value is also a unsigned int, so we have no
idea to separate them for no-return value API or error-return API.

With new approach, I can use below 2 flags, the ugly point is user need to know which API to call.

+++ b/include/linux/firmware/imx/ipc.h
@@ -35,6 +35,11 @@ struct imx_sc_rpc_msg {
        uint8_t func;
 };

+#define IMX_SC_RPC_HAVE_RESP   BIT(0) /* caller has response data */
+#define IMX_SC_RPC_NOERROR     BIT(1) /* caller has response data but no return value from SCU FW */
+
+int imx_scu_call_rpc_flags(struct imx_sc_ipc *ipc, void *msg, u32 flags);

Anson

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Anson,

On 19-09-30 08:32, Anson Huang wrote:
> Hi, Marco
> 
> > On 19-09-30 07:42, Anson Huang wrote:
> > > Hi, Leonard
> > >
> > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > > >>> On 19-09-26 08:03, Anson Huang wrote:
> > > > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > > > >>>>>> 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.
> > > > >>>>>>
> > > > >>>>>> +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 }, };
> > > > >>>>>
> > > > >>>>> Is this going to be extended in the near future? I see some
> > > > >>>>> upcoming problems here if someone uses a different
> > > > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > > > >>>>
> > > > >>>> Could be, but I checked the current APIs, ONLY these 2 will be
> > > > >>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > > >>>
> > > > >>> Okay.
> > > > >>>
> > > > >>>> However, after rethink, maybe we should add another imx_sc_rpc
> > > > >>>> API for those special APIs? To avoid checking it for all the
> > > > >>>> APIs called which
> > > > >> may impact some performance.
> > > > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > > > >>
> > > > >> My suggestion is to refactor the code and add a new API for the
> > > > >> this "no error value" convention. Internally they can call a
> > > > >> common function with flags.
> > > > >
> > > > > If I understand your point correctly, that means the loop check of
> > > > > whether the API is with "no error value" for every API still NOT
> > > > > be skipped, it is just refactoring the code, right?
> > > >
> > > > There would be no "loop" anywhere: the responsibility would fall on
> > > > the call to call the right RPC function. In the current layering
> > > > scheme (drivers -> RPC ->
> > > > mailbox) the RPC layer treats all calls the same and it's up the the
> > > > caller to provide information about calling convention.
> > > >
> > > > An example implementation:
> > > > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > > > * Make a tiny imx_sc_rpc_call wrapper which just converts
> > > > resp/noresp to a flag
> > > > * Make get button status call __imx_sc_rpc_call_flags with the
> > > > _IMX_SC_RPC_NOERROR flag
> > > >
> > > > Hope this makes my suggestion clearer? Pushing this to the caller is
> > > > a bit ugly but I think it's worth preserving the fact that the imx
> > > > rpc core treats services in an uniform way.
> > >
> > > It is clear now, so essentially it is same as 2 separate APIs, still
> > > need to change the button driver and uid driver to use the special
> > > flag, meanwhile, need to change the third parament of imx_sc_rpc_call()
> > from bool to u32.
> > >
> > > If no one opposes this approach, I will redo the patch together with
> > > the button driver and uid driver after holiday.
> > 
> > As Ansons said that are two approaches and in both ways the caller needs to
> > know if the error code is valid. Extending the flags seems better to me but it
> > looks still not that good. One question, does the scu-fw set the error-msg to
> > something? If not than why should we specify a flag or a other api?
> > Nowadays the caller needs to know that the error-msg-field isn't set so if the
> > caller sets the msg-packet to zero and fills the rpc-id the error-msg-field
> > shouldn't be touched by the firmware. So it should be zero.
> 
> The flow are as below for those special APIs with response data but no return value from SCU FW:
> 
> 1. caller sends msg with a header field and data field, the header field has svc ID and function ID;
> 2. SCU FW will service the caller and then clear the SVC ID before return, the response data will be
> Put in msg data field, and if the APIs has return value, SCU FW will put the return value in function ID of msg;  

Thanks for the declaration :)

> The caller has no chance to set the msg-packet to zero and rpc-id, it needs to pass correct rpc-id to SCU FW and
> Get response data from SCU FW, and for those special APIs has function ID NOT over-written by SCU FW's return
> Value, but the function ID is a unsigned int, and the SCU FW return value is also a unsigned int, so we have no
> idea to separate them for no-return value API or error-return API.

I see.

> With new approach, I can use below 2 flags, the ugly point is user need to know which API to call.

I don't see any improve using flags because the caller still needs to
know if the scu-fw works (sorry for that) correctly. So we should go to
adapt your approach to handle that within the core and improve the
caller usage.

What about this:

8<-------------------------------------------------------------------------------

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 04a24a863d6e..8f406a0784a4 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -184,6 +184,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 (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
+		    hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
+			ret = 0;
 	}

 out:
8<-------------------------------------------------------------------------------

As you and Leonard said, this scu-fw behaviour is intended. So this will
be not changed over the time else we need a scu-fw version check too.
Also as you said those special functions shouldn't be extended I think a
simple if-statement should work and no performance regressions are
expected.

Regards,
  Marco

> +++ b/include/linux/firmware/imx/ipc.h
> @@ -35,6 +35,11 @@ struct imx_sc_rpc_msg {
>         uint8_t func;
>  };
> 
> +#define IMX_SC_RPC_HAVE_RESP   BIT(0) /* caller has response data */
> +#define IMX_SC_RPC_NOERROR     BIT(1) /* caller has response data but no return value from SCU FW */
> +
> +int imx_scu_call_rpc_flags(struct imx_sc_ipc *ipc, void *msg, u32 flags);
> 
> Anson
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Anson Huang]

Hi, Marco

> On 19-09-30 08:32, Anson Huang wrote:
> > Hi, Marco
> >
> > > On 19-09-30 07:42, Anson Huang wrote:
> > > > Hi, Leonard
> > > >
> > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > > > >>> On 19-09-26 08:03, Anson Huang wrote:
> > > > > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > > > > >>>>>> 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.
> > > > > >>>>>>
> > > > > >>>>>> +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 }, };
> > > > > >>>>>
> > > > > >>>>> Is this going to be extended in the near future? I see
> > > > > >>>>> some upcoming problems here if someone uses a different
> > > > > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > > > > >>>>
> > > > > >>>> Could be, but I checked the current APIs, ONLY these 2 will
> > > > > >>>> be used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > > > >>>
> > > > > >>> Okay.
> > > > > >>>
> > > > > >>>> However, after rethink, maybe we should add another
> > > > > >>>> imx_sc_rpc API for those special APIs? To avoid checking it
> > > > > >>>> for all the APIs called which
> > > > > >> may impact some performance.
> > > > > >>>> Still under discussion, if you have better idea, please advise,
> thanks!
> > > > > >>
> > > > > >> My suggestion is to refactor the code and add a new API for
> > > > > >> the this "no error value" convention. Internally they can
> > > > > >> call a common function with flags.
> > > > > >
> > > > > > If I understand your point correctly, that means the loop
> > > > > > check of whether the API is with "no error value" for every
> > > > > > API still NOT be skipped, it is just refactoring the code, right?
> > > > >
> > > > > There would be no "loop" anywhere: the responsibility would fall
> > > > > on the call to call the right RPC function. In the current
> > > > > layering scheme (drivers -> RPC ->
> > > > > mailbox) the RPC layer treats all calls the same and it's up the
> > > > > the caller to provide information about calling convention.
> > > > >
> > > > > An example implementation:
> > > > > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > > > > * Make a tiny imx_sc_rpc_call wrapper which just converts
> > > > > resp/noresp to a flag
> > > > > * Make get button status call __imx_sc_rpc_call_flags with the
> > > > > _IMX_SC_RPC_NOERROR flag
> > > > >
> > > > > Hope this makes my suggestion clearer? Pushing this to the
> > > > > caller is a bit ugly but I think it's worth preserving the fact
> > > > > that the imx rpc core treats services in an uniform way.
> > > >
> > > > It is clear now, so essentially it is same as 2 separate APIs,
> > > > still need to change the button driver and uid driver to use the
> > > > special flag, meanwhile, need to change the third parament of
> > > > imx_sc_rpc_call()
> > > from bool to u32.
> > > >
> > > > If no one opposes this approach, I will redo the patch together
> > > > with the button driver and uid driver after holiday.
> > >
> > > As Ansons said that are two approaches and in both ways the caller
> > > needs to know if the error code is valid. Extending the flags seems
> > > better to me but it looks still not that good. One question, does
> > > the scu-fw set the error-msg to something? If not than why should we
> specify a flag or a other api?
> > > Nowadays the caller needs to know that the error-msg-field isn't set
> > > so if the caller sets the msg-packet to zero and fills the rpc-id
> > > the error-msg-field shouldn't be touched by the firmware. So it should be
> zero.
> >
> > The flow are as below for those special APIs with response data but no
> return value from SCU FW:
> >
> > 1. caller sends msg with a header field and data field, the header
> > field has svc ID and function ID; 2. SCU FW will service the caller
> > and then clear the SVC ID before return, the response data will be Put
> > in msg data field, and if the APIs has return value, SCU FW will put
> > the return value in function ID of msg;
> 
> Thanks for the declaration :)
> 
> > The caller has no chance to set the msg-packet to zero and rpc-id, it
> > needs to pass correct rpc-id to SCU FW and Get response data from SCU
> > FW, and for those special APIs has function ID NOT over-written by SCU
> > FW's return Value, but the function ID is a unsigned int, and the SCU FW
> return value is also a unsigned int, so we have no idea to separate them for
> no-return value API or error-return API.
> 
> I see.
> 
> > With new approach, I can use below 2 flags, the ugly point is user need to
> know which API to call.
> 
> I don't see any improve using flags because the caller still needs to know if
> the scu-fw works (sorry for that) correctly. So we should go to adapt your
> approach to handle that within the core and improve the caller usage.
> 
> What about this:
> 
> 8<-------------------------------------------------------------------------------
> 
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> scu.c index 04a24a863d6e..8f406a0784a4 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -184,6 +184,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 (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> +		    hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> +			ret = 0;
>  	}
> 
>  out:
> 8<-------------------------------------------------------------------------------
> 
> As you and Leonard said, this scu-fw behaviour is intended. So this will be
> not changed over the time else we need a scu-fw version check too.
> Also as you said those special functions shouldn't be extended I think a
> simple if-statement should work and no performance regressions are
> expected.
> 

I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
another function to check as my V1 patch did, also no need to add another API for users, so
that users no need to know which API to call. But I can NOT use the example you listed upper
directly, the return value from SCU FW could be an error value which is same as the hdr->func,
so I need to saved the original hdr->func and compare them, see below, please help review V2
patch, thanks.

38 +       if (have_resp) {
 39                 sc_ipc->msg = msg;
 40 +               saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
 41 +               saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
 42 +       }

50 +               /*
 51 +                * Some special SCU firmware APIs do NOT have return value
 52 +                * in hdr->func, but they do have response data, those special
 53 +                * APIs are defined as void function in SCU firmware, so they
 54 +                * should be treated as return success always.
 55 +                */
 56 +               if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
 57 +                       (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
 58 +                        saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
 59 +                       ret = 0;  

Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

On 19-10-07 01:21, Anson Huang wrote:
> Hi, Marco
> 
> > On 19-09-30 08:32, Anson Huang wrote:
> > > Hi, Marco
> > >
> > > > On 19-09-30 07:42, Anson Huang wrote:
> > > > > Hi, Leonard
> > > > >
> > > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > > > > >>> On 19-09-26 08:03, Anson Huang wrote:
> > > > > > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > > > > > >>>>>> 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.
> > > > > > >>>>>>
> > > > > > >>>>>> +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 }, };
> > > > > > >>>>>
> > > > > > >>>>> Is this going to be extended in the near future? I see
> > > > > > >>>>> some upcoming problems here if someone uses a different
> > > > > > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > > > > > >>>>
> > > > > > >>>> Could be, but I checked the current APIs, ONLY these 2 will
> > > > > > >>>> be used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > > > > >>>
> > > > > > >>> Okay.
> > > > > > >>>
> > > > > > >>>> However, after rethink, maybe we should add another
> > > > > > >>>> imx_sc_rpc API for those special APIs? To avoid checking it
> > > > > > >>>> for all the APIs called which
> > > > > > >> may impact some performance.
> > > > > > >>>> Still under discussion, if you have better idea, please advise,
> > thanks!
> > > > > > >>
> > > > > > >> My suggestion is to refactor the code and add a new API for
> > > > > > >> the this "no error value" convention. Internally they can
> > > > > > >> call a common function with flags.
> > > > > > >
> > > > > > > If I understand your point correctly, that means the loop
> > > > > > > check of whether the API is with "no error value" for every
> > > > > > > API still NOT be skipped, it is just refactoring the code, right?
> > > > > >
> > > > > > There would be no "loop" anywhere: the responsibility would fall
> > > > > > on the call to call the right RPC function. In the current
> > > > > > layering scheme (drivers -> RPC ->
> > > > > > mailbox) the RPC layer treats all calls the same and it's up the
> > > > > > the caller to provide information about calling convention.
> > > > > >
> > > > > > An example implementation:
> > > > > > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > > > > > * Make a tiny imx_sc_rpc_call wrapper which just converts
> > > > > > resp/noresp to a flag
> > > > > > * Make get button status call __imx_sc_rpc_call_flags with the
> > > > > > _IMX_SC_RPC_NOERROR flag
> > > > > >
> > > > > > Hope this makes my suggestion clearer? Pushing this to the
> > > > > > caller is a bit ugly but I think it's worth preserving the fact
> > > > > > that the imx rpc core treats services in an uniform way.
> > > > >
> > > > > It is clear now, so essentially it is same as 2 separate APIs,
> > > > > still need to change the button driver and uid driver to use the
> > > > > special flag, meanwhile, need to change the third parament of
> > > > > imx_sc_rpc_call()
> > > > from bool to u32.
> > > > >
> > > > > If no one opposes this approach, I will redo the patch together
> > > > > with the button driver and uid driver after holiday.
> > > >
> > > > As Ansons said that are two approaches and in both ways the caller
> > > > needs to know if the error code is valid. Extending the flags seems
> > > > better to me but it looks still not that good. One question, does
> > > > the scu-fw set the error-msg to something? If not than why should we
> > specify a flag or a other api?
> > > > Nowadays the caller needs to know that the error-msg-field isn't set
> > > > so if the caller sets the msg-packet to zero and fills the rpc-id
> > > > the error-msg-field shouldn't be touched by the firmware. So it should be
> > zero.
> > >
> > > The flow are as below for those special APIs with response data but no
> > return value from SCU FW:
> > >
> > > 1. caller sends msg with a header field and data field, the header
> > > field has svc ID and function ID; 2. SCU FW will service the caller
> > > and then clear the SVC ID before return, the response data will be Put
> > > in msg data field, and if the APIs has return value, SCU FW will put
> > > the return value in function ID of msg;
> > 
> > Thanks for the declaration :)
> > 
> > > The caller has no chance to set the msg-packet to zero and rpc-id, it
> > > needs to pass correct rpc-id to SCU FW and Get response data from SCU
> > > FW, and for those special APIs has function ID NOT over-written by SCU
> > > FW's return Value, but the function ID is a unsigned int, and the SCU FW
> > return value is also a unsigned int, so we have no idea to separate them for
> > no-return value API or error-return API.
> > 
> > I see.
> > 
> > > With new approach, I can use below 2 flags, the ugly point is user need to
> > know which API to call.
> > 
> > I don't see any improve using flags because the caller still needs to know if
> > the scu-fw works (sorry for that) correctly. So we should go to adapt your
> > approach to handle that within the core and improve the caller usage.
> > 
> > What about this:
> > 
> > 8<-------------------------------------------------------------------------------
> > 
> > diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> > scu.c index 04a24a863d6e..8f406a0784a4 100644
> > --- a/drivers/firmware/imx/imx-scu.c
> > +++ b/drivers/firmware/imx/imx-scu.c
> > @@ -184,6 +184,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 (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> > +		    hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> > +			ret = 0;
> >  	}
> > 
> >  out:
> > 8<-------------------------------------------------------------------------------
> > 
> > As you and Leonard said, this scu-fw behaviour is intended. So this will be
> > not changed over the time else we need a scu-fw version check too.
> > Also as you said those special functions shouldn't be extended I think a
> > simple if-statement should work and no performance regressions are
> > expected.
> > 
> 
> I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
> another function to check as my V1 patch did, also no need to add another API for users, so
> that users no need to know which API to call. But I can NOT use the example you listed upper
> directly, the return value from SCU FW could be an error value which is same as the hdr->func,

I tought the SCU FW won't touch this field.

> so I need to saved the original hdr->func and compare them, see below, please help review V2
> patch, thanks.

I did :)

Regards,
  Marco

> 
> 38 +       if (have_resp) {
>  39                 sc_ipc->msg = msg;
>  40 +               saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
>  41 +               saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
>  42 +       }
> 
> 50 +               /*
>  51 +                * Some special SCU firmware APIs do NOT have return value
>  52 +                * in hdr->func, but they do have response data, those special
>  53 +                * APIs are defined as void function in SCU firmware, so they
>  54 +                * should be treated as return success always.
>  55 +                */
>  56 +               if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
>  57 +                       (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
>  58 +                        saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
>  59 +                       ret = 0;  
> 
> Anson

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel