[PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Sultan Alsawaf]

From: Sultan Alsawaf <sultan@kerneltoast.com>

This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
xfers with block reads". That original patchset did not have enough fixes for
the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
extensively in the original email thread.

Here is the original cover letter, which still applies:
"I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
finger on the touchpad would increase my system's power consumption by 4W, which
is quite considerable. Resting my finger on the touchpad would generate roughly
4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.

Upon closer inspection, I noticed that the i2c-hid driver would always transfer
the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
my touchpad's normal touch events are only 32 bytes long according to the length
byte contained in the buffer sequence.

Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
flag in i2c-hid, which says to look for the payload length in the first byte of
the transfer buffer and adjust the i2c transaction accordingly. The only problem
though is that my i2c controller's driver allows bytes other than the first one
to be used to retrieve the payload length, which is incorrect according to the
SMBus spec, and would break my i2c-hid change since not *all* of the reports
from my touchpad are conforming SMBus block reads.

This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
drivers should cope with this and proceed with the i2c transfer using the
original requested length."

Sultan

Sultan Alsawaf (4):
  i2c: designware: Fix transfer failures for invalid SMBus block reads
  i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
  i2c: designware: Allow SMBus block reads up to 255 bytes in length
  HID: i2c-hid: Use block reads when possible to save power

 drivers/hid/i2c-hid/i2c-hid-core.c         |  5 ++++-
 drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.28.0

[PATCH v2 1/4] i2c: designware: Fix transfer failures for invalid SMBus block reads

[Sultan Alsawaf]

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.

In addition, when the length byte is invalid, the original transfer
length still needs to be adjusted to avoid a controller timeout.

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, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..d78f48ca4886 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -430,10 +430,12 @@ 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);
+				else
+					len = i2c_dw_recv_len(dev, len);
 			}
 			*buf++ = tmp;
 			dev->rx_outstanding--;
-- 
2.28.0

[PATCH v2 2/4] i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads

[Sultan Alsawaf]

From: Sultan Alsawaf <sultan@kerneltoast.com>

The point of adding a byte to len in i2c_dw_recv_len() is to make sure
that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
controller know that the i2c transaction can end. Otherwise, the i2c
controller will think that the transaction can never end for block
reads, which results in the stop-detection bit never being set and thus
the transaction timing out.

Adding a byte to len is not a reliable way to do this though; sometimes
it lets tx_buf_len become zero, which results in the scenario described
above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
the issue.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d78f48ca4886..22f28516bca7 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -395,8 +395,9 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
 	 * Adjust the buffer length and mask the flag
 	 * after receiving the first byte.
 	 */
-	len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
-	dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
+	if (flags & I2C_CLIENT_PEC)
+		len++;
+	dev->tx_buf_len = len - min_t(u8, len - 1, dev->rx_outstanding);
 	msgs[dev->msg_read_idx].len = len;
 	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
 
-- 
2.28.0

Re: [PATCH v2 2/4] i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads

[Andy Shevchenko]

On Wed, Sep 16, 2020 at 10:22:54PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> The point of adding a byte to len in i2c_dw_recv_len() is to make sure
> that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
> controller know that the i2c transaction can end. Otherwise, the i2c
> controller will think that the transaction can never end for block
> reads, which results in the stop-detection bit never being set and thus
> the transaction timing out.
> 
> Adding a byte to len is not a reliable way to do this though; sometimes
> it lets tx_buf_len become zero, which results in the scenario described
> above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
> the issue.

I got only patch 2/4, where the other 3 along with cover letter?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads

[Jarkko Nikula]

On 9/17/20 8:22 AM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> The point of adding a byte to len in i2c_dw_recv_len() is to make sure
> that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
> controller know that the i2c transaction can end. Otherwise, the i2c
> controller will think that the transaction can never end for block
> reads, which results in the stop-detection bit never being set and thus
> the transaction timing out.
> 
> Adding a byte to len is not a reliable way to do this though; sometimes
> it lets tx_buf_len become zero, which results in the scenario described
> above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
> the issue.
> 
> 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 | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
Were other patches in series dropped somewhere? I received only this.

Jarkko

Re: [PATCH v2 2/4] i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 04:44:18PM +0300, Jarkko Nikula wrote:
> On 9/17/20 8:22 AM, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > 
> > The point of adding a byte to len in i2c_dw_recv_len() is to make sure
> > that tx_buf_len is nonzero, so that i2c_dw_xfer_msg() can let the i2c
> > controller know that the i2c transaction can end. Otherwise, the i2c
> > controller will think that the transaction can never end for block
> > reads, which results in the stop-detection bit never being set and thus
> > the transaction timing out.
> > 
> > Adding a byte to len is not a reliable way to do this though; sometimes
> > it lets tx_buf_len become zero, which results in the scenario described
> > above. Therefore, just directly ensure tx_buf_len cannot be zero to fix
> > the issue.
> > 
> > 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 | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> Were other patches in series dropped somewhere? I received only this.

@linux.intel.com has some issues in delivery (accepting) messages. You may
download thru lore.kernel.org entire series and reply.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] i2c: designware: Allow SMBus block reads up to 255 bytes in length

[Sultan Alsawaf]

From: Sultan Alsawaf <sultan@kerneltoast.com>

According to the SMBus 3.0 protocol specification, block transfer limits
were increased from 32 bytes to 255 bytes. Remove the obsolete 32-byte
limitation.

Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 22f28516bca7..5bd64bd17d94 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -433,7 +433,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
 			if (flags & I2C_M_RECV_LEN) {
 				/* Ensure length byte is a valid value */
-				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
+				if (tmp > 0)
 					len = i2c_dw_recv_len(dev, tmp);
 				else
 					len = i2c_dw_recv_len(dev, len);
-- 
2.28.0

Re: [PATCH v2 3/4] i2c: designware: Allow SMBus block reads up to 255 bytes in length

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

On Wed, Sep 16, 2020 at 10:22:55PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> According to the SMBus 3.0 protocol specification, block transfer limits
> were increased from 32 bytes to 255 bytes. Remove the obsolete 32-byte
> limitation.

Sadly, it is not that easy. We are trying to extend BLOCK_MAX to 255
(SMBus 3 specs) but there are various things to be considered,
especially with buffers and when passing it to userspace. Check here for
the discussion (and you are welcome to join, of course):

http://patchwork.ozlabs.org/project/linux-i2c/list/?submitter=79741&state=*

> 
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>  drivers/i2c/busses/i2c-designware-master.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 22f28516bca7..5bd64bd17d94 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -433,7 +433,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
>  			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
>  			if (flags & I2C_M_RECV_LEN) {
>  				/* Ensure length byte is a valid value */
> -				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
> +				if (tmp > 0)
>  					len = i2c_dw_recv_len(dev, tmp);
>  				else
>  					len = i2c_dw_recv_len(dev, len);
> -- 
> 2.28.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] i2c: designware: Allow SMBus block reads up to 255 bytes in length

[Sultan Alsawaf]

On Thu, Sep 17, 2020 at 10:57:04PM +0200, Wolfram Sang wrote:
> On Wed, Sep 16, 2020 at 10:22:55PM -0700, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@kerneltoast.com>
> > 
> > According to the SMBus 3.0 protocol specification, block transfer limits
> > were increased from 32 bytes to 255 bytes. Remove the obsolete 32-byte
> > limitation.
> 
> Sadly, it is not that easy. We are trying to extend BLOCK_MAX to 255
> (SMBus 3 specs) but there are various things to be considered,
> especially with buffers and when passing it to userspace. Check here for
> the discussion (and you are welcome to join, of course):
> 
> http://patchwork.ozlabs.org/project/linux-i2c/list/?submitter=79741&state=*
> 
> > 
> > Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-master.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> > index 22f28516bca7..5bd64bd17d94 100644
> > --- a/drivers/i2c/busses/i2c-designware-master.c
> > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > @@ -433,7 +433,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
> >  			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
> >  			if (flags & I2C_M_RECV_LEN) {
> >  				/* Ensure length byte is a valid value */
> > -				if (tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0)
> > +				if (tmp > 0)
> >  					len = i2c_dw_recv_len(dev, tmp);
> >  				else
> >  					len = i2c_dw_recv_len(dev, len);
> > -- 
> > 2.28.0
> > 

Yes, it is not that easy to make the change on a global scale. However, in the
case of the designware adapter, it really *is* that easy. This change covers the
designware adapter, and others can follow later with the much more invasive
changes that are needed.

Sultan

[PATCH v2 4/4] HID: i2c-hid: Use block reads when possible to save power

[Sultan Alsawaf]

From: Sultan Alsawaf <sultan@kerneltoast.com>

We have no way of knowing how large an incoming payload is going to be,
so the only strategy available up until now has been to always retrieve
the maximum possible report length over i2c, which can be quite
inefficient. For devices that send reports in block read format, the i2c
controller driver can read the payload length on the fly and terminate
the i2c transaction early, resulting in considerable power savings.

On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
touchpad causes psys power readings to go up by about 4W and hover there
until I remove my finger. With this patch, my psys readings go from 4.7W
down to 3.1W, yielding about 1.6W in savings. This is because my
touchpad's max report length is 60 bytes, but all of the regular reports
it sends for touch events are only 32 bytes, so the i2c transfer is
roughly halved for the common case.

Acked-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index dbd04492825d..66950f472122 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -476,11 +476,14 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
 	int ret;
 	u32 ret_size;
 	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
+	u16 flags;
 
 	if (size > ihid->bufsize)
 		size = ihid->bufsize;
 
-	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+	/* Try to do a block read if the size fits in one byte */
+	flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN;
+	ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size, flags);
 	if (ret != size) {
 		if (ret < 0)
 			return;
-- 
2.28.0

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 8:26 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>
> From: Sultan Alsawaf <sultan@kerneltoast.com>
>
> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
> xfers with block reads". That original patchset did not have enough fixes for
> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
> extensively in the original email thread.
>
> Here is the original cover letter, which still applies:
> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
> finger on the touchpad would increase my system's power consumption by 4W, which
> is quite considerable. Resting my finger on the touchpad would generate roughly
> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.
>
> Upon closer inspection, I noticed that the i2c-hid driver would always transfer
> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
> my touchpad's normal touch events are only 32 bytes long according to the length
> byte contained in the buffer sequence.
>
> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
> flag in i2c-hid, which says to look for the payload length in the first byte of
> the transfer buffer and adjust the i2c transaction accordingly. The only problem
> though is that my i2c controller's driver allows bytes other than the first one
> to be used to retrieve the payload length, which is incorrect according to the
> SMBus spec, and would break my i2c-hid change since not *all* of the reports
> from my touchpad are conforming SMBus block reads.
>
> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
> drivers should cope with this and proceed with the i2c transfer using the
> original requested length."

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for I²C DesignWare patches.

>
> Sultan
>
> Sultan Alsawaf (4):
>   i2c: designware: Fix transfer failures for invalid SMBus block reads
>   i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
>   i2c: designware: Allow SMBus block reads up to 255 bytes in length
>   HID: i2c-hid: Use block reads when possible to save power
>
>  drivers/hid/i2c-hid/i2c-hid-core.c         |  5 ++++-
>  drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> --
> 2.28.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Jarkko Nikula]

On 9/17/20 5:02 PM, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 8:26 AM Sultan Alsawaf <sultan@kerneltoast.com> wrote:
>>
>> From: Sultan Alsawaf <sultan@kerneltoast.com>
>>
>> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
>> xfers with block reads". That original patchset did not have enough fixes for
>> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
>> extensively in the original email thread.
>>
>> Here is the original cover letter, which still applies:
>> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
>> finger on the touchpad would increase my system's power consumption by 4W, which
>> is quite considerable. Resting my finger on the touchpad would generate roughly
>> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.
>>
>> Upon closer inspection, I noticed that the i2c-hid driver would always transfer
>> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
>> my touchpad's normal touch events are only 32 bytes long according to the length
>> byte contained in the buffer sequence.
>>
>> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
>> flag in i2c-hid, which says to look for the payload length in the first byte of
>> the transfer buffer and adjust the i2c transaction accordingly. The only problem
>> though is that my i2c controller's driver allows bytes other than the first one
>> to be used to retrieve the payload length, which is incorrect according to the
>> SMBus spec, and would break my i2c-hid change since not *all* of the reports
>> from my touchpad are conforming SMBus block reads.
>>
>> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
>> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
>> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
>> drivers should cope with this and proceed with the i2c transfer using the
>> original requested length."
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for I²C DesignWare patches.
> 
Also for i2c-designware

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Jiri Kosina]

