diff mbox series

[1/2] i2c: designware: Only check the first byte for SMBus block read length

Message ID 20200614210255.4641-2-sultan@kerneltoast.com (mailing list archive)
State New, archived
Headers show
Series i2c-hid: Save power by reducing i2c xfers with block reads | expand

Commit Message

Sultan Alsawaf June 14, 2020, 9:02 p.m. UTC 
From: Sultan Alsawaf <sultan@kerneltoast.com>

SMBus block reads can be broken because the read function will just skip
over bytes it doesn't like until reaching a byte that conforms to the
length restrictions for block reads. This is problematic when it isn't
known if the incoming payload is indeed a conforming block read.

According to the SMBus specification, block reads will only send the
payload length in the first byte, so we can fix this by only considering
the first byte in a sequence for block read length purposes.

Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write")
Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko June 15, 2020, 9:40 a.m. UTC | #1 
On Sun, Jun 14, 2020 at 02:02:54PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> SMBus block reads can be broken because the read function will just skip
> over bytes it doesn't like until reaching a byte that conforms to the
> length restrictions for block reads. This is problematic when it isn't
> known if the incoming payload is indeed a conforming block read.
> 
> According to the SMBus specification, block reads will only send the
> payload length in the first byte, so we can fix this by only considering
> the first byte in a sequence for block read length purposes.

I'm wondering if this overlaps with [1]. AFAIU that one is also makes sure that
the length is not a garbage.

[1]: https://lore.kernel.org/linux-i2c/20200613104109.2989-1-mans@mansr.com/T/#u

> Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write")
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>  drivers/i2c/busses/i2c-designware-master.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index d6425ad6e6a3..16d38b8fc19a 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -398,7 +398,6 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
>  	len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
>  	dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
>  	msgs[dev->msg_read_idx].len = len;
> -	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
>  
>  	return len;
>  }
> @@ -430,10 +429,11 @@ i2c_dw_read(struct dw_i2c_dev *dev)
>  			u32 flags = msgs[dev->msg_read_idx].flags;
>  
>  			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
> -			/* Ensure length byte is a valid value */
> -			if (flags & I2C_M_RECV_LEN &&
> -			    tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
> -				len = i2c_dw_recv_len(dev, tmp);
> +			if (flags & I2C_M_RECV_LEN) {
> +				/* Ensure length byte is a valid value */
> +				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
> +					len = i2c_dw_recv_len(dev, tmp);
> +				msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
>  			}
>  			*buf++ = tmp;
>  			dev->rx_outstanding--;
> -- 
> 2.27.0
>
Sultan Alsawaf June 15, 2020, 4:03 p.m. UTC | #2 
On Mon, Jun 15, 2020 at 12:40:19PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 14, 2020 at 02:02:54PM -0700, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > 
> > SMBus block reads can be broken because the read function will just skip
> > over bytes it doesn't like until reaching a byte that conforms to the
> > length restrictions for block reads. This is problematic when it isn't
> > known if the incoming payload is indeed a conforming block read.
> > 
> > According to the SMBus specification, block reads will only send the
> > payload length in the first byte, so we can fix this by only considering
> > the first byte in a sequence for block read length purposes.
> 
> I'm wondering if this overlaps with [1]. AFAIU that one is also makes sure that
> the length is not a garbage.
> 
> [1]: https://lore.kernel.org/linux-i2c/20200613104109.2989-1-mans@mansr.com/T/#u

No overlap. That looks like a similar bug for a different driver. In my case,
the adapter provides native SMBus support, so emulation is never used. This is
clear to see by looking at i2c_transfer_buffer_flags(), which only uses the
master_xfer functions provided by the adapter; it doesn't call the emulation
path at all.

Sultan
Andy Shevchenko June 15, 2020, 4:07 p.m. UTC | #3 
On Mon, Jun 15, 2020 at 7:06 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> On Mon, Jun 15, 2020 at 12:40:19PM +0300, Andy Shevchenko wrote:
> > On Sun, Jun 14, 2020 at 02:02:54PM -0700, Sultan Alsawaf wrote:
> > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > >
> > > SMBus block reads can be broken because the read function will just skip
> > > over bytes it doesn't like until reaching a byte that conforms to the
> > > length restrictions for block reads. This is problematic when it isn't
> > > known if the incoming payload is indeed a conforming block read.
> > >
> > > According to the SMBus specification, block reads will only send the
> > > payload length in the first byte, so we can fix this by only considering
> > > the first byte in a sequence for block read length purposes.
> >
> > I'm wondering if this overlaps with [1]. AFAIU that one is also makes sure that
> > the length is not a garbage.
> >
> > [1]: https://lore.kernel.org/linux-i2c/20200613104109.2989-1-mans@mansr.com/T/#u
>
> No overlap.

Thanks for clarifying.

> That looks like a similar bug for a different driver. In my case,
> the adapter provides native SMBus support, so emulation is never used. This is
> clear to see by looking at i2c_transfer_buffer_flags(), which only uses the
> master_xfer functions provided by the adapter; it doesn't call the emulation
> path at all.

But do we get an advantage if this can be done in the i2c core instead
(once for all)?
Sultan Alsawaf June 15, 2020, 5:15 p.m. UTC | #4 
On Mon, Jun 15, 2020 at 07:07:42PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2020 at 7:06 PM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
> > On Mon, Jun 15, 2020 at 12:40:19PM +0300, Andy Shevchenko wrote:
> > > On Sun, Jun 14, 2020 at 02:02:54PM -0700, Sultan Alsawaf wrote:
> > > > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > > >
> > > > SMBus block reads can be broken because the read function will just skip
> > > > over bytes it doesn't like until reaching a byte that conforms to the
> > > > length restrictions for block reads. This is problematic when it isn't
> > > > known if the incoming payload is indeed a conforming block read.
> > > >
> > > > According to the SMBus specification, block reads will only send the
> > > > payload length in the first byte, so we can fix this by only considering
> > > > the first byte in a sequence for block read length purposes.
> > >
> > > I'm wondering if this overlaps with [1]. AFAIU that one is also makes sure that
> > > the length is not a garbage.
> > >
> > > [1]: https://lore.kernel.org/linux-i2c/20200613104109.2989-1-mans@mansr.com/T/#u
> >
> > No overlap.
> 
> Thanks for clarifying.
> 
> > That looks like a similar bug for a different driver. In my case,
> > the adapter provides native SMBus support, so emulation is never used. This is
> > clear to see by looking at i2c_transfer_buffer_flags(), which only uses the
> > master_xfer functions provided by the adapter; it doesn't call the emulation
> > path at all.
> 
> But do we get an advantage if this can be done in the i2c core instead
> (once for all)?

We can't, because the adapter driver needs to know mid-transfer to look for the
payload length in the first byte, and then alter the transfer size on-the-fly.
That can't be done in the i2c core, sadly. The problem is that we don't know if
a transfer is going to be a block read or not beforehand. And altering the
transfer size mid-transfer is definitely a controller specific task.

Sultan
Jarkko Nikula June 16, 2020, 1:22 p.m. UTC | #5 
On 6/15/20 12:02 AM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> SMBus block reads can be broken because the read function will just skip
> over bytes it doesn't like until reaching a byte that conforms to the
> length restrictions for block reads. This is problematic when it isn't
> known if the incoming payload is indeed a conforming block read.
> 
> According to the SMBus specification, block reads will only send the
> payload length in the first byte, so we can fix this by only considering
> the first byte in a sequence for block read length purposes.
> 
> Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write")
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index d6425ad6e6a3..16d38b8fc19a 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -398,7 +398,6 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
>   	len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
>   	dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
>   	msgs[dev->msg_read_idx].len = len;
> -	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
>   
>   	return len;
>   }

