| Message ID | 20200914001523.3878-1-sultan@kerneltoast.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] i2c: Squash of SMBus block read patchset to save power | expand |
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 <sultan@kerneltoast.com> > > > > 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 <sultan@kerneltoast.com> > > > > 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 <sultan@kerneltoast.com> > > > > 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 <sultan@kerneltoast.com> > > > > 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 <sultan@kerneltoast.com> > > --- > > 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 <sultan@kerneltoast.com> > > 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 <jkosina@suse.cz> for the drivers/hid/i2c-hid/i2c-hid-core.c hunk. Thanks,
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
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--;