Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 10:27:01 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Akhil R <akhilrajeev@nvidia.com>
Cc:     "christian.koenig@amd.com" <christian.koenig@amd.com>,
        "digetx@gmail.com" <digetx@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Laxman Dewangan <ldewangan@nvidia.com>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
        "sumit.semwal@linaro.org" <sumit.semwal@linaro.org>,
        "wsa@kernel.org" <wsa@kernel.org>
Subject: Re: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read
Message-ID: <ZDe81UQpj8esAjTO@orome>
References: <20230324115924.64218-1-akhilrajeev@nvidia.com>
 <20230324115924.64218-2-akhilrajeev@nvidia.com>
 <ZC1oovV5CSfzvhd_@orome>
 <SJ1PR12MB6339EFB7B8496F95CD4225CFC0909@SJ1PR12MB6339.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="syCJBqMyhVFTAA1a"
Content-Disposition: inline
In-Reply-To: <SJ1PR12MB6339EFB7B8496F95CD4225CFC0909@SJ1PR12MB6339.namprd12.prod.outlook.com>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk


--syCJBqMyhVFTAA1a
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 05, 2023 at 04:11:31PM +0000, Akhil R wrote:
> > On Fri, Mar 24, 2023 at 05:29:23PM +0530, Akhil R wrote:
> > > Update the msg->len value correctly for SMBUS block read. The discrep=
ancy
> > > went unnoticed as msg->len is used in SMBUS transfers only when a PEC
> > > byte is added.
> > >
> > > Fixes: d7583c8a5748 ("i2c: tegra: Add SMBus block read function")
> > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > > ---
> > >  drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++++++++---------=
--
> > >  1 file changed, 26 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-=
tegra.c
> > > index 6aab84c8d22b..83e74b8baf67 100644
> > > --- a/drivers/i2c/busses/i2c-tegra.c
> > > +++ b/drivers/i2c/busses/i2c-tegra.c
> > > @@ -245,6 +245,7 @@ struct tegra_i2c_hw_feature {
> > >   * @msg_err: error code for completed message
> > >   * @msg_buf: pointer to current message data
> > >   * @msg_buf_remaining: size of unsent data in the message buffer
> > > + * @msg_len: length of message in current transfer
> > >   * @msg_read: indicates that the transfer is a read access
> > >   * @timings: i2c timings information like bus frequency
> > >   * @multimaster_mode: indicates that I2C controller is in multi-mast=
er
> > mode
> > > @@ -279,6 +280,7 @@ struct tegra_i2c_dev {
> > >  	size_t msg_buf_remaining;
> > >  	int msg_err;
> > >  	u8 *msg_buf;
> > > +	__u16 msg_len;
> > >
> > >  	struct completion dma_complete;
> > >  	struct dma_chan *tx_dma_chan;
> > > @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct
> > tegra_i2c_dev *i2c_dev,
> > >  	else
> > >  		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > >
> > > -	packet_header =3D msg->len - 1;
> > > +	packet_header =3D i2c_dev->msg_len - 1;
> > >
> > >  	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
> > >  		*dma_buf++ =3D packet_header;
> > > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev,
> > >  		return err;
> > >
> > >  	i2c_dev->msg_buf =3D msg->buf;
> > > +	i2c_dev->msg_len =3D msg->len;
> > >
> > > -	/* The condition true implies smbus block read and len is already
> > read */
> > > -	if (msg->flags & I2C_M_RECV_LEN && end_state !=3D
> > MSG_END_CONTINUE)
> > > -		i2c_dev->msg_buf =3D msg->buf + 1;
> > > -
> > > -	i2c_dev->msg_buf_remaining =3D msg->len;
> > >  	i2c_dev->msg_err =3D I2C_ERR_NONE;
> > >  	i2c_dev->msg_read =3D !!(msg->flags & I2C_M_RD);
> > >  	reinit_completion(&i2c_dev->msg_complete);
> > >
> > > +	/* *
> > > +	 * For SMBUS block read command, read only 1 byte in the first
> > transfer.
> > > +	 * Adjust that 1 byte for the next transfer in the msg buffer and m=
sg
> > > +	 * length.
> > > +	 */
> > > +	if (msg->flags & I2C_M_RECV_LEN) {
> > > +		if (end_state =3D=3D MSG_END_CONTINUE) {
> > > +			i2c_dev->msg_len =3D 1;
> > > +		} else {
> > > +			i2c_dev->msg_buf +=3D 1;
> > > +			i2c_dev->msg_len -=3D 1;
> > > +		}
> > > +	}
> > > +
> > > +	i2c_dev->msg_buf_remaining =3D i2c_dev->msg_len;
> > > +
> > >  	if (i2c_dev->msg_read)
> > > -		xfer_size =3D msg->len;
> > > +		xfer_size =3D i2c_dev->msg_len;
> > >  	else
> > > -		xfer_size =3D msg->len + I2C_PACKET_HEADER_SIZE;
> > > +		xfer_size =3D i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE;
> > >
> > >  	xfer_size =3D ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> > >
> > > @@ -1295,7 +1309,7 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev,
> > >  	if (!i2c_dev->msg_read) {
> > >  		if (i2c_dev->dma_mode) {
> > >  			memcpy(i2c_dev->dma_buf +
> > I2C_PACKET_HEADER_SIZE,
> > > -			       msg->buf, msg->len);
> > > +			       msg->buf, i2c_dev->msg_len);
> > >
> > >  			dma_sync_single_for_device(i2c_dev->dma_dev,
> > >  						   i2c_dev->dma_phys,
> > > @@ -1352,7 +1366,7 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev,
> > >  						i2c_dev->dma_phys,
> > >  						xfer_size,
> > DMA_FROM_DEVICE);
> > >
> > > -			memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, msg-
> > >len);
> > > +			memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
> > i2c_dev->msg_len);
> > >  		}
> > >  	}
> > >
> > > @@ -1408,8 +1422,8 @@ static int tegra_i2c_xfer(struct i2c_adapter
> > *adap, struct i2c_msg msgs[],
> > >  			ret =3D tegra_i2c_xfer_msg(i2c_dev, &msgs[i],
> > MSG_END_CONTINUE);
> > >  			if (ret)
> > >  				break;
> > > -			/* Set the read byte as msg len */
> > > -			msgs[i].len =3D msgs[i].buf[0];
> > > +			/* Set the msg length from first byte */
> > > +			msgs[i].len +=3D msgs[i].buf[0];
> >=20
> > I'm having trouble understanding why this change is needed. msg->len is
> > never changed in tegra_i2c_xfer_msg(), as far as I can tell, so it would
> > contain whatever was in it for the previous transfer. But since we want
> > to read the message length from the first byte, wouldn't the assignment
> > (i.e. the old code) be the right way to do that? If we add the length
> > from the first byte, we could potentially be reading more than whan the
> > first byte indicated.
> >=20
> > What am I missing?
> >=20
> The value in the first byte will contain only the number of bytes to read=
 further.
> The value excludes the first byte as well as the PEC byte.=20
> The function i2c_smbus_xfer_emulated(), in file i2c-core-smbus.c, increme=
nts
> msg->len based on 'wants_pec'. Other functions, specifically the function=
=20
> i2c_smbus_check_pec() expects msg->len to be the number of bytes of data =
+=20
> first length byte + PEC byte.
>=20
> To avoid reading more bytes, I added another parameter ' i2c_dev->msg_len'
> which will contain the exact number of bytes to read in the current xfer_=
msg().

Okay, sounds good:

Acked-by: Thierry Reding <treding@nvidia.com>

--syCJBqMyhVFTAA1a
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmQ3vNUACgkQ3SOs138+
s6GDig//aVivvLg5vlG6uOo7QIiqS45I8kzJohG81Q9Wt76vtJIDhiMGLf+y/ZSX
wHGbTZj6b732rVarjw5gqD/dtviySt6ZVu/9ppzGH84NvRmPqHu/N5ZzPTv0bsJ5
h+41D+DB9ARWEnjFJQi88EjGNw62nX6QCjVqhdZFuoNeo+sk0fZguHRaoYd6vSmk
hyfOctHtGSF2+tod6bmH3fRDo8cT3RizPO/CLG460t82oTO7jhPEi5pnK8DZYIku
2+6dnA/Zxzk5Dttiox2EDooB0oS1oKs7Y8pLkVO3f01dDvMfgSjgDL+K/tJREHxz
juF+uw4O8MLTyVzyYipQ+WxqZeoUVU+tb/Cl8UiB5En5mteesfTlupmjxwkm13dF
lnur9es02Mfdguw8rek/EKfywkd7mMLLRpV2QFLveFaxeiHLlIKOFfoBqkGevVG+
s0gEPxUf74FeZuVN58UJ5euZcUNYR6/t6c/bxezx8at3pHLnlUnUyd6roHCybxGb
HLWsHzcONr8W7elhauHXl8r3QFPE9YI1fz9jL6UlOjlZ3sUPp5T1fPbCzEsLbjnt
jOiGaqZqB9IYtx2GqXoh/gEAWR8tJjHzsoVjUe7TyXjZEAuecCA07wVrodIuRta8
/ZRxmTQFuxX3cSxF5Vgr+n3U5fYakePwfQEynu6wAZLX1pKuA3E=
=e+2e
-----END PGP SIGNATURE-----

--syCJBqMyhVFTAA1a--