On Wed, 16 Sep 2020, Sultan Alsawaf wrote:

> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
> xfers with block reads". That original patchset did not have enough fixes for
> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
> extensively in the original email thread.
> 
> Here is the original cover letter, which still applies:
> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
> finger on the touchpad would increase my system's power consumption by 4W, which
> is quite considerable. Resting my finger on the touchpad would generate roughly
> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.
> 
> Upon closer inspection, I noticed that the i2c-hid driver would always transfer
> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
> my touchpad's normal touch events are only 32 bytes long according to the length
> byte contained in the buffer sequence.
> 
> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
> flag in i2c-hid, which says to look for the payload length in the first byte of
> the transfer buffer and adjust the i2c transaction accordingly. The only problem
> though is that my i2c controller's driver allows bytes other than the first one
> to be used to retrieve the payload length, which is incorrect according to the
> SMBus spec, and would break my i2c-hid change since not *all* of the reports
> from my touchpad are conforming SMBus block reads.
> 
> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
> drivers should cope with this and proceed with the i2c transfer using the
> original requested length."
> 
> Sultan
> 
> Sultan Alsawaf (4):
>   i2c: designware: Fix transfer failures for invalid SMBus block reads
>   i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
>   i2c: designware: Allow SMBus block reads up to 255 bytes in length
>   HID: i2c-hid: Use block reads when possible to save power
> 
>  drivers/hid/i2c-hid/i2c-hid-core.c         |  5 ++++-
>  drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)

