From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A99FCC4360C for ; Mon, 30 Sep 2019 08:14:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8693520679 for ; Mon, 30 Sep 2019 08:14:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729829AbfI3IOi (ORCPT ); Mon, 30 Sep 2019 04:14:38 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:44529 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726008AbfI3IOi (ORCPT ); Mon, 30 Sep 2019 04:14:38 -0400 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1iEqpI-00012w-AU; Mon, 30 Sep 2019 10:14:36 +0200 Received: from mfe by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1iEqpG-0002Dx-6y; Mon, 30 Sep 2019 10:14:34 +0200 Date: Mon, 30 Sep 2019 10:14:34 +0200 From: Marco Felsch To: Anson Huang Cc: Leonard Crestez , Aisheng Dong , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "linux-kernel@vger.kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "festevam@gmail.com" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs Message-ID: <20190930081434.qrrv3yqczzxihntm@pengutronix.de> References: <1569406066-16626-1-git-send-email-Anson.Huang@nxp.com> <20190926075914.i7tsd3cbpitrqe4q@pengutronix.de> <20190926100558.egils3ds37m3s5wo@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 09:49:54 up 135 days, 14:08, 87 users, load average: 0.01, 0.02, 0.07 User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: mfe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Anson, Leonard, 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. Regards, Marco > Anson > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- 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 | From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B5558C388F3 for ; Mon, 30 Sep 2019 08:15:20 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 87F3B20679 for ; Mon, 30 Sep 2019 08:15:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="rOx9F/zO" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 87F3B20679 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XzXealamejJ0DFC2BvvXoeUEbuFesXz55W8edPffOl8=; b=rOx9F/zOPoz1DI lJ6vP63VMeuq17i8XBGWhj2Pan4SMD7OiNDuZvv8wo4iCQK1dQXmU4p+3osQZ0LV9C6fKXSV/4Yzb kStXAI9MNERyl7kJAR6VhHKBcsY4/Td9ldGCbo6T6d123/rofwwaB4PHOKosESKxk+Dm9GdMY+iA0 cnBEq0hfr9s12CagTS2utXiaHlGdfoJYJhCuF9p2MU75jSud+mLsfQ0xuFGLcw7xGr5P/WTMHtSz+ m0ADy9bkFf3/TR+Hks9SX3LgV9x3BiHo9blPot/IDSrUt49d/w3YOtLkvn0WRAKBGG+yJYLLitqsI wY1kgjrjNYcdGZoJg1vQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1iEqpz-0003AQ-T3; Mon, 30 Sep 2019 08:15:19 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iEqpQ-0001Vg-2F for linux-arm-kernel@lists.infradead.org; Mon, 30 Sep 2019 08:14:45 +0000 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1iEqpI-00012w-AU; Mon, 30 Sep 2019 10:14:36 +0200 Received: from mfe by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1iEqpG-0002Dx-6y; Mon, 30 Sep 2019 10:14:34 +0200 Date: Mon, 30 Sep 2019 10:14:34 +0200 From: Marco Felsch To: Anson Huang Subject: Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs Message-ID: <20190930081434.qrrv3yqczzxihntm@pengutronix.de> References: <1569406066-16626-1-git-send-email-Anson.Huang@nxp.com> <20190926075914.i7tsd3cbpitrqe4q@pengutronix.de> <20190926100558.egils3ds37m3s5wo@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 09:49:54 up 135 days, 14:08, 87 users, load average: 0.01, 0.02, 0.07 User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: mfe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190930_011444_148925_66C4CA27 X-CRM114-Status: GOOD ( 32.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Aisheng Dong , "festevam@gmail.com" , "s.hauer@pengutronix.de" , "linux-kernel@vger.kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , Leonard Crestez , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Anson, Leonard, 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. Regards, Marco > Anson > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel