From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AF86A2034A7CB for ; Mon, 23 Oct 2017 22:40:11 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP; 23 Oct 2017 22:43:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,426,1503385200"; d="scan'208";a="1028579981" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 23 Oct 2017 22:43:54 -0700 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 23 Oct 2017 22:43:54 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 23 Oct 2017 22:43:53 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Tue, 24 Oct 2017 13:43:52 +0800 From: "Zeng, Star" To: "Ni, Ruiyu" , Laszlo Ersek , "Julien Grall" , "Dong, Eric" , "pankaj.bansal@nxp.com" , "leif.lindholm@linaro.org" CC: "edk2-devel@lists.01.org" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported Thread-Index: AQHTR/q0Q6xtpNlJx0GsJPlQbCk51KLo/naAgAD5GACACIwywA== Date: Tue, 24 Oct 2017 05:43:50 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9ACE44@shsmsx102.ccr.corp.intel.com> References: <20171018101951.1890-1-julien.grall@linaro.org> <1f489f32-e185-d430-a4fb-29906ddf2c32@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BA9DD62@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BA9DD62@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Oct 2017 05:40:11 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I also agree the mapping. Julien, There is a typo in commit log " instropecting " should be " introspecting "= . Anyway, you may need the commit log according to the mapping, you can take = care of the typo in the updated commit log. Thanks, Star -----Original Message----- From: Ni, Ruiyu=20 Sent: Thursday, October 19, 2017 11:03 AM To: Laszlo Ersek ; Julien Grall ; Zeng, Star ; Dong, Eric ; pank= aj.bansal@nxp.com; leif.lindholm@linaro.org Cc: edk2-devel@lists.01.org Subject: RE: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when = SetAttributes is not supported Laszlo, I agree with your status mapping. It will make the implementation more clear and easier to maintain. Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Laszlo Ersek > Sent: Wednesday, October 18, 2017 8:12 PM > To: Julien Grall ; Zeng, Star=20 > ; Dong, Eric ;=20 > pankaj.bansal@nxp.com; leif.lindholm@linaro.org > Cc: edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset=20 > when SetAttributes is not supported >=20 > On 10/18/17 12:19, Julien Grall wrote: > > After commit 91cc526b15 "MdeModulePkg/SerialDxe: Fix not able to=20 > > change serial attributes", serial is initialized using the reset=20 > > method that will call SetAttributes. > > > > However, SetAttributes may not be supported by the driver and will=20 > > return an error (i.e RETURN_UNSUPPORTED) that will be propagate and=20 > > lead to UEFI failing to get the console setup. > > > > For instance, this is the case when using the Xen console driver. > > > > Fix it by instropecting the result and return RETURN_SUCCESS when=20 > > the driver report it is not supported (i.e RETURN_UNSUPPORTED). > > > > Contributed-under: Tianocore Contribution Agreement 1.1 > > Signed-off-by: Julien Grall > > --- > > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > index ebcd927263..4253e0b8ea 100644 > > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > > @@ -238,6 +238,12 @@ SerialReset ( > > (UINT8) This->Mode->DataBits, > > (EFI_STOP_BITS_TYPE) This->Mode->StopBits > > ); > > + // > > + // The serial device may not support SetAttributes. > > + // Set the status to RETURN_SUCCESS to prevent later failure. > > + // > > + if ( Status =3D=3D RETURN_UNSUPPORTED ) >=20 > The extra spaces within the parens are Xen coding style, not edk2=20 > coding style; please remove them. >=20 > > + return RETURN_SUCCESS; >=20 > The edk2 coding style requires braces. >=20 > The edk2 coding style requires two spaces as indentation, in this context= . >=20 > > > > return Status; > > } > > >=20 > The UEFI spec (v2.7) describes the following return values for > EFI_SERIAL_IO_PROTOCOL.Reset(): >=20 > - EFI_SUCCESS: The serial device was reset. > - EFI_DEVICE_ERROR: The serial device could not be reset. >=20 > For the EFI_SERIAL_IO_PROTOCOL.SetAttributes() member function: >=20 > - EFI_SUCCESS: The new attributes were set on the serial device. > - EFI_INVALID_PARAMETER: One or more of the attributes has an > unsupported value. > - EFI_DEVICE_ERROR: The serial device is not functioning correctly. >=20 > In MdeModulePkg/Universal/SerialDxe, the SetAttributes() member=20 > function is implemented by SerialSetAttributes(), and it delegates the=20 > operation to the SerialPortSetAttributes() API from the following library= class: >=20 > MdePkg/Include/Library/SerialPortLib.h >=20 > The API defines the following return codes: >=20 > @retval RETURN_SUCCESS The new attributes were set on the > serial device. > @retval RETURN_UNSUPPORTED The serial device does not support > this operation. > @retval RETURN_INVALID_PARAMETER One or more of the attributes has=20 > an > unsupported value. > @retval RETURN_DEVICE_ERROR The serial device is not functioning > correctly. >=20 > Therefore I think the following should be done: >=20 > (1) The SerialPortSetAttributes() implementation in=20 > "OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c" is=20 > correct; returning RETURN_UNSUPPORTED is valid, according to the lib=20 > class header. >=20 > (2) The direct propagation of the return value in=20 > SerialSetAttributes() [MdeModulePkg/Universal/SerialDxe/SerialIo.c]=20 > does not look correct. It should implement the following mapping: >=20 > RETURN_SUCCESS -> EFI_SUCCESS > RETURN_UNSUPPORTED -> EFI_INVALID_PARAMETER > RETURN_INVALID_PARAMETER -> EFI_INVALID_PARAMETER > everything else -> EFI_DEVICE_ERROR >=20 > (That is, the outer interface requires us to map >=20 > "The serial device does not support this operation." >=20 > to >=20 > "One or more of the attributes has an unsupported value." >=20 > ... As long as we want to adhere to the UEFI-2.7 spec.) >=20 > (3) The SerialReset() function in > "MdeModulePkg/Universal/SerialDxe/SerialIo.c" transparently propagates=20 > the return value of the internally called SerialSetAttributes()=20 > function. This looks incorrect as well; it should do the following mappin= g: >=20 > EFI_SUCCESS -> EFI_SUCCESS > EFI_INVALID_PARAMETER -> EFI_SUCCESS > everything else -> EFI_DEVICE_ERROR >=20 > The idea (correctly captured in your patch, IMO) is that we're already=20 > past the SerialPortInitialize() function, so restoring the attributes=20 > is "best effort"; if it fails, we should still report the reset operation= as successful. >=20 > I just think that EFI_SERIAL_IO_PROTOCOL.SetAttributes() is not=20 > supposed to return EFI_UNSUPPORTED at all (it is an external interface=20 > governed by the UEFI spec). >=20 > I suggest waiting for feedback from Star & Eric. Dependent on their=20 > response, your patch could be good enough (once you fix up the coding sty= le issues). > Or else, they could agree with me that the return value mapping of > SerialSetAttributes() should be corrected first (2), and then your=20 > patch should be please adapted as well (3). >=20 > Bonus comment: >=20 > (4) The propagation of the SerialPortInitialize() retval in > SerialReset() looks correct, thankfully. (Both callee and caller are=20 > expected to return one of *_SUCCESS and *_DEVICE_ERROR.) >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel