Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs

From: Marco Felsch <m.felsch@pengutronix.de>
To: Anson Huang <anson.huang@nxp.com>
Cc: Aisheng Dong <aisheng.dong@nxp.com>,
	"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>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	"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: Thu, 26 Sep 2019 12:05:58 +0200	[thread overview]
Message-ID: <20190926100558.egils3ds37m3s5wo@pengutronix.de> (raw)
In-Reply-To: <DB3PR0402MB391683202692BEAE4D2CD9C1F5860@DB3PR0402MB3916.eurprd04.prod.outlook.com>

Hi Anson,

On 19-09-26 08:03, Anson Huang wrote:
> Hi, Marco
> 
> > 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.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > > 	- This patch is based on the patch of
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > >
> > hwork.kernel.org%2Fpatch%2F11129553%2F&amp;data=02%7C01%7Canson.
> > huang%
> > >
> > 40nxp.com%7C1f4108cc25eb4618f43c08d742576fa3%7C686ea1d3bc2b4c6fa
> > 92cd99
> > >
> > c5c301635%7C0%7C0%7C637050815608963707&amp;sdata=BZBg4cOR2rP%2
> > BRBNn15i
> > > Qq3%2FXBYwhuCLkgYzFRbfEgVU%3D&amp;reserved=0
> > > ---
> > >  drivers/firmware/imx/imx-scu.c | 34
> > > ++++++++++++++++++++++++++++++++--
> > >  1 file changed, 32 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/firmware/imx/imx-scu.c
> > > b/drivers/firmware/imx/imx-scu.c index 869be7a..ced5b12 100644
> > > --- a/drivers/firmware/imx/imx-scu.c
> > > +++ b/drivers/firmware/imx/imx-scu.c
> > > @@ -78,6 +78,11 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] =
> > {
> > >  	-EIO,	 /* IMX_SC_ERR_FAIL */
> > >  };
> > >
> > > +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!

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..

Regards,
  Marco

> 
> Anson

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel