From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c06::229; helo=mail-io0-x229.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 71A99203525FD for ; Thu, 26 Oct 2017 06:15:51 -0700 (PDT) Received: by mail-io0-x229.google.com with SMTP id b186so5281312iof.8 for ; Thu, 26 Oct 2017 06:19:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=UkqRv8DX8EXBTjRXZjPWpoOpfR8ORfCDM9oGu1SiG7k=; b=E5Ulep3bWc9/oa4US9G/UneM0o8/mO6n/aNWi+HL4mN22GvhwsywaxJHKJuQ3Z9HcG UnmWWttRmJYK/N/qEUjI2sI37exPpcjlNs5lccHrqUWfE2JhVgUA/mxEOuAkJB7A2uvJ xhmgPj+KlSFvOwxStrTmT4ocLxBuVdd8QH8Oex4Ueqok9YnLeE0u8+JsZulB3PYcT5Ob /egHJOUCFMg5apdV+ag31/Cbn+brwlWWicnCtBuH+WnJ8Ae17cGp1JP5r5icgcHnthsj Z1/6lfjyLI64Rse+bI+75wDeZgQcZ6kGvL+ClKhKDPdHctx+PfcxDIM/3m7HnG0/APus 91Jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UkqRv8DX8EXBTjRXZjPWpoOpfR8ORfCDM9oGu1SiG7k=; b=NAcuf0Icj11KoUoQaqToNBrlGDTSWs4B+LnUOKG/MkTEV/Z4QN0qktF7HQMqNCtgpJ ulrJ1o4UwIazxBhV2rYHsCcCU51uGrY8LWk+C3Y+WCNdOusV3j0aYx4o7rSIDbvAW7hc UhfXimY7/vfYEfrDFRE4+Nl6Yb+GKVSfADaXhIs3yal7pOaPuRWn/GHhc0cKPQv+5wbH lq2JtxzfoKUIw+jhcNTQ9DIXaSUEQfVZHt2mGG0GGnGrehoqDauO5KqTjhBm5XPNWQwU FPCsnWswRZjDQlvhcCRlsDrwsP8BVPsHVRrDqkBfZZKTWo6oWe7+9KCKW/yPDHtFy9B4 SpMQ== X-Gm-Message-State: AMCzsaV/07RYrxRENZTvk3NvIcima+88CvUnTp9E3dyHjoz/wydw8AIB WLCVE8XMTWxTBOemIZSss0bE2KUBHhIemM5XoW2TJQ== X-Google-Smtp-Source: ABhQp+Rl1jmlEQdfOU/28SXowlzWR+lINzIUmqECct+bDGpOlt84fqXPjHKaE0k1iW49hKI8PEc85mXE5WH1nGmDKoo= X-Received: by 10.36.9.144 with SMTP id 138mr2453590itm.106.1509023976837; Thu, 26 Oct 2017 06:19:36 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.167.208 with HTTP; Thu, 26 Oct 2017 06:19:36 -0700 (PDT) In-Reply-To: <20171026125143.bki4fywgtf3ybvmm@bivouac.eciton.net> References: <1508980777-29006-1-git-send-email-mw@semihalf.com> <1508980777-29006-2-git-send-email-mw@semihalf.com> <20171026125143.bki4fywgtf3ybvmm@bivouac.eciton.net> From: Marcin Wojtas Date: Thu, 26 Oct 2017 15:19:36 +0200 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan , David Greeson Subject: Re: [platforms: PATCH 01/10] Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail 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, 26 Oct 2017 13:15:51 -0000 Content-Type: text/plain; charset="UTF-8" Hi Leif, 2017-10-26 14:51 GMT+02:00 Leif Lindholm : > On Thu, Oct 26, 2017 at 03:19:28AM +0200, Marcin Wojtas wrote: >> From: David Greeson >> >> Although the I2C transaction routines were prepared to >> return their status, they were never used. This could >> cause bus lock-up e.g. in case of failing to send a >> slave address, the data transfer was attempted to be >> continued anyway. >> >> This patch fixes faulty behavior by checking transaction >> status and stopping it immediately, once the fail >> is detected. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: David Greeson >> [Style adjustment and cleanup] >> Signed-off-by: Marcin Wojtas >> --- >> Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 34 +++++++++++++++++--- >> 1 file changed, 29 insertions(+), 5 deletions(-) >> >> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c >> index d85ee0b..7faf1f7 100755 >> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c >> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c >> @@ -565,6 +565,7 @@ MvI2cStartRequest ( >> UINTN Transmitted; >> I2C_MASTER_CONTEXT *I2cMasterContext = I2C_SC_FROM_MASTER(This); >> EFI_I2C_OPERATION *Operation; >> + EFI_STATUS Status = EFI_SUCCESS; >> >> ASSERT (RequestPacket != NULL); >> ASSERT (I2cMasterContext != NULL); >> @@ -574,35 +575,58 @@ MvI2cStartRequest ( >> ReadMode = Operation->Flags & I2C_FLAG_READ; >> >> if (Count == 0) { >> - MvI2cStart ( I2cMasterContext, >> + Status = MvI2cStart (I2cMasterContext, >> (SlaveAddress << 1) | ReadMode, >> I2C_TRANSFER_TIMEOUT > > Much as I appreciate seeing this form of the code, since it simplifies > seeing the functional changes, this does cause those lines left > unchanges to no longer conform to coding style. > Can you please adjust throughout for a v2? > No problem. I of course saw style violations, but I gave up on them for "no mix of functional improvements and style cleanups" contraint :) I will correct the modified function calls. >> ); >> } else if (!(Operation->Flags & I2C_FLAG_NORESTART)) { >> - MvI2cRepeatedStart ( I2cMasterContext, >> + Status = MvI2cRepeatedStart (I2cMasterContext, >> (SlaveAddress << 1) | ReadMode, >> I2C_TRANSFER_TIMEOUT >> ); >> } >> >> + /* I2C transaction was aborted, so stop further transactions */ >> + if (EFI_ERROR (Status)) { >> + MvI2cStop (I2cMasterContext); >> + break; >> + } >> + >> + /* >> + * If sending the slave address was successful, >> + * proceed to read or write section. >> + */ >> if (ReadMode) { >> - MvI2cRead ( I2cMasterContext, >> + Status = MvI2cRead (I2cMasterContext, >> Operation->Buffer, >> Operation->LengthInBytes, >> &Transmitted, >> Count == 1, >> I2C_TRANSFER_TIMEOUT >> ); >> + Operation->LengthInBytes = Transmitted; >> } else { >> - MvI2cWrite ( I2cMasterContext, >> + Status = MvI2cWrite (I2cMasterContext, >> Operation->Buffer, >> Operation->LengthInBytes, >> &Transmitted, >> I2C_TRANSFER_TIMEOUT >> ); >> + Operation->LengthInBytes = Transmitted; >> } >> + >> + /* >> + * The I2C read or write transaction failed. >> + * Stop the I2C transaction. >> + */ >> + if (EFI_ERROR (Status)) { >> + MvI2cStop (I2cMasterContext); >> + break; >> + } >> + >> + /* Check if there is any more data to be sent */ >> if (Count == RequestPacket->OperationCount - 1) { >> - MvI2cStop ( I2cMasterContext ); >> + MvI2cStop (I2cMasterContext); > > Can you simply drop this non-functional change? > I'd prefer the non-adherence to coding-style over a misleading > history. > Right, I saw it after sending - I was cleaning dirty patch and splitting into 3, this line got here by mistake. > No objection to functional aspects of this patch. Ok, thanks! Marcin > > / > Leif > >> } >> } >> >> -- >> 2.7.4 >>