Please update the comment about masking the flag a few lines above this 
change.

> @@ -430,10 +429,11 @@ i2c_dw_read(struct dw_i2c_dev *dev)
>   			u32 flags = msgs[dev->msg_read_idx].flags;
>   
>   			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
> -			/* Ensure length byte is a valid value */
> -			if (flags & I2C_M_RECV_LEN &&
> -			    tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
> -				len = i2c_dw_recv_len(dev, tmp);
> +			if (flags & I2C_M_RECV_LEN) {
> +				/* Ensure length byte is a valid value */
> +				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
> +					len = i2c_dw_recv_len(dev, tmp);
> +				msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
>   			}
>   			*buf++ = tmp;
>   			dev->rx_outstanding--;

With above comment change this looks good to me.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..16d38b8fc19a 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -398,7 +398,6 @@  i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
 	len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
 	dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
 	msgs[dev->msg_read_idx].len = len;
-	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
 
 	return len;
 }
@@ -430,10 +429,11 @@  i2c_dw_read(struct dw_i2c_dev *dev)
 			u32 flags = msgs[dev->msg_read_idx].flags;
 
 			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
-			/* Ensure length byte is a valid value */
-			if (flags & I2C_M_RECV_LEN &&
-			    tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
-				len = i2c_dw_recv_len(dev, tmp);
+			if (flags & I2C_M_RECV_LEN) {
+				/* Ensure length byte is a valid value */
+				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
+					len = i2c_dw_recv_len(dev, tmp);
+				msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
 			}
 			*buf++ = tmp;
 			dev->rx_outstanding--;