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