From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::235; helo=mail-wr0-x235.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (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 41060203525FD for ; Thu, 26 Oct 2017 05:48:01 -0700 (PDT) Received: by mail-wr0-x235.google.com with SMTP id j15so3041104wre.8 for ; Thu, 26 Oct 2017 05:51:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=u1G992aAuflalm0dpy5SWy3zXmcFkckcZs6umq8mxNc=; b=KvdaR9Pjxcgd9AaiL3yCHTk7Yitc4XBxa/yXNkuLAHRWlvGiU8CwIpS5GH1WEk18K3 exsVW2kAVeMM0ifVtZiKAOlJimdQ7WuL8dOnaYxQqsD0S5elDURH5XQpa37Uvmpem3RW BCm3GPaVu5a8DD/nJOXGiwHtP5BIiBuN5noCc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=u1G992aAuflalm0dpy5SWy3zXmcFkckcZs6umq8mxNc=; b=WptAnXcxftJQt6trvtsH3sN3xrkVob5Tzn7q+Yoe+SJyBEz/Vo1zT1AgPVEQoKGxjj iNQqNmUwXgiuePsGJhk9vX4VrW1F/Bg+lwWkN4dxwZqu1ppaKpVbGKtZDN2aiB97zyPK meRfXGfLjUBGWfQn5zCzEIny9EuHhHpzKFoqQjhh8OVzOJJGDST5ADGVLWBTHPpBfHpf fmLNsxXLka8Ws289fz04Xk5LkoaZc+Hy43eMgyFmLgPGPPKmE6v7St/aoe9bR/7BnobA zXwdcetxn3+2SiJ0AGvris40Sx64QNhdab3KF/fOrndh3Ccn5SQHGxOpiMfU7eJDkvh0 mOWA== X-Gm-Message-State: AMCzsaVNev1NUGk8SLAA/XnJ5gMrLh0d3uUP4Xi9T30K+J/mv/Ge2DFW /aG1zpKF+7K78YeLTwVYu5UdFsxwkyg= X-Google-Smtp-Source: ABhQp+SnUPpRsFZYkAL+jJEsbxGdoZKPDYLmD8utUgIVkTYo5ZktTD4J5H7KTv+yMrQtt1xe63RYAw== X-Received: by 10.223.170.202 with SMTP id i10mr5148683wrc.100.1509022306643; Thu, 26 Oct 2017 05:51:46 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c4sm3860284wre.57.2017.10.26.05.51.45 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 26 Oct 2017 05:51:45 -0700 (PDT) Date: Thu, 26 Oct 2017 13:51:43 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, neta@marvell.com, kostap@marvell.com, jinghua@marvell.com, jsd@semihalf.com, David Greeson Message-ID: <20171026125143.bki4fywgtf3ybvmm@bivouac.eciton.net> References: <1508980777-29006-1-git-send-email-mw@semihalf.com> <1508980777-29006-2-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1508980777-29006-2-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) 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 12:48:02 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? > ); > } 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. No objection to functional aspects of this patch. / Leif > } > } > > -- > 2.7.4 >