Hans, Benjamin, could you please give this patchset some smoke-testing? It 
looks good to me, but I'd like it to get some testing from your testing 
machinery before merging.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]


> Hans, Benjamin, could you please give this patchset some smoke-testing? It 
> looks good to me, but I'd like it to get some testing from your testing 
> machinery before merging.

Please give me some more days. I am not fully convinced yet that this
use of I2C_M_RECV_LEN is not broken on some controllers.

Plus, I'd favor if this could go via I2C tree. It is within I2C where
the non-trivial changes are. The HID part is just the final bit. Can we
agree on that?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Jiri Kosina]

On Tue, 22 Sep 2020, Wolfram Sang wrote:

> > Hans, Benjamin, could you please give this patchset some smoke-testing? It 
> > looks good to me, but I'd like it to get some testing from your testing 
> > machinery before merging.
> 
> Please give me some more days. I am not fully convinced yet that this
> use of I2C_M_RECV_LEN is not broken on some controllers.
> 
> Plus, I'd favor if this could go via I2C tree. It is within I2C where
> the non-trivial changes are. The HID part is just the final bit. Can we
> agree on that?

Absolutely no problem with that. But I'd like to have this ran through 
Benjamin/Hans first too.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Sultan Alsawaf]

On Tue, Sep 22, 2020 at 09:59:44PM +0200, Jiri Kosina wrote:
> On Tue, 22 Sep 2020, Wolfram Sang wrote:
> 
> > > Hans, Benjamin, could you please give this patchset some smoke-testing? It 
> > > looks good to me, but I'd like it to get some testing from your testing 
> > > machinery before merging.
> > 
> > Please give me some more days. I am not fully convinced yet that this
> > use of I2C_M_RECV_LEN is not broken on some controllers.
> > 
> > Plus, I'd favor if this could go via I2C tree. It is within I2C where
> > the non-trivial changes are. The HID part is just the final bit. Can we
> > agree on that?
> 
> Absolutely no problem with that. But I'd like to have this ran through 
> Benjamin/Hans first too.
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

I suppose the HID part does need to be held off until all the adapters are
updated with functional I2C_M_RECV_LEN bits.

I just got a Ryzen laptop which panics when using I2C_M_RECV_LEN.

So it looks like only the designware changes can be considered for merging now.

Sultan

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Hans de Goede]

Hi,

On 9/22/20 11:19 AM, Jiri Kosina wrote:
> On Wed, 16 Sep 2020, Sultan Alsawaf wrote:
> 
>> From: Sultan Alsawaf <sultan@kerneltoast.com>
>>
>> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
>> xfers with block reads". That original patchset did not have enough fixes for
>> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
>> extensively in the original email thread.
>>
>> Here is the original cover letter, which still applies:
>> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
>> finger on the touchpad would increase my system's power consumption by 4W, which
>> is quite considerable. Resting my finger on the touchpad would generate roughly
>> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.
>>
>> Upon closer inspection, I noticed that the i2c-hid driver would always transfer
>> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
>> my touchpad's normal touch events are only 32 bytes long according to the length
>> byte contained in the buffer sequence.
>>
>> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
>> flag in i2c-hid, which says to look for the payload length in the first byte of
>> the transfer buffer and adjust the i2c transaction accordingly. The only problem
>> though is that my i2c controller's driver allows bytes other than the first one
>> to be used to retrieve the payload length, which is incorrect according to the
>> SMBus spec, and would break my i2c-hid change since not *all* of the reports
>> from my touchpad are conforming SMBus block reads.
>>
>> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
>> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
>> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
>> drivers should cope with this and proceed with the i2c transfer using the
>> original requested length."
>>
>> Sultan
>>
>> Sultan Alsawaf (4):
>>   i2c: designware: Fix transfer failures for invalid SMBus block reads
>>   i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
>>   i2c: designware: Allow SMBus block reads up to 255 bytes in length
>>   HID: i2c-hid: Use block reads when possible to save power
>>
>>  drivers/hid/i2c-hid/i2c-hid-core.c         |  5 ++++-
>>  drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
>>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> Hans, Benjamin, could you please give this patchset some smoke-testing? It 
> looks good to me, but I'd like it to get some testing from your testing 
> machinery before merging.

