Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752485AbcDRORL (ORCPT ); Mon, 18 Apr 2016 10:17:11 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34888 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751807AbcDRORJ (ORCPT ); Mon, 18 Apr 2016 10:17:09 -0400 Date: Mon, 18 Apr 2016 16:17:05 +0200 From: Thierry Reding To: Shardar Shariff Md Cc: ldewangan@nvidia.com, swarren@wwwdotorg.org, linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, wsa@the-dreams.de, gnurou@gmail.com Subject: Re: [PATCH v3 2/2] i2c: tegra: proper handling of error cases Message-ID: <20160418141705.GB20508@ulmo.ba.sec> References: <1460985355-28901-1-git-send-email-smohammed@nvidia.com> <1460985355-28901-2-git-send-email-smohammed@nvidia.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="61jdw2sOBCFtR2d/" Content-Disposition: inline In-Reply-To: <1460985355-28901-2-git-send-email-smohammed@nvidia.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4315 Lines: 118 --61jdw2sOBCFtR2d/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 18, 2016 at 06:45:55PM +0530, Shardar Shariff Md wrote: > To summarize the issue observed in error cases: >=20 > SW Flow: For i2c message transfer, packet header and data payload is > posted and then required error/packet completion interrupts are enabled > later. >=20 > HW flow: HW process the packet just after packet header is posted, if > ARB lost/NACK error occurs (SW will not handle immediately when error > happens as error interrupts are not enabled at this point). HW assumes > error is acknowledged and clears current data in FIFO, But SW here posts > the remaining data payload which still stays in FIFO as stale data > (data without packet header). >=20 > Now once the interrupts are enabled, SW handles ARB lost/NACK error by > clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended > the error and will parse/process stale data (data without packet header) > present in FIFO which causes invalid NACK errors. >=20 > Fix: Enable the error interrupts before posting the packet into FIFO > which make sure HW to not clear the fifo. Also disable the packet mode > before acknowledging errors (ARB lost/NACK error) to not process any > stale data. As error interrupts are enabled before posting the packet > header use spinlock to avoid preempting. >=20 > Signed-off-by: Shardar Shariff Md >=20 > Changes since v1: > - Align the commit message to 72 characters per line. > - Removing unnecessary paranthesis. > - Printing error if tegra_i2c_disable_packet_mode() fails > is already present and handling error is not taken cared > in ISR which was done in v2. I think I didn't express myself very clearly, or gave insufficient information. The changelog reads easier if you have one entry per revision. The above would then read: Changes in v3: - Printing error if tegra_i2c_disable_packet_mode() fails is already present and handling error is not taken cared in ISR which was done in v2. Changes in v2: - Align the commit message to 72 characters per line. - Removing unnecessary paranthesis. And it should go below the --- separator. One more nitpick below. > --- > drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) >=20 > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegr= a.c > index 6235f16..7922892 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -191,6 +191,7 @@ struct tegra_i2c_dev { > u16 clk_divisor_non_hs_mode; > bool is_suspended; > bool is_multimaster_mode; > + spinlock_t xfer_lock; > }; > =20 > static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned = long reg) > @@ -510,14 +511,27 @@ err: > return err; > } > =20 > +static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev) > +{ > + u32 cnfg; > + > + cnfg =3D i2c_readl(i2c_dev, I2C_CNFG); > + if (cnfg & I2C_CNFG_PACKET_MODE_EN) > + i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG); > + > + return tegra_i2c_wait_for_config_load(i2c_dev); > +} Since you no longer use the return value you might as well just make the function return void. I don't feel very strongly about it, though, so I'll defer to Wolfram. Thierry --61jdw2sOBCFtR2d/ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXFOxhAAoJEN0jrNd/PrOhiQQP/jR9dv5aUEdLqoO/1BKEX3v2 AF4Cx+6YlIYZ9KUuDvyav0pEydKXSYirWVG8ruRGr96ZH95EXyJ2JM/d1NVmun8t SYnaUnJeJZ40TkzQPkR2EfarKzoxkT+FeDfRpK3UUcPuVk3al9C7Rhth+mwkjV8h CkLs80vAdMCkYYdTKZ7992+quNh3M2/BG4CcPI357xGeeCWTGGGWFq/ZtF7ZbaGH Fet0xj8SYNq3RJRb3W9dPYV43TdRqF6ECisyaWukDZQQ8fX51Oip0NXJFUuQIoU9 gGUVlNDJI92C+m9gGTjZjVZAiIV48mSC0kgFIvNIp5LfXjc9d0MWnI5qTq3PGGrA V7R5WwbtQbegPKv7o4zQ+AuJ50rilP/ph6IQw39H8Quxla1lqq/T+YRSWnn73JYH 2rFurqCiGD0caXalKLmcT3T0WoalS7bzwFcstLJm5LVet15zG91EHusSqHVPm5UH /CZ31hbNcBCNnhV7XAqXxwQGcafO32rRJlgsBqi05h8JbbWn2jGNAh07tRqhkIzS +sjwt4o9a0YcWsNl4CfLGuOoahYdUGrEdBuGyP9olIRc2fh4Ui1Debw4gteBPrUS +RbjSAWPePRNLRwDSwdGTmMz2isr24YEGUr+jD47KP7EAdRBreD6xYWxdyqIy2FO vh+mNgO+NffpnJvCmWHI =rtI0 -----END PGP SIGNATURE----- --61jdw2sOBCFtR2d/--