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
