From: Leonard Crestez <leonard.crestez@nxp.com> To: Anson Huang <anson.huang@nxp.com>, Marco Felsch <m.felsch@pengutronix.de>, Aisheng Dong <aisheng.dong@nxp.com> Cc: "shawnguo@kernel.org" <shawnguo@kernel.org>, "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>, "kernel@pengutronix.de" <kernel@pengutronix.de>, "festevam@gmail.com" <festevam@gmail.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, dl-linux-imx <linux-imx@nxp.com> Subject: Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs Date: Fri, 27 Sep 2019 11:16:24 +0000 [thread overview] Message-ID: <VI1PR04MB70236265478233D8025706F1EE810@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw) In-Reply-To: DB3PR0402MB391675F9BF6FCA315B124BEBF5810@DB3PR0402MB3916.eurprd04.prod.outlook.com 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
WARNING: multiple messages have this Message-ID (diff)
From: Leonard Crestez <leonard.crestez@nxp.com> To: Anson Huang <anson.huang@nxp.com>, Marco Felsch <m.felsch@pengutronix.de>, Aisheng Dong <aisheng.dong@nxp.com> Cc: "shawnguo@kernel.org" <shawnguo@kernel.org>, "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, dl-linux-imx <linux-imx@nxp.com>, "kernel@pengutronix.de" <kernel@pengutronix.de>, "festevam@gmail.com" <festevam@gmail.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs Date: Fri, 27 Sep 2019 11:16:24 +0000 [thread overview] Message-ID: <VI1PR04MB70236265478233D8025706F1EE810@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw) In-Reply-To: DB3PR0402MB391675F9BF6FCA315B124BEBF5810@DB3PR0402MB3916.eurprd04.prod.outlook.com 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
next prev parent reply other threads:[~2019-09-27 11:16 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-25 10:07 [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs Anson Huang 2019-09-25 10:07 ` Anson Huang 2019-09-25 13:13 ` Leonard Crestez 2019-09-25 13:13 ` Leonard Crestez 2019-09-26 0:34 ` Anson Huang 2019-09-26 0:34 ` Anson Huang 2019-09-26 7:59 ` Marco Felsch 2019-09-26 7:59 ` Marco Felsch 2019-09-26 8:03 ` Anson Huang 2019-09-26 8:03 ` Anson Huang 2019-09-26 10:05 ` Marco Felsch 2019-09-26 10:05 ` Marco Felsch 2019-09-26 13:25 ` Leonard Crestez 2019-09-26 13:25 ` Leonard Crestez 2019-09-27 1:20 ` Anson Huang 2019-09-27 1:20 ` Anson Huang 2019-09-27 9:06 ` Marco Felsch 2019-09-27 9:06 ` Marco Felsch 2019-09-27 9:27 ` Anson Huang 2019-09-27 9:27 ` Anson Huang 2019-09-27 11:22 ` Leonard Crestez 2019-09-27 11:22 ` Leonard Crestez 2019-09-29 1:12 ` Anson Huang 2019-09-29 1:12 ` Anson Huang 2019-09-27 11:16 ` Leonard Crestez [this message] 2019-09-27 11:16 ` Leonard Crestez 2019-09-30 7:28 ` Leonard Crestez 2019-09-30 7:28 ` Leonard Crestez 2019-09-30 7:42 ` Anson Huang 2019-09-30 7:42 ` Anson Huang 2019-09-30 7:54 ` Anson Huang 2019-09-30 7:54 ` Anson Huang 2019-09-30 8:14 ` Marco Felsch 2019-09-30 8:14 ` Marco Felsch 2019-09-30 8:32 ` Anson Huang 2019-09-30 8:32 ` Anson Huang 2019-09-30 10:02 ` Marco Felsch 2019-09-30 10:02 ` Marco Felsch 2019-10-07 1:21 ` Anson Huang 2019-10-07 1:21 ` Anson Huang 2019-10-07 8:07 ` Marco Felsch 2019-10-07 8:07 ` Marco Felsch
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=VI1PR04MB70236265478233D8025706F1EE810@VI1PR04MB7023.eurprd04.prod.outlook.com \ --to=leonard.crestez@nxp.com \ --cc=aisheng.dong@nxp.com \ --cc=anson.huang@nxp.com \ --cc=festevam@gmail.com \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=m.felsch@pengutronix.de \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.