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.