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 223E221B00DC4 for ; Wed, 29 Nov 2017 10:20:24 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8FCD6883AB; Wed, 29 Nov 2017 18:24:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-48.rdu2.redhat.com [10.10.120.48]) by smtp.corp.redhat.com (Postfix) with ESMTP id D15015D6A3; Wed, 29 Nov 2017 18:24:46 +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: <20171129172823.2906-1-julien.grall@linaro.org> <20171129172823.2906-3-julien.grall@linaro.org> From: Laszlo Ersek Message-ID: <25aa1898-f6ac-e743-34ad-eeb9d553db17@redhat.com> Date: Wed, 29 Nov 2017 19:24:45 +0100 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: <20171129172823.2906-3-julien.grall@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 29 Nov 2017 18:24:48 +0000 (UTC) Subject: Re: [PATCH v3 2/3] MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes 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, 29 Nov 2017 18:20:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/29/17 18:28, Julien Grall wrote: > SerialSetAttributes is meant to match the behavior of the function > EFI_SERIAL_IO_PROTOCOL.SetAttributes() in the UEFI spec (v2.7). This > means the function can only return: > - EFI_SUCCESS > - EFI_INVALID_PARAMETER > - EFI_DEVICE_ERROR > > However the function SerialPortSetAttributes may also validly return > EFI_UNSUPPORTED. For instance this is the case of the Xen Console > driver. > > EFI_UNSUPPORTED could be also interpreted as "One or more of the attributes > has an unsupported value". So return EFI_INVALID_PARAMETER in that case. > > Lastly, to prevent another return slipping in the future, all the errors > but EFI_INVALID_PARAMETERR and EFI_UNSUPPORTED will return "EFI_INVALID_PARAMETERR" has a typo here. Other than that, series Reviewed-by: Laszlo Ersek Thanks Laszlo > EFI_DEVICE_ERROR. > > Contributed-under: Tianocore Contribution Agreement 1.1 > Signed-off-by: Julien Grall > Reviewed-by: Star Zeng > > --- > > Star, I found a prototype for SetAttributes in SerialIo.c as well > and updated the description there. I decided to keep the Reviewed-by > even with that change. Let me know whether it is fine for you. > > Changes in v3: > - Add description of EFI_INVALID_PARAMETER above the prototypes > for SetAttributes as well. > - Add Star reviewed-by > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 14 +++++++++----- > MdePkg/Include/Protocol/SerialIo.h | 5 +++-- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index 19fc889c25..ee10ec7e05 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -66,8 +66,9 @@ SerialReset ( > value of DefaultStopBits will use the device's default number of > stop bits. > > - @retval EFI_SUCCESS The device was reset. > - @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > + @retval EFI_SUCCESS The device was reset. > + @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value. > + @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > > **/ > EFI_STATUS > @@ -264,8 +265,9 @@ SerialReset ( > value of DefaultStopBits will use the device's default number of > stop bits. > > - @retval EFI_SUCCESS The device was reset. > - @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > + @retval EFI_SUCCESS The device was reset. > + @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value. > + @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > > **/ > EFI_STATUS > @@ -323,8 +325,10 @@ SerialSetAttributes ( > DataBits = OriginalDataBits; > StopBits = OriginalStopBits; > Status = EFI_SUCCESS; > + } else if (Status == EFI_INVALID_PARAMETER || Status == EFI_UNSUPPORTED) { > + return EFI_INVALID_PARAMETER; > } else { > - return Status; > + return EFI_DEVICE_ERROR; > } > } > > diff --git a/MdePkg/Include/Protocol/SerialIo.h b/MdePkg/Include/Protocol/SerialIo.h > index 84cb34364d..1263dc4fe9 100644 > --- a/MdePkg/Include/Protocol/SerialIo.h > +++ b/MdePkg/Include/Protocol/SerialIo.h > @@ -125,8 +125,9 @@ EFI_STATUS > value of DefaultStopBits will use the device's default number of > stop bits. > > - @retval EFI_SUCCESS The device was reset. > - @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > + @retval EFI_SUCCESS The device was reset. > + @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value. > + @retval EFI_DEVICE_ERROR The serial device is not functioning correctly. > > **/ > typedef >