Re: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read

[Thierry Reding]

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 discrepancy
> > > 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@xxxxxxxxxx>
> > > ---
> > >  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-master
> > 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 = msg->len - 1;
> > > +	packet_header = i2c_dev->msg_len - 1;
> > >
> > >  	if (i2c_dev->dma_mode && !i2c_dev->msg_read)
> > >  		*dma_buf++ = packet_header;
> > > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev,
> > >  		return err;
> > >
> > >  	i2c_dev->msg_buf = msg->buf;
> > > +	i2c_dev->msg_len = msg->len;
> > >
> > > -	/* The condition true implies smbus block read and len is already
> > read */
> > > -	if (msg->flags & I2C_M_RECV_LEN && end_state !=
> > MSG_END_CONTINUE)
> > > -		i2c_dev->msg_buf = msg->buf + 1;
> > > -
> > > -	i2c_dev->msg_buf_remaining = msg->len;
> > >  	i2c_dev->msg_err = I2C_ERR_NONE;
> > >  	i2c_dev->msg_read = !!(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 msg
> > > +	 * length.
> > > +	 */
> > > +	if (msg->flags & I2C_M_RECV_LEN) {
> > > +		if (end_state == MSG_END_CONTINUE) {
> > > +			i2c_dev->msg_len = 1;
> > > +		} else {
> > > +			i2c_dev->msg_buf += 1;
> > > +			i2c_dev->msg_len -= 1;
> > > +		}
> > > +	}
> > > +
> > > +	i2c_dev->msg_buf_remaining = i2c_dev->msg_len;
> > > +
> > >  	if (i2c_dev->msg_read)
> > > -		xfer_size = msg->len;
> > > +		xfer_size = i2c_dev->msg_len;
> > >  	else
> > > -		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> > > +		xfer_size = i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE;
> > >
> > >  	xfer_size = 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 = tegra_i2c_xfer_msg(i2c_dev, &msgs[i],
> > MSG_END_CONTINUE);
> > >  			if (ret)
> > >  				break;
> > > -			/* Set the read byte as msg len */
> > > -			msgs[i].len = msgs[i].buf[0];
> > > +			/* Set the msg length from first byte */
> > > +			msgs[i].len += msgs[i].buf[0];
> > 
> > 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.
> > 
> > What am I missing?
> > 
> 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. 
> The function i2c_smbus_xfer_emulated(), in file i2c-core-smbus.c, increments
> msg->len based on 'wants_pec'. Other functions, specifically the function 
> i2c_smbus_check_pec() expects msg->len to be the number of bytes of data + 
> first length byte + PEC byte.
> 
> 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@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature