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