From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 8FB3921C8EFB5 for ; Wed, 18 Oct 2017 05:08:22 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3FBB5C0587F7; Wed, 18 Oct 2017 12:11:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3FBB5C0587F7 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-113.rdu2.redhat.com [10.10.120.113]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7473E58827; Wed, 18 Oct 2017 12:11:51 +0000 (UTC) To: Julien Grall , star.zeng@intel.com, eric.dong@intel.com, pankaj.bansal@nxp.com, leif.lindholm@linaro.org Cc: edk2-devel@lists.01.org References: <20171018101951.1890-1-julien.grall@linaro.org> From: Laszlo Ersek Message-ID: <1f489f32-e185-d430-a4fb-29906ddf2c32@redhat.com> Date: Wed, 18 Oct 2017 14:11:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171018101951.1890-1-julien.grall@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 18 Oct 2017 12:11:59 +0000 (UTC) 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: Wed, 18 Oct 2017 12:08:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 == RETURN_UNSUPPORTED ) The extra spaces within the parens are Xen coding style, not edk2 coding style; please remove them. > + return RETURN_SUCCESS; The edk2 coding style requires braces. The edk2 coding style requires two spaces as indentation, in this context. > > return Status; > } > The UEFI spec (v2.7) describes the following return values for EFI_SERIAL_IO_PROTOCOL.Reset(): - EFI_SUCCESS: The serial device was reset. - EFI_DEVICE_ERROR: The serial device could not be reset. For the EFI_SERIAL_IO_PROTOCOL.SetAttributes() member function: - 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. 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: MdePkg/Include/Library/SerialPortLib.h The API defines the following return codes: @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. Therefore I think the following should be done: (1) The SerialPortSetAttributes() implementation in "OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c" is correct; returning RETURN_UNSUPPORTED is valid, according to the lib class header. (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: RETURN_SUCCESS -> EFI_SUCCESS RETURN_UNSUPPORTED -> EFI_INVALID_PARAMETER RETURN_INVALID_PARAMETER -> EFI_INVALID_PARAMETER everything else -> EFI_DEVICE_ERROR (That is, the outer interface requires us to map "The serial device does not support this operation." to "One or more of the attributes has an unsupported value." ... As long as we want to adhere to the UEFI-2.7 spec.) (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: EFI_SUCCESS -> EFI_SUCCESS EFI_INVALID_PARAMETER -> EFI_SUCCESS everything else -> EFI_DEVICE_ERROR The idea (correctly captured in your patch, IMO) is that we're already past the SerialPortInitialize() function, so restoring the attributes is "best effort"; if it fails, we should still report the reset operation as successful. 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 the UEFI spec). I suggest waiting for feedback from Star & Eric. Dependent on their response, 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). Bonus comment: (4) The propagation of the SerialPortInitialize() retval in SerialReset() looks correct, thankfully. (Both callee and caller are expected to return one of *_SUCCESS and *_DEVICE_ERROR.) Thanks Laszlo