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 |