From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 F08C721E7821E for ; Wed, 18 Oct 2017 19:59:43 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 18 Oct 2017 20:03:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,399,1503385200"; d="scan'208";a="324960383" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 18 Oct 2017 20:03:20 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 18 Oct 2017 20:03:20 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Thu, 19 Oct 2017 11:03:19 +0800 From: "Ni, Ruiyu" To: Laszlo Ersek , Julien Grall , "Zeng, Star" , "Dong, Eric" , "pankaj.bansal@nxp.com" , "leif.lindholm@linaro.org" CC: "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported Thread-Index: AQHTR/q9TJ0LDU+NGUadju29s3w92aLo/naAgAF+/rA= Date: Thu, 19 Oct 2017 03:03:18 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA9DD62@SHSMSX104.ccr.corp.intel.com> References: <20171018101951.1890-1-julien.grall@linaro.org> <1f489f32-e185-d430-a4fb-29906ddf2c32@redhat.com> In-Reply-To: <1f489f32-e185-d430-a4fb-29906ddf2c32@redhat.com> Accept-Language: en-US, zh-CN 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: Thu, 19 Oct 2017 02:59:45 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 > Laszlo Ersek > Sent: Wednesday, October 18, 2017 8:12 PM > To: Julien Grall ; Zeng, Star ; > Dong, Eric ; pankaj.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 >=20 > On 10/18/17 12:19, Julien Grall wrote: > > After commit 91cc526b15 "MdeModulePkg/SerialDxe: Fix not able to > > change serial attributes", serial is initialized using the reset > > method that will call SetAttributes. > > > > However, SetAttributes may not be supported by the driver and will > > return an error (i.e RETURN_UNSUPPORTED) that will be propagate and > > 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 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 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 > function is implemented by SerialSetAttributes(), and it delegates the > 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 > 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 > "OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c" is > correct; returning RETURN_UNSUPPORTED is valid, according to the lib clas= s > header. >=20 > (2) The direct propagation of the return value in SerialSetAttributes() > [MdeModulePkg/Universal/SerialDxe/SerialIo.c] 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 > the return value of the internally called SerialSetAttributes() function.= This > looks incorrect as well; it should do the following mapping: >=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 pa= st > the SerialPortInitialize() function, so restoring the attributes 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 supposed = to > return EFI_UNSUPPORTED at all (it is an external interface governed by th= e > UEFI spec). >=20 > I suggest waiting for feedback from Star & Eric. Dependent on their respo= nse, > your patch could be good enough (once you fix up the coding style issues)= . > Or else, they could agree with me that the return value mapping of > SerialSetAttributes() should be corrected first (2), and then your 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 expe= cted 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