Sorry for being slow to respond to this. I have not gotten around to testing
this, but I saw another email that this breaks things on at least AMD
platforms, so I guess that this is on hold for now ?

Regards,

Hans

Re: [PATCH v2 0/4] i2c-hid: Save power by reducing i2c xfers with block reads

[Sultan Alsawaf]

On Fri, Oct 16, 2020 at 01:16:18PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/22/20 11:19 AM, Jiri Kosina wrote:
> > On Wed, 16 Sep 2020, Sultan Alsawaf wrote:
> > 
> >> From: Sultan Alsawaf <sultan@kerneltoast.com>
> >>
> >> This is a fixed resubmission of "[PATCH 0/2] i2c-hid: Save power by reducing i2c
> >> xfers with block reads". That original patchset did not have enough fixes for
> >> the designware i2c adapter's I2C_M_RECV_LEN feature, which is documented
> >> extensively in the original email thread.
> >>
> >> Here is the original cover letter, which still applies:
> >> "I noticed on my Dell Precision 15 5540 with an i9-9880H that simply putting my
> >> finger on the touchpad would increase my system's power consumption by 4W, which
> >> is quite considerable. Resting my finger on the touchpad would generate roughly
> >> 4000 i2c irqs per second, or roughly 20 i2c irqs per touchpad irq.
> >>
> >> Upon closer inspection, I noticed that the i2c-hid driver would always transfer
> >> the maximum report size over i2c (which is 60 bytes for my touchpad), but all of
> >> my touchpad's normal touch events are only 32 bytes long according to the length
> >> byte contained in the buffer sequence.
> >>
> >> Therefore, I was able to save about 2W of power by passing the I2C_M_RECV_LEN
> >> flag in i2c-hid, which says to look for the payload length in the first byte of
> >> the transfer buffer and adjust the i2c transaction accordingly. The only problem
> >> though is that my i2c controller's driver allows bytes other than the first one
> >> to be used to retrieve the payload length, which is incorrect according to the
> >> SMBus spec, and would break my i2c-hid change since not *all* of the reports
> >> from my touchpad are conforming SMBus block reads.
> >>
> >> This patchset fixes the I2C_M_RECV_LEN behavior in the designware i2c driver and
> >> modifies i2c-hid to use I2C_M_RECV_LEN to save quite a bit of power. Even if the
> >> peripheral controlled by i2c-hid doesn't support block reads, the i2c controller
> >> drivers should cope with this and proceed with the i2c transfer using the
> >> original requested length."
> >>
> >> Sultan
> >>
> >> Sultan Alsawaf (4):
> >>   i2c: designware: Fix transfer failures for invalid SMBus block reads
> >>   i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
> >>   i2c: designware: Allow SMBus block reads up to 255 bytes in length
> >>   HID: i2c-hid: Use block reads when possible to save power
> >>
> >>  drivers/hid/i2c-hid/i2c-hid-core.c         |  5 ++++-
> >>  drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
> >>  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > Hans, Benjamin, could you please give this patchset some smoke-testing? It 
> > looks good to me, but I'd like it to get some testing from your testing 
> > machinery before merging.
> 
> Sorry for being slow to respond to this. I have not gotten around to testing
> this, but I saw another email that this breaks things on at least AMD
> platforms, so I guess that this is on hold for now ?
> 
> Regards,
> 
> Hans

Hi,

Only the i2c-hid-core.c hunk is on hold. It is on hold until every i2c adapter
gets updated for proper block read functionality up to the new block read limit
of 255 bytes. This is not really a surprise.

The designware patches are good to go. Please let me know what you think of
them.

Sultan