From: Sultan Alsawaf <[email protected]>
Hi,
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 (2):
i2c: designware: Only check the first byte for SMBus block read length
HID: i2c-hid: Use block reads when possible to save power
drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++-
drivers/i2c/busses/i2c-designware-master.c | 10 +++++-----
2 files changed, 7 insertions(+), 6 deletions(-)
--
2.27.0
From: Sultan Alsawaf <[email protected]>
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 <[email protected]>
---
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
From: Sultan Alsawaf <[email protected]>
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.
Signed-off-by: Sultan Alsawaf <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 294c84e136d7..4b507de48d70 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -476,7 +476,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
if (size > ihid->bufsize)
size = ihid->bufsize;
- ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+ ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size,
+ I2C_M_RD | I2C_M_RECV_LEN);
if (ret != size) {
if (ret < 0)
return;
--
2.27.0
On Sun, Jun 14, 2020 at 02:02:54PM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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/[email protected]/T/#u
> Fixes: c3ae106050b9 ("i2c: designware: Implement support for SMBus block read and write")
> Signed-off-by: Sultan Alsawaf <[email protected]>
> ---
> 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
>
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> >
> > 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/[email protected]/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
On Mon, Jun 15, 2020 at 7:06 PM Sultan Alsawaf <[email protected]> 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 <[email protected]>
> > >
> > > 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/[email protected]/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)?
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 15, 2020 at 07:07:42PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2020 at 7:06 PM Sultan Alsawaf <[email protected]> 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 <[email protected]>
> > > >
> > > > 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/[email protected]/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
On 6/15/20 12:02 AM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
--
Jarkko
On 6/15/20 12:02 AM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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.
>
> Signed-off-by: Sultan Alsawaf <[email protected]>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 294c84e136d7..4b507de48d70 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -476,7 +476,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> if (size > ihid->bufsize)
> size = ihid->bufsize;
>
> - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> + ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size,
> + I2C_M_RD | I2C_M_RECV_LEN);
This causes i2c-hid compatible touchscreen to stop working for me.
Ok (with patch 1/2)
[ 9.346134] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[ 9.346141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[ 9.362082] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[ 9.385897] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[ 9.386547] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 9.386598] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 9.386616] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 9.391595] i2c_hid i2c-ELAN221D:00: resetting...
[ 9.408864] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[ 9.410162] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[ 9.418223] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[ 9.418231] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 9.418236] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 9.418531] i2c_hid i2c-ELAN221D:00: asking HID report descriptor
[ 9.418537] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=02 00
[ 9.440093] i2c_hid i2c-ELAN221D:00: Report Descriptor: 05 0d 09 04
a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03
75 06 09 51 25 3f 81 02 26 ff 00 75 08 09 48 81 02 09 49 81 02 95 01 05
01 a4 26 c0 0c 75 10 55 0f 65 11 09
Not ok (with patches 1-2/2)
[ 9.428690] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[ 9.428698] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[ 9.430017] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[ 9.430836] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[ 9.431205] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 9.431294] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 9.431314] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 9.435937] i2c_hid i2c-ELAN221D:00: resetting...
[ 9.435944] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[ 9.436150] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[ 10.461304] i2c_designware i2c_designware.3: controller timed out
[ 10.498312] i2c_designware i2c_designware.3: timeout in disabling adapter
[ 14.525115] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[ 14.525130] i2c_hid i2c-ELAN221D:00: failed to reset device.
[ 14.532507] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 14.532520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[ 14.553027] i2c_designware i2c_designware.3: timeout waiting for bus
ready
...
I don't know what causes the breakage but according to HID Over I2C
Protocol Specification the descriptor length is 16 bits. Maybe the code
misses the last byte and/or the data is off by one byte by taking the
2nd length byte as 1st data byte?
--
Jarkko
From: Sultan Alsawaf <[email protected]>
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 <[email protected]>
---
drivers/i2c/busses/i2c-designware-master.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..d22271438869 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -391,14 +391,10 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
struct i2c_msg *msgs = dev->msgs;
u32 flags = msgs[dev->msg_read_idx].flags;
- /*
- * Adjust the buffer length and mask the flag
- * after receiving the first byte.
- */
+ /* Adjust the buffer length */
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 +426,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);
+ /* Mask the flag after receiving the first byte */
+ msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
}
*buf++ = tmp;
dev->rx_outstanding--;
--
2.27.0
From: Sultan Alsawaf <[email protected]>
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.
Signed-off-by: Sultan Alsawaf <[email protected]>
---
Jarkko, could you try this?
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 294c84e136d7..739dccfc57e1 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -472,11 +472,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.27.0
On Tue, Jun 16, 2020 at 08:49:51AM -0700, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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.
> + /* 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;
AFAIR SMBus specification tells about 256. Why 255?
Andi, am I correct?
--
With Best Regards,
Andy Shevchenko
Hi Andy,
> > 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.
>
> > + /* 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;
>
> AFAIR SMBus specification tells about 256. Why 255?
>
> Andi, am I correct?
Actually the SMBUS 3.0 protocol from 2015[*] says 255:
"
D.6 255 Bytes in Process Call
The maximum number of bytes allowed in the Block Write-Block Read
Process Call (Section 6.5.8) was increased from 32 to 255.
"
But why does it matter... I see the patch is detatching itself
from smbus.
And, actually, I wonder if this is the right way to fix it, isn't
it better to fix smbus instead?
I have a patch ready that fixes the smbus transfer size, perhaps
I should rebase, test and send it.
Andi
[*] http://smbus.org/specs/SMBus_3_0_20141220.pdf
On Tue, Jun 16, 2020 at 08:18:54PM +0300, Andi Shyti wrote:
> Hi Andy,
>
> > > 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.
> >
> > > + /* 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;
> >
> > AFAIR SMBus specification tells about 256. Why 255?
> >
> > Andi, am I correct?
>
> Actually the SMBUS 3.0 protocol from 2015[*] says 255:
>
> "
> D.6 255 Bytes in Process Call
>
> The maximum number of bytes allowed in the Block Write-Block Read
> Process Call (Section 6.5.8) was increased from 32 to 255.
> "
>
> But why does it matter... I see the patch is detatching itself
> from smbus.
>
> And, actually, I wonder if this is the right way to fix it, isn't
> it better to fix smbus instead?
I think the best solution would be to modify the i2c api to allow passing in a
function pointer and a payload size length, to specify how to interpret the size
of the incoming payload, so the adapter could handle both the HID over i2c
transfer spec and SMBus block reads without needing to read more bytes than
needed.
For example, for an SMBus block read, the payload size is specified in the first
byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
is specified in the first two bytes, and there are also some device quirks
involved to reinterpret the reported size.
A nice solution would be to pass in how many bytes the i2c payload size can
contain, as well as a function pointer to evaluate the reported payload size in
a way that the caller wants. This would require modifying every i2c adapter
driver to add this functionality, but it would fix the efficiency problem faced
by i2c-hid and perhaps others.
> I have a patch ready that fixes the smbus transfer size, perhaps
> I should rebase, test and send it.
For the i2c-hid driver?
Sultan
Hi Sultan,
> > > > 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.
> > >
> > > > + /* 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;
> > >
> > > AFAIR SMBus specification tells about 256. Why 255?
> > >
> > > Andi, am I correct?
> >
> > Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> >
> > "
> > D.6 255 Bytes in Process Call
> >
> > The maximum number of bytes allowed in the Block Write-Block Read
> > Process Call (Section 6.5.8) was increased from 32 to 255.
> > "
> >
> > But why does it matter... I see the patch is detatching itself
> > from smbus.
> >
> > And, actually, I wonder if this is the right way to fix it, isn't
> > it better to fix smbus instead?
>
> I think the best solution would be to modify the i2c api to allow passing in a
> function pointer and a payload size length, to specify how to interpret the size
> of the incoming payload, so the adapter could handle both the HID over i2c
> transfer spec and SMBus block reads without needing to read more bytes than
> needed.
Can't you do that by specifying the xfer function?
When you use smbus_read/write in block or byte or whatever, smbus
always checks if there is an xfer function specified and uses
that.
If it's not specified it uses the default smbus functions with
the limitations that come with it.
> For example, for an SMBus block read, the payload size is specified in the first
> byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
> is specified in the first two bytes, and there are also some device quirks
> involved to reinterpret the reported size.
which is wrong. The 32 bytes limitation is outdated: in the link
that I gave before (i.e. this one [*]), the new SMBUS specifies
255 maximum for read/write block.
> A nice solution would be to pass in how many bytes the i2c payload size can
> contain, as well as a function pointer to evaluate the reported payload size in
> a way that the caller wants. This would require modifying every i2c adapter
> driver to add this functionality, but it would fix the efficiency problem faced
> by i2c-hid and perhaps others.
>
> > I have a patch ready that fixes the smbus transfer size, perhaps
> > I should rebase, test and send it.
>
> For the i2c-hid driver?
No, sorry, for smbus.
Now... here you are replacing "i2c_master_recv" with
"i2c_transfer_buffer_flags". I do not really like this change,
although I understand it's necessary, because we are bypassing
the real issue that is that the smbus implementation is outdated.
I have a patch for that that for a matter of time I never sent.
Andi
On Tue, Jun 16, 2020 at 09:02:54PM +0300, Andi Shyti wrote:
> Hi Sultan,
>
> > > > > 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.
> > > >
> > > > > + /* 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;
> > > >
> > > > AFAIR SMBus specification tells about 256. Why 255?
> > > >
> > > > Andi, am I correct?
> > >
> > > Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> > >
> > > "
> > > D.6 255 Bytes in Process Call
> > >
> > > The maximum number of bytes allowed in the Block Write-Block Read
> > > Process Call (Section 6.5.8) was increased from 32 to 255.
> > > "
> > >
> > > But why does it matter... I see the patch is detatching itself
> > > from smbus.
> > >
> > > And, actually, I wonder if this is the right way to fix it, isn't
> > > it better to fix smbus instead?
> >
> > I think the best solution would be to modify the i2c api to allow passing in a
> > function pointer and a payload size length, to specify how to interpret the size
> > of the incoming payload, so the adapter could handle both the HID over i2c
> > transfer spec and SMBus block reads without needing to read more bytes than
> > needed.
>
> Can't you do that by specifying the xfer function?
>
> When you use smbus_read/write in block or byte or whatever, smbus
> always checks if there is an xfer function specified and uses
> that.
>
> If it's not specified it uses the default smbus functions with
> the limitations that come with it.
The xfer functions are specified on a per-adapter basis. In the case of i2c-hid,
we need to tell the adapter to interpret the payload size in a specific way,
which I *think* is only specific to HID over i2c (i.e., using 16 bits to store
the length and then checking it for device quirks).
> > For example, for an SMBus block read, the payload size is specified in the first
> > byte and it is limited to 32 bytes. However, for HID over i2c, the payload size
> > is specified in the first two bytes, and there are also some device quirks
> > involved to reinterpret the reported size.
>
> which is wrong. The 32 bytes limitation is outdated: in the link
> that I gave before (i.e. this one [*]), the new SMBUS specifies
> 255 maximum for read/write block.
Oops. But still, for SMBus block reads, the size is limited to 8 bits. For HID
over i2c, it can be 16 bits. I don't see how we can handle this without some api
cooperation to tell the adapter what the caller is expecting to see.
> > A nice solution would be to pass in how many bytes the i2c payload size can
> > contain, as well as a function pointer to evaluate the reported payload size in
> > a way that the caller wants. This would require modifying every i2c adapter
> > driver to add this functionality, but it would fix the efficiency problem faced
> > by i2c-hid and perhaps others.
> >
> > > I have a patch ready that fixes the smbus transfer size, perhaps
> > > I should rebase, test and send it.
> >
> > For the i2c-hid driver?
>
> No, sorry, for smbus.
>
> Now... here you are replacing "i2c_master_recv" with
> "i2c_transfer_buffer_flags". I do not really like this change,
> although I understand it's necessary, because we are bypassing
> the real issue that is that the smbus implementation is outdated.
>
> I have a patch for that that for a matter of time I never sent.
Can it handle block reads (8 bit size) and HID over i2c (16 bit size)?
Sultan
On 6/16/20 6:43 PM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/i2c/busses/i2c-designware-master.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
Acked-by: Jarkko Nikula <[email protected]>
On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> 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.
>
> Signed-off-by: Sultan Alsawaf <[email protected]>
> ---
> Jarkko, could you try this?
> 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 294c84e136d7..739dccfc57e1 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -472,11 +472,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;
This still causes a regression for me.
[ 9.457656] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[ 9.457663] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[ 9.458591] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[ 9.459519] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[ 9.459526] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 9.459576] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 9.459591] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 9.464070] i2c_hid i2c-ELAN221D:00: resetting...
[ 9.464078] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[ 9.464346] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[ 10.497169] i2c_designware i2c_designware.3: controller timed out
[ 10.533940] i2c_designware i2c_designware.3: timeout in disabling adapter
[ 14.528677] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[ 14.528695] i2c_hid i2c-ELAN221D:00: failed to reset device.
[ 14.536125] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 14.536141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[ 14.556335] i2c_designware i2c_designware.3: timeout waiting for bus
ready
[ 14.565086] i2c_hid i2c-ELAN221D:00: failed to change power setting.
[ 15.584374] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 15.584395] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 15.584410] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 15.605683] i2c_designware i2c_designware.3: timeout waiting for bus
ready
[ 15.614304] i2c_hid i2c-ELAN221D:00: failed to change power setting.
...
--
Jarkko
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 0b987151f0180d48dcef44d3c67f2dc5c6564348 ("[PATCH 2/2] HID: i2c-hid: Use block reads when possible to save power")
url: https://github.com/0day-ci/linux/commits/Sultan-Alsawaf/i2c-hid-Save-power-by-reducing-i2c-xfers-with-block-reads/20200615-050443
base: https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git i2c/for-next
in testcase: suspend-stress
with following parameters:
mode: freeze
iterations: 10
on test machine: 4 threads Skylake with 8G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
kern :debug : [ 11.777472] calling acpi_cpufreq_init+0x0/0x1000 [acpi_cpufreq] @ 283
kern :debug : [ 11.777930] initcall acpi_cpufreq_init+0x0/0x1000 [acpi_cpufreq] returned -17 after 1 usecs
kern :debug : [ 11.793085] calling acpi_cpufreq_init+0x0/0x1000 [acpi_cpufreq] @ 287
kern :debug : [ 11.793473] initcall acpi_cpufreq_init+0x0/0x1000 [acpi_cpufreq] returned -17 after 0 usecs
kern :err : [ 12.118024] i2c_designware i2c_designware.0: controller timed out
kern :warn : [ 12.143529] i2c_designware i2c_designware.0: timeout in disabling adapter
kern :warn : [ 12.164029] i2c_designware i2c_designware.0: timeout waiting for bus ready
kern :err : [ 12.164433] i2c_hid i2c-ITE8396:00: failed to reset device.
kern :warn : [ 12.185027] i2c_designware i2c_designware.0: timeout waiting for bus ready
kern :err : [ 12.185431] i2c_hid i2c-ITE8396:00: failed to change power setting.
kern :info : [ 12.539283] [drm] Initialized i915 1.6.0 20200313 for 0000:00:02.0 on minor 0
kern :info : [ 12.541703] ACPI: Video Device [GFX0] (multi-head: yes rom: no post: no)
kern :info : [ 12.542333] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input9
kern :debug : [ 12.542942] probe of LNXVIDEO:00 returned 1 after 2678 usecs
kern :info : [ 12.543495] snd_hda_intel 0000:00:1f.3: bound 0000:00:02.0 (ops i915_audio_component_bind_ops [i915])
kern :debug : [ 12.544108] probe of 0000:00:02.0 returned 1 after 1235581 usecs
kern :debug : [ 12.544900] initcall i915_init+0x0/0x70 [i915] returned 0 after 733716 usecs
kern :warn : [ 12.678480] i915 0000:00:02.0: DP-1: EDID is invalid:
kern :warn : [ 12.678778] [00] BAD 00 ff ff ff ff ff ff 00 ff ff ff ff ff ff ff ff
kern :warn : [ 12.679164] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.679528] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.679892] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.680262] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.680627] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.680987] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :warn : [ 12.681361] [00] BAD ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
kern :info : [ 12.695329] fbcon: i915drmfb (fb0) is primary device
kern :warn : [ 13.226027] i2c_designware i2c_designware.0: timeout waiting for bus ready
kern :err : [ 13.226041] i2c_hid i2c-ITE8396:00: failed to change power setting.
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
Thanks,
Rong Chen
On Wed, Jun 17, 2020 at 02:17:19PM +0300, Jarkko Nikula wrote:
> On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <[email protected]>
> >
> > 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.
> >
> > Signed-off-by: Sultan Alsawaf <[email protected]>
> > ---
> > Jarkko, could you try this?
> > 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 294c84e136d7..739dccfc57e1 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -472,11 +472,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;
>
> This still causes a regression for me.
Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you
try this on 5.7 and see if it works?
In the meantime I'll bisect 5.8 to see why it's causing problems for me...
Sultan
On 6/29/20 8:43 PM, Sultan Alsawaf wrote:
> Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you
> try this on 5.7 and see if it works?
>
> In the meantime I'll bisect 5.8 to see why it's causing problems for me...
>
I see the same issue on top of v5.7:
[ 9.330514] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[ 9.334761] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[ 9.335716] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[ 9.353408] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[ 9.353416] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 9.353502] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 9.353520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[ 9.362304] i2c_hid i2c-ELAN221D:00: resetting...
[ 9.370585] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[ 9.389175] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[ 10.416458] i2c_designware i2c_designware.3: controller timed out
[ 10.476853] i2c_designware i2c_designware.3: timeout in disabling adapter
[ 11.983806] [<00000000fac753ed>] i2c_dw_isr [i2c_designware_core]
[ 14.544499] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[ 14.552123] i2c_hid i2c-ELAN221D:00: failed to reset device.
[ 14.559263] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 14.565822] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[ 14.600256] i2c_designware i2c_designware.3: timeout waiting for bus
ready
[ 14.608800] i2c_hid i2c-ELAN221D:00: failed to change power setting.
[ 15.632103] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[ 15.638460] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[ 15.646422] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
...
--
Jarkko
On Wed, Jul 01, 2020 at 11:04:01AM +0300, Jarkko Nikula wrote:
> On 6/29/20 8:43 PM, Sultan Alsawaf wrote:
> > Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you
> > try this on 5.7 and see if it works?
> >
> > In the meantime I'll bisect 5.8 to see why it's causing problems for me...
> >
> I see the same issue on top of v5.7:
Try reverting my "i2c: designware: Only check the first byte for SMBus block
read length" patch and apply the following change instead:
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -394,10 +394,12 @@ i2c_dw_read(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_read_idx].flags;
*buf = dw_readl(dev, DW_IC_DATA_CMD);
- /* Ensure length byte is a valid value */
- if (flags & I2C_M_RECV_LEN &&
- *buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0) {
- len = i2c_dw_recv_len(dev, *buf);
+ if (flags & I2C_M_RECV_LEN) {
+ /* Ensure length byte is a valid value */
+ if (*buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0)
+ len = i2c_dw_recv_len(dev, *buf);
+ else
+ len = i2c_dw_recv_len(dev, len);
}
buf++;
dev->rx_outstanding--;
Hi
On 7/1/20 6:00 PM, Sultan Alsawaf wrote:
> On Wed, Jul 01, 2020 at 11:04:01AM +0300, Jarkko Nikula wrote:
>> On 6/29/20 8:43 PM, Sultan Alsawaf wrote:
>>> Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you
>>> try this on 5.7 and see if it works?
>>>
>>> In the meantime I'll bisect 5.8 to see why it's causing problems for me...
>>>
>> I see the same issue on top of v5.7:
>
> Try reverting my "i2c: designware: Only check the first byte for SMBus block
> read length" patch and apply the following change instead:
>
This combination (the diff and this HID patch) works on top of v5.7.
I tried also these other combinations:
v5.7
- HID patch + this diff -> ok
- HID patch -> not ok
- HID + acked i2c-dw patch -> acked i2c-dw patch doesn't apply
v5.8-rc3
- acked i2c-dw patch -> ok
- HID patch -> nok
- HID patch + acked i2c-dw patch -> nok
- HID patch + this diff -> diff doesn't apply
Hopefully gives some glue. I'll be out of office for a few weeks and
unfortunately cannot test patches meanwhile.
Jarkko
From: Sultan Alsawaf <[email protected]>
This is a squash of the following:
i2c: designware: Fix transfer failures for invalid SMBus block reads
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 <[email protected]>
i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
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 <[email protected]>
i2c: designware: Allow SMBus block reads up to 255 bytes in length
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 <[email protected]>
HID: i2c-hid: Use block reads when possible to save power
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.
Signed-off-by: Sultan Alsawaf <[email protected]>
---
Hi Jarkko,
Sorry for the delayed response. Life gets in the way of the things that really
matter, like kernel hacking ;)
I fixed the issue with the i2c block reads on 5.8. I've squashed all 4 of my i2c
commits into this email for simplicity; please apply this patch on either 5.8 or
5.9 (it applies cleanly to both) and let me know if it works with your i2c-hid
touchscreen. If all is well, I will resubmit these patches individually in one
patchset, in a new thread.
Thanks,
Sultan
drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
2 files changed, 13 insertions(+), 7 deletions(-)
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;
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..5bd64bd17d94 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;
@@ -430,10 +431,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 > 0)
+ len = i2c_dw_recv_len(dev, tmp);
+ else
+ len = i2c_dw_recv_len(dev, len);
}
*buf++ = tmp;
dev->rx_outstanding--;
--
2.28.0
On Tue, Sep 15, 2020 at 02:55:48PM +0300, Jarkko Nikula wrote:
> Hi
>
> On 9/14/20 3:15 AM, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <[email protected]>
> >
> > This is a squash of the following:
> >
> > i2c: designware: Fix transfer failures for invalid SMBus block reads
> >
> > 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 <[email protected]>
> >
> > i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
> >
> > 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 <[email protected]>
> >
> > i2c: designware: Allow SMBus block reads up to 255 bytes in length
> >
> > 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 <[email protected]>
> >
> > HID: i2c-hid: Use block reads when possible to save power
> >
> > 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.
> >
> > Signed-off-by: Sultan Alsawaf <[email protected]>
> > ---
> > Hi Jarkko,
> >
> > Sorry for the delayed response. Life gets in the way of the things that really
> > matter, like kernel hacking ;)
> >
> > I fixed the issue with the i2c block reads on 5.8. I've squashed all 4 of my i2c
> > commits into this email for simplicity; please apply this patch on either 5.8 or
> > 5.9 (it applies cleanly to both) and let me know if it works with your i2c-hid
> > touchscreen. If all is well, I will resubmit these patches individually in one
> > patchset, in a new thread.
> >
> I tested this on top of fc4f28bb3daf ("Merge tag 'for-5.9-rc5-tag' of
> git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux") and seems to be
> working fine. What was the key change compared to previous version that was
> regressing for me?
This change fixed your issue (and my issue with 5.8):
--- 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;
I've attributed this change with the following commit message:
"i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
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."
Does the patch series look good to submit?
Sultan
On Sun, 13 Sep 2020, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <[email protected]>
>
> This is a squash of the following:
>
> i2c: designware: Fix transfer failures for invalid SMBus block reads
[ .... ]
> HID: i2c-hid: Use block reads when possible to save power
[ .... ]
> ---
> Hi Jarkko,
>
> Sorry for the delayed response. Life gets in the way of the things that really
> matter, like kernel hacking ;)
>
> I fixed the issue with the i2c block reads on 5.8. I've squashed all 4 of my i2c
> commits into this email for simplicity; please apply this patch on either 5.8 or
> 5.9 (it applies cleanly to both) and let me know if it works with your i2c-hid
> touchscreen. If all is well, I will resubmit these patches individually in one
> patchset, in a new thread.
>
> Thanks,
> Sultan
> drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
> drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++------
> 2 files changed, 13 insertions(+), 7 deletions(-)
I believe it makes most sense for this to go through i2c tree as a whole,
so feel free to add
Acked-by: Jiri Kosina <[email protected]>
for the drivers/hid/i2c-hid/i2c-hid-core.c hunk. Thanks,
--
Jiri Kosina
SUSE Labs
On 9/15/20 8:48 PM, Sultan Alsawaf wrote:
> On Tue, Sep 15, 2020 at 02:55:48PM +0300, Jarkko Nikula wrote:
>> I tested this on top of fc4f28bb3daf ("Merge tag 'for-5.9-rc5-tag' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux") and seems to be
>> working fine. What was the key change compared to previous version that was
>> regressing for me?
>
> This change fixed your issue (and my issue with 5.8):
> --- 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;
>
> I've attributed this change with the following commit message:
> "i2c: designware: Ensure tx_buf_len is nonzero for SMBus block reads
>
> 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."
>
Ok, nice that you found it.
> Does the patch series look good to submit?
>
Yes, go ahead.
Jarkko